Critical Leonardo + Micro Bootkey conflicts

[NicoHood]

The way we write the Bootloader key in the ram is not reliable. You can destroy important data since you can revert the bootloader reset. With a 32u4 it might be very unlikely (because of that much ram) but still possible and then people are wondering about weird bugs. And with a 16u2 (500b ram) it is even more likely that this will happen if the reset is reverted.

Referring to this lines:
https://github.com/arduino/Arduino/blob/master/hardware/arduino/cores/arduino/CDC.cpp#L94-L108

So what we need to do is to ensure this address is really NOT USED.

3 Solutions i have in mind:
Reserve the Bootkey address so its not used in any case. See this for a possible solution:
http://electronics.stackexchange.com/questions/140516/where-to-put-bootloader-key-in-ram-what-address-is-the-last-ram-address

The other solution would be to force a reset in any case or add some other debounce routines.

The last might be the best but 'biggest' change. Also save the value to a fixed address, add this to the bootloader and Arduino core. Shrink the size to 8bit since its reserved anyways and move it to the beginning of the ram to not fragment data. I know you wont be happy about this since you need to update all Bootloaders and the USB Core.

I think the simplest option to fix this is the first. Someone should have thought about this first but well, it would be a mess to tell the customers now to update the bootloader and most just cant do it.
But what we should do for further boards is to use the first ram address 0x100 and a 8 bit value (option3). I will also use this for my 16u2/32u2 core that i will publish soon (also as pullup request maybe).

[NicoHood]

A possible solution can be found here (2nd option):
https://github.com/NicoHood/HID/blob/master/CDC.cpp#L135-L146

[NicoHood]

Also the Bootloader itself should be fixed. Because it starts the sketch when the USB clock is still on. A better way would be a complete watchdog reset with different fuses then (like HoodLoader2).

Is anyone willing to accept/improve that? I would work on this, but not if nobody cares and the work is for nothing.

[joshgoebel]

Is there some way for a sketch to override the 0x800 location in CDC.cpp (part of core). I can imagine various ways to fix this but all of them require changing that address in CDC to something else... is there any each way for a sketch (or library) to do this without having to create a whole custom Arduino core lib?

[joshgoebel]

It seems for a "default" Arduino program the top 2 bytes of the stack should always be usable anyways... __do_global_ctors makes a CALL to main, pushing the return address onto the stack. main will call setup then just go into a tight for (;;) loop calling loop() and will never return. So it seems that return address will technically not ever be needed. So since this is kind of a "rare" things anyways why not make the "magic" address the top of the stack vs the middle of active RAM?

It seems far less likely to corrupt user space by using the top 2 bytes of the stack vs 2 bytes in the middle of user RAM.

[NicoHood]

The implementation is here:
https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/cores/arduino/CDC.cpp#L97

That sounds like a good idea. The only problem we have is that we stick in an endless loop, waiting for the MCU to reset. They have added a revert of the reset because on Macs there were problems were the OS accidentally set and removed the dtr line as far as I understood. However I am not using this on my version anyways.

The bigger problem we have is that every Leonardo needs this update. And that there will be two version of the IDE core files. You could write those bytes to the stack top of course with function. But then you need access to the DTR states which is not possible with the current IDE core. In my HID core it would work with a Controle Line Event.

But if we not stick to compatibility, or at least think of a better solution for further boards, we could do something like that. It would be interesting how the bootloader should get those 2 bytes after the reset. Isnt the stack pointer resetted afterwards?

[joshgoebel]

But if we not stick to compatibility, or at least think of a better solution for further boards, we could do something like that. It would be interesting how the bootloader should get those 2 bytes after the reset. Isnt the stack pointer resetted afterwards?

The location would be fixed (depending on hardware ram size). The 2 bytes would always be the top 2 bytes of RAM (0xAFF on Leonardo). My point was that for all default Arduino compiles this should actually be "safe" since the main function is never returned from - making the 2 byte return address on the top of the stack actually memory that really never serves a purpose at all. IE, it's "free" - and doesn't require us to steal 2 bytes from anywhere else.

Good questions about backwards compatibility... some thorny issues there.

[NicoHood]

