Flaw in changes to save/restore bootloader magic key RAM #134
Comments
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? |
|
@yyyc514, I believe it may be something that Macs sometimes do, as mentioned here and also in the comment at line 134 in CDC.cpp. |
|
Hi @MLXXXp , 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! |
|
@facchinm, My questions are:
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:
// 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:
The continue to corrupt the location: 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. |
|
@MLXXXp , perfect analysis indeed, to answer your points:
So your solution looks the most promising, I'll try coding something with less overhead tomorrow and let you know 😉 |
|
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: |
|
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. |
|
@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);
} |
|
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 😉 |
|
FYI: arduino/Arduino#6064 |
|
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... I fear that the solution posted above does not fully cover the situation when
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. |
|
Closing as merged with 289faaa and following |
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.
I've come up with a proposal to address the problem:
Only inintiate the boot sequence (signaled by baudrate = 1200 and DTR off) if DTR was previously on.
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.
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.
The text was updated successfully, but these errors were encountered: