search

arduino / Arduino

Arduino IDE 1.x
https://www.arduino.cc/en/software
Other
14.16k stars 7.02k forks source link

Nano Every - fatal security bug in Wire.h #11221

Open mikrocoder opened 3 years ago

mikrocoder commented 3 years ago

Hallo,

wire.begin should disable 'output and 'pullup' of pin 18 and 19 before initializing TWI as a precaution. In case someone programs pin 18 and 19 digitally and then turns on TWI. Because pin 18/19 (PortF) TWI (PortA) are different ports, electrical short circuits could occur. Another problem is that Wire.end does not turn off its own port pullups of PA2 and PA3. Pin 18 and 19 are inputs, Wire is terminated and still the pins output 4.7 volts.

Translated with www.DeepL.com/Translator (free version)

#include <Streaming.h>
#include <Wire.h>

const byte wait {1};  // for enventual electrical measurements

void setup(void) 
{
  Serial.begin(250000);
  Serial.println(F("\nStart #### #### #### ####"));
  showData();

  Serial.println("\nInput_Pullup.18");  pinMode(18, INPUT_PULLUP);  
  Serial.println("Input_Pullup.19");    pinMode(19, INPUT_PULLUP);
  showData();
  delay(wait);
  Serial.println("\nOutput.18");  pinMode(18, OUTPUT);        
  Serial.println("Output.19");    pinMode(19, OUTPUT);
  showData();
  delay(wait);
  Serial.println("\nWire.begin"); Wire.begin();
  showData();
  delay(wait);
  Serial.println("\nInput.18");   pinMode(18, INPUT);  
  Serial.println("Input.19");     pinMode(19, INPUT);
  showData();
  delay(wait);
  Serial.println("\nWire.end");   Wire.end();
  showData();
}

void loop(void) 
{ }

void showData(void)
{
  Serial << "TWI0_MCTRLA    " << _HEX((uint16_t)&TWI0_MCTRLA)    << '\t' << _BIN(TWI0_MCTRLA)    << endl;
  Serial << "TWI0_SCTRLA    " << _HEX((uint16_t)&TWI0_SCTRLA)    << '\t' << _BIN(TWI0_MCTRLA)    << endl;
  Serial << "PORTA_DIR      " << _HEX((uint16_t)&PORTA_DIR)      << '\t' << _BIN(PORTA_DIR)      << endl;
  Serial << "PORTA_PIN2CTRL " << _HEX((uint16_t)&PORTA_PIN2CTRL) << '\t' << _BIN(PORTA_PIN2CTRL) << endl;
  Serial << "PORTA_PIN3CTRL " << _HEX((uint16_t)&PORTA_PIN3CTRL) << '\t' << _BIN(PORTA_PIN3CTRL) << endl;
  Serial << "PORTF_DIR      " << _HEX((uint16_t)&PORTF_DIR)      << '\t' << _BIN(PORTF_DIR)      << endl;
  Serial << "PORTF_PIN2CTRL " << _HEX((uint16_t)&PORTF_PIN2CTRL) << '\t' << _BIN(PORTF_PIN2CTRL) << endl;
  Serial << "PORTF_PIN3CTRL " << _HEX((uint16_t)&PORTF_PIN3CTRL) << '\t' << _BIN(PORTF_PIN3CTRL) << endl;
}
mikrocoder commented 3 years ago

Edit: I have delete this code here, wrong file.

If that's not what you want, then you could replace the rest of the code in the two methods with 

#if !defined(ARDUINO_AVR_NANO_EVERY)
...
#endif

frame. I don't know how this harmonizes with the big Arduino framework.

Furthermore you can consider to use a twi_disable() for a pinMode call for pin 18 and 19? This can ensure that the pins do not work against each other.

The thought with PF2/PF3 and PA2/PA3 is good, but is also somehow dangerous. :-) Perhaps one should have built a small series resistor at PF2/PF3 on the board.

Translated with www.DeepL.com/Translator (free version)

mikrocoder commented 3 years ago

And if we are going to edit TWI, we might as well delete these methods without replacement. 

uint8_t TwoWire::requestFrom(int address, int quantity)

uint8_t TwoWire::requestFrom(int address, int quantity, int sendStop)

They only generate unnecessary warnings and are good for nothing. The parameters are implicitly cast to uint8_t.

mikrocoder commented 3 years ago

Sorry, I was in the wrong twi.c file for the Nano Every.

wrong path: C:\Arduino IDE Portable\megaAVR0\arduino-1.8.13\hardware\arduino\avr\libraries\Wire\src\utility.

correct path: C:\Arduino IDE Portable\megaAVR0\arduino-1.8.13\portable\packages\arduino\hardware\megaavr\1.8.7\libraries\Wire\src\utility.

The corrected proposal for the twi.c

