Flaw in changes to save/restore bootloader magic key RAM

[MLXXXp]

The changes added to save and restore the data at the bootloader "magic key" location in RAM are flawed. These changes were made in a commit on Apr 6, 2016 and discussed in issue arduino/Arduino#2474.

The code is located in file CDC.cpp, which as of this writing is here.

The problem is that data can be restored to the magic key location even if it hasn't been saved in the first place, possibly overwriting data that the sketch has put there. This is because received USB information can cause the else at line 132 to execute before the if statement has ever become true.

The following sketch demonstrates the problem, when compiled under IDE 1.8.1 with core version 1.6.17, for a Leonardo or other ATMega32u4 based board. overlapArray[] has been set to sit over the magic key location at RAM address 0x800. If the IDE serial monitor is opened, the printed data will show that the array data has been corrupted with the value that was at RAMEND - 1.

// Bootloader magic key overlap test

// allocate some global variable space so that...
uint8_t globalArray[1698];
// this array will overlap the bootloader magic key
uint8_t overlapArray[8];

void setup() {
  Serial.begin(9600);
  // fill the array to make sure it's not optimised out
  for (uint16_t i = 0; i < sizeof(globalArray); i++) {
    globalArray[i] = i; 
  }
  // fill the overlap array with F0 F1 F2 F3 F4 F5 F6 F7
  for (uint8_t i = 0; i < sizeof(overlapArray); i++) {
    overlapArray[i] = i + 0xF0;
  }
  delay(5000);
}

void loop() {
  Serial.print("\noverlapArray =");
  for (uint8_t i = 0; i < sizeof(overlapArray); i++) {
    Serial.print(" 0x");
    Serial.print(overlapArray[i], HEX);
  }
  Serial.print("\n*(RAMEND-1) = 0x");
  Serial.println(*(uint16_t *) (RAMEND-1), HEX);
  delay(2000);
}

I've come up with a proposal to address the problem:

1. Only inintiate the boot sequence (signaled by baudrate = 1200 and DTR off) if DTR was previously on.

2. Only restore the data if it was previously saved. Conversly, only save data if previously saved data has been restored.

This change requires 2 boolean global variables to be added and creates 54 more bytes of code.

A unified diff against the CDC.cpp file above follows. I can create a pull request for it if desired.

--- CDC.cpp	2016-07-01 07:16:50.000000000 -0400
+++ CDC.cpp.new	2017-03-02 13:10:11.498486656 -0500
@@ -36,6 +36,12 @@
 
 bool _updatedLUFAbootloader = false;
 
+static bool boot_allowed = false;
+
+#if MAGIC_KEY_POS != (RAMEND-1)
+static bool magic_key_saved = false;
+#endif
+
 #define WEAK __attribute__ ((weak))
 
 extern const CDCDescriptor _cdcInterface PROGMEM;
@@ -116,18 +122,20 @@
 #endif
 
 			// We check DTR state to determine if host port is open (bit 0 of lineState).
-			if (1200 == _usbLineInfo.dwDTERate && (_usbLineInfo.lineState & 0x01) == 0)
+			if (1200 == _usbLineInfo.dwDTERate && (_usbLineInfo.lineState & 0x01) == 0 && boot_allowed)
 			{
 #if MAGIC_KEY_POS != (RAMEND-1)
 				// Backup ram value if its not a newer bootloader.
 				// This should avoid memory corruption at least a bit, not fully
-				if (magic_key_pos != (RAMEND-1)) {
+				if (magic_key_pos != (RAMEND-1) && !magic_key_saved) {
 					*(uint16_t *)(RAMEND-1) = *(uint16_t *)magic_key_pos;
+					magic_key_saved = true;
 				}
 #endif
 				// Store boot key
 				*(uint16_t *)magic_key_pos = MAGIC_KEY;
 				wdt_enable(WDTO_120MS);
+				boot_allowed = false;
 			}
 			else
 			{
@@ -135,13 +143,19 @@
 				// twiggle more than once before stabilizing.
 				// To avoid spurious resets we set the watchdog to 250ms and eventually
 				// cancel if DTR goes back high.
-
+				if ((_usbLineInfo.lineState & 0x01) == 1) {
+					boot_allowed = true;
+				}
 				wdt_disable();
 				wdt_reset();
 #if MAGIC_KEY_POS != (RAMEND-1)
 				// Restore backed up (old bootloader) magic key data
 				if (magic_key_pos != (RAMEND-1)) {
-					*(uint16_t *)magic_key_pos = *(uint16_t *)(RAMEND-1);
+					if (magic_key_saved)
+					{
+						magic_key_saved = false;
+						*(uint16_t *)magic_key_pos = *(uint16_t *)(RAMEND-1);
+					}
 				} else
 #endif
 				{

Note that there is still a potential for boot problems if certain system variables end up overlapping the magic key location, such as _updatedLUFAbootloader, the new flags, or other USB storage, because they could be overwritten by the magic key (0x7777), or overwrite the magic key, during the watchdog timer window. This could cause incorrect program flow or corrupted USB data.

A solution for _updatedLUFAbootloader would be to not use it. Instead, test for NEW_LUFA_SIGNATURE directly in program space at CDC.cpp line 112.

For the others, I don't see that much can be done unless these variables can somehow be placed in an area guaranteed not to overlap the magic key.

As before, there's also a chance that the sketch could change a variable at the magic number location during the watchdog window and then have it set back to the saved value if the boot sequence is aborted and the watchdog is reset before triggering.

[joshgoebel]

if the boot sequence is aborted and the watchdog is reset before triggering.

I've never seen talk of this "undo" mentioned in practice... is this actually a thing or is the 250ms window have more to do with giving USB time to wind down?

[MLXXXp]

@yyyc514,
Keep in mind that USB serial (CDC) can be used for things other than talking to the boot loader. A sketch could use the Serial object to communicate with a terminal emulator over USB. If that terminal were to set to 1200 baud and then raise then lower then raise DTR during its initialisation or at some other point, that's were you would need to handle the "undo".

I believe it may be something that Macs sometimes do, as mentioned here and also in the comment at line 134 in CDC.cpp.

[facchinm]

Hi @MLXXXp ,
thanks for reporting and investigating this issue. I'm reproducing it too, consistently, but I'd rather follow another approach to solve it.
In fact, the code in that section should be executed only if the selected baud rate is 1200bps, so the easier solution is to move the check as a precondition for that code path.

diff --git a/hardware/arduino/avr/cores/arduino/CDC.cpp b/hardware/arduino/avr/cores/arduino/CDC.cpp
index 0a743e1ea..e11438aae 100644
--- a/hardware/arduino/avr/cores/arduino/CDC.cpp
+++ b/hardware/arduino/avr/cores/arduino/CDC.cpp
@@ -94,7 +94,8 @@ bool CDC_Setup(USBSetup& setup)
                        _usbLineInfo.lineState = setup.wValueL;
                }
 
-               if (CDC_SET_LINE_CODING == r || CDC_SET_CONTROL_LINE_STATE == r)
+               if ((CDC_SET_LINE_CODING == r || CDC_SET_CONTROL_LINE_STATE == r)
+                       && 1200 == _usbLineInfo.dwDTERate)
                {
                        // auto-reset into the bootloader is triggered when the port, already 
                        // open at 1200 bps, is closed.  this is the signal to start the watchdog
@@ -116,7 +117,7 @@ bool CDC_Setup(USBSetup& setup)
 #endif
 
                        // We check DTR state to determine if host port is open (bit 0 of lineState).
-                       if (1200 == _usbLineInfo.dwDTERate && (_usbLineInfo.lineState & 0x01) == 0)
+                       if ((_usbLineInfo.lineState & 0x01) == 0)
                        {
 #if MAGIC_KEY_POS != (RAMEND-1)
                                // Backup ram value if its not a newer bootloader.

How does it look? On Linux, it appears that the CDC host code tries all the baudrates available so the MAGIC_POS -> RAMEND -1 code path ("backup") is always executed, but we are avoiding the MAGIC_POS overwrite.

Would you mind testing it? You could propose a PR if you wish . Thanks!

[MLXXXp]

@facchinm,
Sorry for taking so long to respond. I wanted to take the time to properly review and test your proposal.

My questions are:

1. Can we state that for boards that use CDC serial to communicate with the bootloader, communications with the sketch itself are not allowed at 1200 baud? (Using 1200 baud isn't wise anyway, since the bootloader will be invoked whenever the port is closed for longer than the watchdog timeout.)
2. Will all programs that communicate with the Arduino using CDC, which are configured for a speed other than 1200 baud, never attempt to use 1200 baud, even briefly, during initialisation or any other time (unless it's to invoke the bootloader)?

If the answers are both "yes" then your proposal will work. We wouldn't even have to save and restore the magic key location, set the magic key back to 0, or cancel the watchdog, because the only time 1200 baud would used would be for entering the bootloader, which isn't expected to fail or be aborted by raising DTR.

If the answer to either question is "no" then there's still the possibility for corruption due to restoring the the magic key location value before it has been saved.

I modified my test sketch to help with testing:

• For simplicity, it now just forces the array to overlap the magic key location, so the old globalArray[] size doesn't have to be adjusted when other variables are added or removed.
• A button wired between GND and a pin (defined near the top) will increment all values in the array and fix any corrupted ones.

// Bootloader magic key overlap test 2

#define BUTTON_PIN A2
#define PRINT_INTERVAL 2000

#define OVERLAP_ARRAY_LEN 8
// this array will overlap the bootloader magic key
uint8_t* overlapArray;

unsigned long previousMillis = 0;

void setup() {
  Serial.begin(9600);
  pinMode(BUTTON_PIN, INPUT_PULLUP);
  overlapArray = (uint8_t *) (MAGIC_KEY_POS - ((OVERLAP_ARRAY_LEN - 2) / 2));
  // fill the overlap array with F0 F1 F2 F3 F4 F5 F6 F7
  for (uint8_t i = 0; i < OVERLAP_ARRAY_LEN; i++) {
    overlapArray[i] = i + 0xF0;
  }
}

void loop() {
  if (digitalRead(BUTTON_PIN) == LOW) {
    uint8_t x = overlapArray[0];
    for (uint8_t i = 0; i < OVERLAP_ARRAY_LEN; i++) {
      overlapArray[i] = ++x;
    }
    delay(50);
    while (digitalRead(BUTTON_PIN) == LOW) { }
    delay(50);
  }
  
  if (millis() - previousMillis >= PRINT_INTERVAL) {
    previousMillis = millis();
    Serial.print("\noverlapArray address: 0x");
    Serial.println((uint16_t) overlapArray, HEX);
    for (uint8_t i = 0; i < OVERLAP_ARRAY_LEN; i++) {
      Serial.print(" 0x");
      if (overlapArray[i] < 0x10) {
        Serial.print('0');
      }
      Serial.print(overlapArray[i], HEX);
    }
    Serial.print("\n\n*(RAMEND-1) = 0x");
    Serial.println(*(uint16_t *) (RAMEND-1), HEX);
  }
}

To cause corruption of the magic key location:

• Set the IDE Serial monitor to a speed other than 1200.
• Close the IDE.
• Open the IDE.
• Attach to USB a board with the above sketch loaded.
• Open the Serial Monitor.
• Change the baud rate to 1200 baud.

The continue to corrupt the location:
Switch back an forth between 1200 baud and another speed. Press the button to change the array values before each speed change.

This corruption is what my changes prevented, by only restoring when a value was previously saved, and only saving and initiating the bootloader when DTR was previously high.

[facchinm]

@MLXXXp , perfect analysis indeed, to answer your points:

• if the host issues a CDC_SET_LINE_CODING with 1200bps and a CDC_SET_CONTROL_LINE_STATE with DTR low and RTS high, it's clearly trying to reset the board. If the event is spurious, everything should be reverted and the watchdog cancelled. The sample code is something like https://github.com/sudar/Arduino-Makefile/blob/master/bin/ard-reset-arduino#L32
• So we should "pre-activate" the saving procedure when 1200bps is issued, check for DTR high and DTR low, activate the watchdog and save the context, restore it if the baud or any of the signals change before the watchdog event.

So your solution looks the most promising, I'll try coding something with less overhead tomorrow and let you know 😉

[MLXXXp]

It's too bad that Arduino didn't initially provide an attachInterrupt() function for the watchdog vector. Then, when the port was closed at 1200 baud, we could save the vector set by the sketch (if any) then set the vector to an ISR that sets the magic key, then arm the watchdog in Interrupt and System Reset mode. This way the key would only be set immediately before the watchdog caused a system reset, so no save/restore would be necessary. If the port came back up before the watchdog timeout, we would restore the sketch interrupt vector (and reset the watchdog, as is done currently).

There are probably too many sketches using the watchdog timer that would be impacted, to allow this change now.

On that note:
Shouldn't we be saving the current state of the watchdog timer before taking it over, and restoring it on an abort, so as not to affect sketches that may be using it?

[facchinm]

Could the following snippet work?

diff --git a/hardware/arduino/avr/cores/arduino/CDC.cpp b/hardware/arduino/avr/cores/arduino/CDC.cpp
index 0a743e1ea..268bb9bde 100644
--- a/hardware/arduino/avr/cores/arduino/CDC.cpp
+++ b/hardware/arduino/avr/cores/arduino/CDC.cpp
@@ -129,7 +129,7 @@ bool CDC_Setup(USBSetup& setup)
                                *(uint16_t *)magic_key_pos = MAGIC_KEY;
                                wdt_enable(WDTO_120MS);
                        }
-                       else
+                       else if (*(uint16_t *)magic_key_pos == MAGIC_KEY)
                        {
                                // Most OSs do some intermediate steps when configuring ports and DTR can
                                // twiggle more than once before stabilizing.

It's essentially your patch without any extra flag (the fact that the magic key is in the right place means that the reset-to-bootloader procedure has been initiated).

However, I'd spend some more time on this bug, since as you noted disabling the watchdog is not a good idea if it was previously used by the sketch.

[MLXXXp]

@facchinm, Using the magic key value itself to indicate that it has been saved is an excellent idea! I'm kicking myself for not having though of doing this. This change has worked in all my tests.

However, I'm still worried that there could be a serial communications program out there that, intentionally or in error, sends multiple CDC_SET_CONTROL_LINE_STATE commands in a row with DTR low at 1200 baud. (For example, closing the port more than once, or setting DTR low and then setting RTS low separately.) The result would be that the value at MAGIC_KEY_POS would be saved the first time but then the saved value would be overwritten with 0x7777 the second time.

To prevent this, I suggest that the value only be saved if MAGIC_KEY_POS doesn't contain 0x7777. Reading a value of 0x7777 would indicate that the original value has already been saved and shouldn't be overwritten.

---

I've also been thinking about what it means to "close the port at 1200 baud". I believe that a port is considered open if DTR is high and closed if DTR is low. Presumably, the baud rate would already be set when a port is opened or closed. Therefore, the code to handle invoking the bootloader need only be executed on a control line change, not a baud rate change. We still have to test for being at 1200 baud but the code doesn't have to be run for a CDC_SET_LINE_CODING command.

---

As for restoring the watchdog timer state after an aborted boot initiation, in case a sketch is using it, I've written some code that will do this. I've tested it with a modified version of my test sketch, which uses the watchdog in interrupt mode to blink an LED. I've also verified that it works with a sketch that uses the watchdog in reset mode, and sketches which don't use the watchdog.

---

And finally, as I've already mentioned, there is a slight chance, due to the RAM that the sketch uses, that the _updatedLUFAbootloader flag would end up being located at MAGIC_KEY_POS. If that happened, setting the key to 0x7777 would make it appear that there was a new LUFA bootloader, so the save/restore operation wouldn't work properly. Instead of setting a flag, it's better to provide a function that always directly tests program memory for NEW_LUFA_SIGNATURE. This means that a bit more code will be executed for each test but overall less code will be generated. I've put this function in CDC.cpp but it could be relocated to a more common location if it's something that should be available for additional uses.

---

The diff for my changes is:

--- org/CDC.cpp	2016-07-01 07:16:50.000000000 -0400
+++ new/CDC.cpp	2017-03-09 22:37:18.760790014 -0500
@@ -34,7 +34,7 @@
 static volatile LineInfo _usbLineInfo = { 57600, 0x00, 0x00, 0x00, 0x00 };
 static volatile int32_t breakValue = -1;
 
-bool _updatedLUFAbootloader = false;
+static u8 wdtcsr_save;
 
 #define WEAK __attribute__ ((weak))
 
@@ -57,6 +57,11 @@
 	D_ENDPOINT(USB_ENDPOINT_IN (CDC_ENDPOINT_IN ),USB_ENDPOINT_TYPE_BULK,USB_EP_SIZE,0)
 };
 
+bool isLUFAbootloader()
+{
+	return pgm_read_word(FLASHEND - 1) == NEW_LUFA_SIGNATURE;
+}
+
 int CDC_GetInterface(u8* interfaceNum)
 {
 	interfaceNum[0] += 2;	// uses 2
@@ -92,10 +97,7 @@
 		if (CDC_SET_CONTROL_LINE_STATE == r)
 		{
 			_usbLineInfo.lineState = setup.wValueL;
-		}
 
-		if (CDC_SET_LINE_CODING == r || CDC_SET_CONTROL_LINE_STATE == r)
-		{
 			// auto-reset into the bootloader is triggered when the port, already 
 			// open at 1200 bps, is closed.  this is the signal to start the watchdog
 			// with a relatively long period so it can finish housekeeping tasks
@@ -109,7 +111,7 @@
 #if MAGIC_KEY_POS != (RAMEND-1)
 			// For future boards save the key in the inproblematic RAMEND
 			// Which is reserved for the main() return value (which will never return)
-			if (_updatedLUFAbootloader) {
+			if (isLUFAbootloader()) {
 				// horray, we got a new bootloader!
 				magic_key_pos = (RAMEND-1);
 			}
@@ -121,23 +123,25 @@
 #if MAGIC_KEY_POS != (RAMEND-1)
 				// Backup ram value if its not a newer bootloader.
 				// This should avoid memory corruption at least a bit, not fully
-				if (magic_key_pos != (RAMEND-1)) {
+				if (magic_key_pos != (RAMEND-1) && *(uint16_t *)magic_key_pos != MAGIC_KEY) {
 					*(uint16_t *)(RAMEND-1) = *(uint16_t *)magic_key_pos;
 				}
 #endif
 				// Store boot key
 				*(uint16_t *)magic_key_pos = MAGIC_KEY;
+				wdtcsr_save = WDTCSR;
 				wdt_enable(WDTO_120MS);
 			}
-			else
+			else if (*(uint16_t *)magic_key_pos == MAGIC_KEY)
 			{
 				// Most OSs do some intermediate steps when configuring ports and DTR can
 				// twiggle more than once before stabilizing.
 				// To avoid spurious resets we set the watchdog to 250ms and eventually
 				// cancel if DTR goes back high.
 
-				wdt_disable();
 				wdt_reset();
+				WDTCSR |= (1<<WDCE) | (1<<WDE);
+				WDTCSR = wdtcsr_save;
 #if MAGIC_KEY_POS != (RAMEND-1)
 				// Restore backed up (old bootloader) magic key data
 				if (magic_key_pos != (RAMEND-1)) {

And for USBCore.h and USBCore.cpp to eliminate _updatedLUFAbootloader

--- org/USBCore.h	2017-01-04 12:48:12.000000000 -0500
+++ new/USBCore.h	2017-03-09 22:31:46.931795868 -0500
@@ -285,8 +285,7 @@
 // Old Caterina bootloader places the MAGIC key into unsafe RAM locations (it can be rewritten
 // by the running sketch before to actual reboot).
 // Newer bootloaders, recognizable by the LUFA "signature" at the end of the flash, can handle both
-// the usafe and the safe location. Check once (in USBCore.cpp) if the bootloader in new, then set the global
-// _updatedLUFAbootloader variable to true/false and place the magic key consequently
+// the unsafe and the safe location.
 #ifndef MAGIC_KEY
 #define MAGIC_KEY 0x7777
 #endif

--- org/USBCore.cpp	2017-01-05 06:01:03.000000000 -0500
+++ new/USBCore.cpp	2017-03-09 22:09:04.864135766 -0500
@@ -36,7 +36,6 @@
 extern const u8 STRING_MANUFACTURER[] PROGMEM;
 extern const DeviceDescriptor USB_DeviceDescriptor PROGMEM;
 extern const DeviceDescriptor USB_DeviceDescriptorB PROGMEM;
-extern bool _updatedLUFAbootloader;
 
 const u16 STRING_LANGUAGE[2] = {
 	(3<<8) | (2+2),
@@ -815,12 +814,6 @@
 	UDIEN = (1<<EORSTE) | (1<<SOFE) | (1<<SUSPE);	// Enable interrupts for EOR (End of Reset), SOF (start of frame) and SUSPEND
 	
 	TX_RX_LED_INIT;
-
-#if MAGIC_KEY_POS != (RAMEND-1)
-	if (pgm_read_word(FLASHEND - 1) == NEW_LUFA_SIGNATURE) {
-		_updatedLUFAbootloader = true;
-	}
-#endif
 }
 
 void USBDevice_::detach()

My test sketch that uses the watchdog in interrupt mode:

// Bootloader magic key overlap test 3

#include <avr/wdt.h>

#define BUTTON_PIN A2

#define LED_PIN_PORT PORTC
#define LED_PIN_DDR DDRC
#define LED_PIN PORTC7
#define LED_PIN_MASK bit(LED_PIN)

#define PRINT_INTERVAL 2000

#define OVERLAP_ARRAY_LEN 8
// this array will overlap the bootloader magic key
uint8_t* overlapArray;

unsigned long previousMillis = 0;

void setup() {
  Serial.begin(9600);
  pinMode(BUTTON_PIN, INPUT_PULLUP);
  bitSet(LED_PIN_DDR, LED_PIN); // set the pin to output mode
  bitClear(LED_PIN_PORT, LED_PIN);
  overlapArray = (uint8_t*) (MAGIC_KEY_POS - ((OVERLAP_ARRAY_LEN - 2) / 2));
  // fill the overlap array with F0 F1 F2 F3 F4 F5 F6 F7
  for (uint8_t i = 0; i < OVERLAP_ARRAY_LEN; i++) {
    overlapArray[i] = i + 0xF0;
  }
  wdt_enable(WDTO_1S);
  WDTCSR |= bit(WDIE);
}

void loop() {
  uint8_t x;
  while (digitalRead(BUTTON_PIN) == LOW) {
    x = overlapArray[0];
    for (uint8_t i = 0; i < OVERLAP_ARRAY_LEN; i++) {
      overlapArray[i] = ++x;
    }
  }
  
  if (millis() - previousMillis >= PRINT_INTERVAL) {
    previousMillis = millis();
    Serial.print("\noverlapArray address: 0x");
    Serial.println((uint16_t) overlapArray, HEX);
    for (uint8_t i = 0; i < OVERLAP_ARRAY_LEN; i++) {
      Serial.print(" 0x");
      if (overlapArray[i] < 0x10) {
        Serial.print('0');
      }
      Serial.print(overlapArray[i], HEX);
    }
    Serial.print("\n\n*(RAMEND-1) = 0x");
    Serial.println(*(uint16_t*) (RAMEND-1), HEX);
    Serial.print("WDTCSR = ");
    Serial.println(WDTCSR, BIN);
  }
}

ISR(WDT_vect) {
  LED_PIN_PORT ^= LED_PIN_MASK;
  WDTCSR |= bit(WDIE);
}

[facchinm]

Great! Would you like to open a PR with this patch? I only have some doubts about

-		if (CDC_SET_LINE_CODING == r || CDC_SET_CONTROL_LINE_STATE == r)

but we can sort them out once the PR is ready 😉

[NicoHood]

FYI: arduino/Arduino#6064

[KelrobGithub]

I would really appreciate to restore the WDT settings defined by the sketch instead of disabling the WDT.

Background is that I've used the ATMega32U4 in a high power amplifier with the WDT enabled for safety reasons. Thinking of being at the safe side we went to a lab for EMI/EMC testing. During immunity tests if faced with high electromagnetic energy our amp failed and the control firmware hooked up. No watchdog fired which has well been tested before...
The difference was that a serial communication has been established shortly after powering up the system, and as I do know now that has disabled the wachtdog (because it occured later in time than my call to enable the watchdog in the setup function).

I fear that the solution posted above does not fully cover the situation when

• a sketch activates the watchdog
• and then the serial port is opened at a baud rate >1200 bps.

The proposed code now sets the watchdog to the content of "wdtcsr_save" which is not initialized. This agains alters the WDT behavior.

So, please add the change to reset the WDT to the behavior defined by the sketch and do not disable it anymore as the current implementation renders the WDT useless from my point of view.
But initialize "wdtcsr_save" to 0xFF and only apply it to the WDT register if it does not contain 0xFF any more (0xFF is a value which is not the initial register value and which is defined by Atmel for future use so it should not appear when being read).

[facchinm]

Closing as merged with 289faaa and following