Thats great! Can you provide some code to see how it could work?
Ergo: cant we add a compiler flag to the main function that it never returns to save those 2 bytes on any other AVR?

[joshgoebel]

There isn't really any code to show. You'd just change the magic 0x800 address to 0xAFE, and you're done.

Ergo: cant we add a compiler flag to the main function that it never returns to save those 2 bytes on any other AVR?

It's only Arduino's main() that never returns. To the compiler main is still called and technically could return. There is code to handle exiting (technically just putting the processor into a loop). The compiler expects methods to return. If you really needed those 2 extra bytes of RAM you could of course provide your own main function and just move the stack pointer manually back to the top.

For most software there are easier ways to get 2 bytes back though.

[NicoHood]

is there a definition for 0xAFE aka the end of the stack? and why 0xAFE? I am not sure if this is correct.

[joshgoebel]

On Leonardo the top of ram (and beginning stack pointer) is 0xAFF, making 0xAFE the first address you could store a 2 byte sequence for the magic 0x7777 number currently used.

[NicoHood]

Were can I read up those values?
I also need this for 2kb ram, 1kb ram and 500b ram.

Edit: is RAMEND as definition related to this?

[joshgoebel]

I'd search for the chip specific PDFs on Amtels site and then learn about the AVR memory architecture in general. The address would of course have to be different for <2k chips. Of course I can't see how the current code would work at all there since it requires 2k or RAM.

Edit: is RAMEND as definition related to this?

Yes, using RAMEND-1 is really what we'd do if we wanted to hijack the top of the stack.

[NicoHood]

RAMEND-1 seems a good solution to me. I need to test this though. Doesnt it have to be RAMEND-2 for 2 bytes?

[joshgoebel]

No.

[matthijskooijman]

Wouldn't it be possible to declare a global variable and force it to end up at a specific location? Or does that require a custom linker script? I haven't looked closely at this option, just thought of it now.

[joshgoebel]

Sure, at the cost of losing a byte or two of RAM that can't be used for anything else. I think that requires passing command line options. I like using RAMEND since that memory is essentially "free" as it's already stuck on the stack and you aren't taking away memory that would otherwise be available to the sketch (without some manual SP hacking).

[NicoHood]

RAMEND seems useful because for any other option you have to add special linker stuff which I never got working for the IDE. And it works on any MCU. Still have to test this suggestion...

[joshgoebel]

It might even be possible with some research to set *RAMEND back to some valid-ish jump location in the event a reboot is cancelled... so if main ever really did quit... I'd have to look at how the default exit handler code is compiled right now. ...but at this point that would really be more of an academic exercise than anything.

Still have to test this suggestion...

What, using RAMEND? :-)

[NicoHood]

Yeah had no time yet.
I dont knwo what you are talking about but I think RAMEND (from the idea at least) is saver than just using a more or less random ram location as it is right now.

Edit: What we can do is to write it both locations, so newer bootloaders get the use of it. And then we can finally remove it after some time, that fixed address.

[joshgoebel]

Yes, RAMEND is way safer - for all intents and purposes unused after a program books.

What we can do is to write it both locations, so newer bootloaders get the use of it. And then we can finally remove it after some time, that fixed address.

Yep, that would be a safe transition plan.

[joshgoebel]

How does it fix everything if it's not touching CDC.cpp?

[facchinm]

I recently took a look at Hoodloader2, to use it as 32u4 bootloader.
It works great except for the led dim, and by packing the latest LUFA code it inherits support for writing infos in a specific place of the flash (the end).
Using this feature, if you apply the following patch to the core the "reboot to bootloader" sequence will work out of the box with both Caterina and Hoodloader without requiring any action to the user.

diff --git a/hardware/arduino/avr/cores/arduino/CDC.cpp b/hardware/arduino/avr/cores/arduino/CDC.cpp
index f19b44c..c7766ff 100644
--- a/hardware/arduino/avr/cores/arduino/CDC.cpp
+++ b/hardware/arduino/avr/cores/arduino/CDC.cpp
@@ -102,21 +102,27 @@ bool CDC_Setup(USBSetup& setup)
 #ifndef MAGIC_KEY
 #define MAGIC_KEY 0x7777
 #endif