void TWI_MasterInit(uint32_t frequency)
{
  if(twi_mode != TWI_MODE_UNKNOWN) return;

  // Precaution due to possible electrical short circuit. PA2/PA3 and PF2/PF3 share one pin.
  #if defined(ARDUINO_AVR_NANO_EVERY)
    // turn off pullups
    PORTF_PIN2CTRL = PORTF_PIN2CTRL & ~PORT_PULLUPEN_bm; // Pin 18 (PF2)
    PORTF_PIN3CTRL = PORTF_PIN3CTRL & ~PORT_PULLUPEN_bm; // Pin 19 (PF3)
    // switch to input
    PORTF_DIRCLR = PORTF_DIRCLR | PIN3_bm | PIN2_bm; // Pin 18 & 19
  #endif

  // Enable pullups just in case, should have external ones though

  #ifdef NO_EXTERNAL_I2C_PULLUP
    pinMode(PIN_WIRE_SDA, INPUT_PULLUP);
    pinMode(PIN_WIRE_SCL, INPUT_PULLUP);
  #endif
  PORTMUX.TWISPIROUTEA |= TWI_MUX;

  twi_mode = TWI_MODE_MASTER;

  master_bytesRead = 0;
  master_bytesWritten = 0;
  master_trans_status = TWIM_STATUS_READY;
  master_result = TWIM_RESULT_UNKNOWN;

  TWI0.MCTRLA = TWI_RIEN_bm | TWI_WIEN_bm | TWI_ENABLE_bm;
  TWI_MasterSetBaud(frequency);
  TWI0.MSTATUS = TWI_BUSSTATE_IDLE_gc;
}

void TWI_SlaveInit(uint8_t address)
{
  if(twi_mode != TWI_MODE_UNKNOWN) return;

  // Precaution due to possible electrical short circuit. PA2/PA3 and PF2/PF3 share one pin.
  #if defined(ARDUINO_AVR_NANO_EVERY)
    // turn off pullups
    PORTF_PIN2CTRL = PORTF_PIN2CTRL & ~PORT_PULLUPEN_bm; // Pin 18 (PF2)
    PORTF_PIN3CTRL = PORTF_PIN3CTRL & ~PORT_PULLUPEN_bm; // Pin 19 (PF3)
    // switch to input
    PORTF_DIRCLR = PORTF_DIRCLR | PIN3_bm | PIN2_bm; // Pin 18 & 19
  #endif

  twi_mode = TWI_MODE_SLAVE;

  slave_bytesRead = 0;
  slave_bytesWritten = 0;
  slave_trans_status = TWIS_STATUS_READY;
  slave_result = TWIS_RESULT_UNKNOWN;
  slave_callUserRequest = 0;
  slave_callUserReceive = 0;

  TWI0.SADDR = address << 1;    
  TWI0.SCTRLA = TWI_DIEN_bm | TWI_APIEN_bm | TWI_PIEN_bm  | TWI_ENABLE_bm;

  /* Bus Error Detection circuitry needs Master enabled to work */
  TWI0.MCTRLA = TWI_ENABLE_bm;
}

void TWI_Disable(void)
{
  TWI0.MCTRLA = 0x00;
  TWI0.MBAUD = 0x00;
  TWI0.MSTATUS = TWI_BUSSTATE_IDLE_gc;
  TWI0.SADDR = 0x00;
  TWI0.SCTRLA = 0x00;

  twi_mode = TWI_MODE_UNKNOWN;

  // Precaution due to possible electrical short circuit. PA2/PA3 and PF2/PF3 share one pin.
  #if defined(ARDUINO_AVR_NANO_EVERY)
    // turn off pullups
    PORTA_PIN2CTRL = PORTA_PIN2CTRL & ~PORT_PULLUPEN_bm; // Pin 22 (PA2)
    PORTA_PIN3CTRL = PORTA_PIN3CTRL & ~PORT_PULLUPEN_bm; // Pin 23 (PA3)
    // switch to input
    PORTA_DIRCLR = PORTA_DIRCLR | PIN3_bm | PIN2_bm; // Pin 22 & 23
  #endif
}
mikrocoder commented 3 years ago

Why are my code tags not working properly? Is someone correcting this for me after the fact?

per1234 commented 3 years ago

Why are my code tags not working properly?

You need to use code fencing. You are using inline code blocks, which only works for single lines or less. You can learn how to use code fencing from this guide: https://guides.github.com/features/mastering-markdown/#examples

Is someone correcting this for me after the fact?

Yes. I have been adding code fencing to all your posts.

mikrocoder commented 3 years ago

Okay thanks. :thumbsup: I edited the previous post.

code tags != code tags

:smile:

To be honest, it's very cumbersome. If I may say so. But well, let's stay on the topic with the pins.