-#ifndef MAGIC_KEY_POS
-#define MAGIC_KEY_POS 0x0800
-#endif
+
+#define NEW_LUFA_SIGNATURE 0xDCFB
+
+           uint16_t magic_key_pos = 0x0800;
+
+           if (pgm_read_word(0x7FFE) == NEW_LUFA_SIGNATURE) {
+               // horray, we got a new bootloader!
+               magic_key_pos = (RAMEND-1);
+           }

            // We check DTR state to determine if host port is open (bit 0 of lineState).
            if (1200 == _usbLineInfo.dwDTERate && (_usbLineInfo.lineState & 0x01) == 0)
            {
-#if MAGIC_KEY_POS != (RAMEND-1)
-               *(uint16_t *)(RAMEND-1) = *(uint16_t *)MAGIC_KEY_POS;
-               *(uint16_t *)MAGIC_KEY_POS = MAGIC_KEY;
-#else
-               // for future boards save the key in the inproblematic RAMEND
-               // which is reserved for the main() return value (which will never return)
-               *(uint16_t *)MAGIC_KEY_POS = MAGIC_KEY;
-#endif
+               if (magic_key_pos != (RAMEND-1)) {
+                   *(uint16_t *)(RAMEND-1) = *(uint16_t *)magic_key_pos;
+                   *(uint16_t *)magic_key_pos = MAGIC_KEY;
+               } else {
+                   // for future boards save the key in the inproblematic RAMEND
+                   // which is reserved for the main() return value (which will never return)
+                   *(uint16_t *)magic_key_pos = MAGIC_KEY;
+               }
                wdt_enable(WDTO_120MS);
            }
            else
@@ -128,11 +134,11 @@ bool CDC_Setup(USBSetup& setup)

                wdt_disable();
                wdt_reset();
-#if MAGIC_KEY_POS != (RAMEND-1)
-               *(uint16_t *)MAGIC_KEY_POS = *(uint16_t *)(RAMEND-1);
-#else
-               *(uint16_t *)MAGIC_KEY_POS = 0x0000;
-#endif
+               if (magic_key_pos != (RAMEND-1)) {
+                   *(uint16_t *)magic_key_pos = *(uint16_t *)(RAMEND-1);
+               } else {
+                   *(uint16_t *)magic_key_pos = 0x0000;
+               }
            }
        }
        return true;

The patch brings no extra RAM usage compared with the static version (+60byte flash indeed)
I'm wondering if there is any drawback in this approach, so I'm asking you if you can see any 😄
@cmaglie @NicoHood @yyyc514 @davidperrenoud

[NicoHood]

Wow. The progmem idea is great!!!

However, before you apply this patch, let me check the free progmem space. HoodLoader2 uses almost 4kb of the bootloader. If we can make it compatible with the ide, it would be awesome. So please let us use the address at the very end of progmem/flash, so I can give away those 2 bytes.

Edit: Please also consider that the 32u4 isnt the only usb mcu, we have to adapt it to the 16u2 as well. via #ifdef or via FLASHEND-X.

I also confirmed that Hoodloader2 does solve this bug:

uint8_t array[2148];

void setup(){}

void loop()
{
    int offset = 1500;
    memset(array + offset, 0xff, sizeof(array) - offset);
}

If we change the bootloader, can we please do this upstream at lufa? The lufa leonardo bootloader does not work out of the box with the ide, I wanted to patch it for a long time now. We could patch it there and then just change the VIDs with the IDE. Please. Also we could then switch to a newer version of avr-gcc (4.7, 4.8, 4.9 or 5.1). This would be very nice, since the current bootloader is not compileable under linux.

We also need to fix the other issues (like usb core still running after programming). This would also enable us to deactivate the usb core fully. The 32u4 is very good even without usb support.

@facchinm How did you edit the lufa source files, so it reserves this position from the maximum available bytes? I'd like to use FLASHEND-1, or as an alternative only a single byte. Was there a reason you choose the number above? How about coding a version number into it? Maybe Lufa version in the lower, and bootloader version in the upper byte? Or something that at least makes sense.

Edit: It seems that this is the lufa signature, and the old bootloader doesnt have this option. This means that my HoodLoader2 is possibly already compatible with this methode?
https://github.com/abcminiuser/lufa/blob/master/Bootloaders/CDC/BootloaderCDC.txt#L185-L188

[facchinm]

Yep, Hoodloader 2 is already compatible and the last two bytes of the flash will always contain that signature in newer bootloader so it's perfectly forward-compatible

[facchinm]

Indeed, 0x7FFE is (FLASHEND-1) but I didn't find a suitable macro. Glad to change it if it exist however!

[NicoHood]

I tried you patch, and its the best idea i've heard for a few month now. This is probably the best and only solution to solve this issue.

What is most important now is to update the bootloader properly and dont ship a buggy bootloader again. So I suggest we apply this patch now (since it does not cause any problems with current boards) but develop the new bootloader with concentration. Maybe as upstream PR to LUFA as already suggested.

I improved the patch like this:

diff --git a/hardware/arduino/avr/cores/arduino/CDC.cpp b/hardware/arduino/avr/cores/arduino/CDC.cpp
index f19b44c..a2c4ea7 100644
--- a/hardware/arduino/avr/cores/arduino/CDC.cpp
+++ b/hardware/arduino/avr/cores/arduino/CDC.cpp
@@ -102,21 +102,43 @@ bool CDC_Setup(USBSetup& setup)
 #ifndef MAGIC_KEY
 #define MAGIC_KEY 0x7777
 #endif
+
+// Legacy magic key position. Looks for old and new boot key position.
+// Use RAMEND-1 as MAGIC_KEY_POS to save a few bytes of flash and force the new bootloader.
 #ifndef MAGIC_KEY_POS
 #define MAGIC_KEY_POS 0x0800
 #endif

+#ifndef NEW_LUFA_SIGNATURE
+#define NEW_LUFA_SIGNATURE 0xDCFB
+#endif
+
+       uint16_t magic_key_pos = MAGIC_KEY_POS;
+
+// If we don't use the new RAMEND directly, check manually if we have a newer bootloader.
+// This is used to keep compatible with the old leonardo bootloaders.
+// You are still able to set the magic key position manually yo RAMEND-1 to save a few bytes for this check.
+#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 (pgm_read_word(FLASHEND - 1) == NEW_LUFA_SIGNATURE) {
+              // horray, we got a new bootloader!
+              magic_key_pos = (RAMEND-1);
+           }
+#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 MAGIC_KEY_POS != (RAMEND-1)
-               *(uint16_t *)(RAMEND-1) = *(uint16_t *)MAGIC_KEY_POS;
-               *(uint16_t *)MAGIC_KEY_POS = MAGIC_KEY;
-#else
-               // for future boards save the key in the inproblematic RAMEND
-               // which is reserved for the main() return value (which will never return)
-               *(uint16_t *)MAGIC_KEY_POS = MAGIC_KEY;
+               // 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)) {
+                  *(uint16_t *)(RAMEND-1) = *(uint16_t *)magic_key_pos;
+               }
 #endif
+               // Store boot key
+               *(uint16_t *)magic_key_pos = MAGIC_KEY;
                wdt_enable(WDTO_120MS);
            }
            else
@@ -128,10 +150,17 @@ bool CDC_Setup(USBSetup& setup)

                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);
+               } else {
+#endif
+                   // Clean up RAMEND key
+                   *(uint16_t *)magic_key_pos = 0x0000;
 #if MAGIC_KEY_POS != (RAMEND-1)
-               *(uint16_t *)MAGIC_KEY_POS = *(uint16_t *)(RAMEND-1);
-#else
-               *(uint16_t *)MAGIC_KEY_POS = 0x0000;
+               }
 #endif
            }
        }

[facchinm]

Hi Nico,
it was quite difficult to understand the rationale behind your patch but at the end I made it 😉
To make it more comprehensible, I would add

 #ifndef MAGIC_KEY_POS
 #define MAGIC_KEY_POS 0x0800
+#define LEGACY_BOOTLOADER
 #endif

and use this for all the subsequent checks

-#if MAGIC_KEY_POS != (RAMEND-1)
+#ifdef LEGACY_BOOTLOADER

Anyway, I'd go with this solution! Time to start cooking a new bootloader 😄

[NicoHood]

The patch you added is not good. Because you cannot undo it again. You need to add something like "if not defined NEW_CDC_BOOTLOADER define legacy". Then you are able to explicitely use the new bootloader. That would also make sense to me.

Would you like to open a PR for this patch or should I do this? (basically it was your great idea)

Who will patch the CDC bootloader. I'd like to review it for sure, I could also do it myself. But I cannot promise anything. The reason why I am asking is to not start both parallel with the same ideas.

A bit off topic:
I really need this HID related PR merged, could you have a look please? #4105

[facchinm]

Defining a new MAGIC_KEY_POS will also mean that the bootloader is new, right? Because the bug arises from the magic key position, and leaving it as is in my opinion means that the bootloader has not been updated 😄
By the way, I'll open the PR today!
About #4105 I'm commenting there

[facchinm]

PR created (#4280)

[NicoHood]

Defining MAGIC_KEY_POS does mean nothing. It just tells where to search for the boot key.
For example the u2 boards of mattairtech used 0x280 as magic key position (and my hoodloader as well before). So one can specify this for those boards. This was required, because the u2 board had no such memory location 0x800.

Defining MAGIC_KEY_POS to RAMEND-1 means its the new bootloader. Then we specify explicte to use the new position instead of any other. This will save some flash to our programs. This could be useful on further arduino boards, such as arduboy and others.

[joshgoebel]

Any idea on the status of this? We're wanting to know if there is anything we can do with Arduboy to make it a little more "future proof"... I guess all we could really do at this point is make our boot loader look at both the OLD magic key position and the NEW (RAMEND-1)?

Then when Arduino IDE is updated you could compile Arduboy sketches for it with the new locations and the boot loader would do the right thing when it was entered after the WDT resets?

Seems that is the best we could do at this point?

[NicoHood]

You should orientate on the official lufa bootloader and patch this one to work with the arduino ide. Then apply the RAMEND addition and you are fine. Fixing the old bootloader is not an option. There are other issues which needs to be addressed too! Arduino should do this, but it seems we do not get any information about this...

What about placing a bounty?

[joshgoebel]

What are the other big issues with the old bootloader? I believe Arduboy is using the Caterina bootloader that is currently in the Arduino repository.

Not sure we want to be the guineu pigs testing entirely new stuff if all we KNOW we want is a 1 line patch to a memory address. :-)

[NicoHood]

We also need to change the sketch loading process as described somewhere above i guess.
Otherwise the usb clock still runs after the sketch is loaded and you cannot code stuff without usb core. Not only that register setting is kept, also a lot more which might lead to weird bugs. Yeah and other improvements ive listed on the hoodloader2 page.

Edit:
also things like this: arduino/ArduinoCore-samd#62 (comment)

[joshgoebel]

Also the Bootloader itself should be fixed. Because it starts the sketch when the USB clock is still on. A better way would be a complete watchdog reset with different fuses then (like HoodLoader2).

There isn't a reset after a sketch is uploaded? How does one do a "complete" reset?

[NicoHood]

Via Watchdog you do a complete reset. And the the bootloader runs again, but does not configure the usb clock and enters the sketch directly.

Now it does it like that:

• sketch resets via watchdog (triggered by pc 1200baud touch)
• bootloader runs, selfprogramms
• bootloader starts sketch
• usb clock still running, and other registers already set

How it should work:

• sketch resets via watchdog (triggered by pc 1200baud touch)
• bootloader runs, selfprogramms
• bootloader does a watchdog reset again (without magic key)
• bootloader starts sketch (cause no magic key was set)
• usb clock is NOT running, and other registers are clean

[joshgoebel]

Is the USB clock not necessary to detect being plugged into USB when already powered?

[NicoHood]

It is required to run usb functionality. Therefor you need to enable it. Thats what the Arduino sketch does. But the bootloader also leaves it running. This is not a problem unless you want to not use usb functions inside the program. But the usb clock is just an example of configurations that are not reset.

[facchinm]

"Solved" with 4c901d3, next iteration of bootloaders will expect the key on both legacy and safe locations

[NicoHood]

Great to see this merged!
However this issue is still not fixed, as there is no fixed bootloader available yet.