Fix Issue #918: Enable avrdude to send full input file incl trailing 0xff

[stefanrueger]

Piggy-backs "Do not remove trailing 0xff sequences from avr reads or file input" onto option -D, see Issue #918

The rationale is that some SPM programmers (bootloaders) are unable to carry out chip erase, but perfectly able to carry out paged writes clearing the memory to 0xff using SPM page erase internally. These SPM programmers need to see the full input file including any trailing 0xff.

This PR provides this functionality under -D.

Testing with this sketch-ending-in-ff.hex now works as it should:

$ avrdude -Dqp atmega328p -c arduino -b 115200 -P /dev/ttyUSB0 -U flash:w:sketch-ending-in-ff.hex:i

avrdude: AVR device initialized and ready to accept instructions
avrdude: Device signature = 0x1e950f (probably m328p)
avrdude: reading input file "sketch-ending-in-ff.hex"
avrdude: writing flash (2185 bytes):
avrdude: 2185 bytes of flash written
avrdude: verifying flash memory against sketch-ending-in-ff.hex:
avrdude: 2185 bytes of flash verified

avrdude done.  Thank you.

Documentation will be changed when/if this PR is accepted

[dl8dtl]

I'd prefer a separate option.
After all, the optimization to not program 0xFFs to flash memory has been there probably for two decades (so it's older than the buggy Arduino bootloader...).
As Arduino knows about their buggy bootloader (well, or should know), and uses its own commandline anyway, they could easily add that (new) option to their commandline then.

[dl8dtl]

We could use option -A for "Arduino" … :-)

[stefanrueger]

Cool; thanks for supporting this change. Would a new option -d (disable 0xff optimisation) be acceptable?

Using -A gives Arduino too much credit; this is more related to bootloaders (in general) being able to do a page erase and their desire to shun a device erase to save code bytes. Alas, -b and -B are taken. As an aside, one fun application of -d could be the new-found ability to upload to flash an input file that consists of as many 0xff as there are bytes below the bootloader (a slow ersatz for device erase).

[dl8dtl]

Using -A gives Arduino too much credit

Well, after all, it's the most widespread deployment of that bug … and I cannot see any mnemonic for -d here.

Yes, I still call it a bug. If someone is asked to "chip erase" (as this is the only available erase command in the chosen protocol), and they do – just nothing, I cannot call this anything but a bug. I see it as their responsibility to do the closest approximation of erasing the chip. The minimum for a bootloader is to erase the application flash area then (ideally, they'd also honor the EESAVE fuse, and depending on it, erase EEPROM).

I don't think there's a big difference in program size between "upon 'chip erase', loop over all application flash pages, and erase them" vs. "upon 'chip erase', do nothing, but when programming a page, first erase it".

In addition, a boot loader not acting on chip erase creates a huge security hole. Suppose you have a "secure application" inside, and taken care to set the lock fuses appropriately so nobdy can read it out. With that bootloader, one can come along, craft a minimal application that just covers the first flash page, and then toggles out the entire remaining flash contents through a GPIO pin. :-o This is probably not a big deal for opensource-like Arduino applications, but if anyone would have the idea to use that "well-proven bootloader" for their own purposes, it's getting hot. (That's also where the EEPROM erasing might get into the game. There's a reason Atmel has made erasing the EEPROM on "chip erase" the default.)

Don't get me wrong, I'm all for also providing an option to perform page erases instead of a chip erase (and for them, the 0xFF input must obviously be examined), but for the mentioned security reasons, this must never be the default. -d would be a good mnemonic for that. ;-) It doesn't only affect bootloaders (you already started a discussion for that) but could also be applied to modern AVRs and Atmel/Microchip tools supporting this feature. But adding that is going to be a bit of work.

[stefanrueger]

Another minimalist idea is to not use any option at all, but instead to automatically disable trailing-0xff optimisation in arduino.c inside arduino_initpgm(PROGRAMMER *pgm). This way every avrdude -c arduino ... just works (™ @mariusgreuel) no matter what the options are and without the need for Arduino folk to change anything beyond using the latest avrdude (as they should). I cannot see a disadvantage to this approach.

Of course, this could (and I think should) still be done in tandem with giving the user an explicit option to disable trailing-0xff optimisation independently: who knows which of the 112 programmers that .conf lists also talk to bootloaders or external programmers that do implicit page erase. I am agnostic as to the name of the option. You call the shots, @dl8dtl (though if you agree to the arduino.c modification, -A kind of loses its mnemonic appeal).

Yes, I agree, the very fact of using a bootloader increases the security attack surface. Going down that road mellows the strict Harvard architecture by providing functioning SPM opcodes. This allows the kind of attack you describe. (There is a neat 2008 paper by Francillon and Castelluccia, INRIA (free-access preprint here) with an example of code injection on an ATmega128 using a bootloader and, you will have guessed, some buggy software in addition.)

Note, though, the problem in your example is not that optiboot ignores avrdude's explicit chip erase command. The attacker could have instructed avrdude -D to not send the chip erase command for uploading the tiny application that taps out the secret, big application in Morse. In my view, it is the existence of the page erase SPM opcode in the bootloader that is exploited in your scenario. Similar risks come from, for example, ATxmega's external programming page erase command (or its atomic page erase and write command). In all fairness, the lock bits for ATxmega can be set to block external programming with a view to protect secret applications. I recommend the discerning secret-application architect better consider the implications of their bootloader, too, when they activate those lock bits.

[stefanrueger]

Here is a use case without a bootloader:

I program an ATxmega384D3 to display light effects based on the text of Chaucer's Canterbury Tales. In order to not always have to upload the full monty for testing the light effects I put the 380 kB text in the top of the 392 kB flash and rig up an external programmer (smr-0815x, since you asked, which communicates with the host through the STK500 protocol and uses PDI with the atomic erase-and-write-page for a paged write). Now I reprogram my test runs using

avrdude -Dqp atxmega384d3 -c stk500 -b 115200 -P /dev/ttyUSB0 -U flash:w:sketch-ending-in-ff.hex:i

in an attempt to keep the Canterbury Tales in top flash (-D) but find myself mildly surprised that only 2160 bytes of my (legitimate!) 2185-bytes application have been uploaded.

So, we have two use cases in which avrdude's belief that writing 0xff to flash is a NOP falls short of user expectations. One is brought about by -D and the other is brought about by optiboot.

Hence, a simple and unobtrusive solution to this is to piggy-back "disable trailing 0xff-optimisation" onto -D and make that also the default when the arduino programmer is invoked. I'll add the latter to this PR tonight.

Of course, other option names exist. Happy to oblige.

[stefanrueger]

Now, trailing 0xff sequences are no longer removed when either invoking -D or when using the -c arduino programmer.

$ avrdude -qp atmega328p -c arduino -b 115200 -P /dev/ttyUSB0 -U flash:w:/tmp/sketch-ending-in-ff.hex:i

avrdude: AVR device initialized and ready to accept instructions
avrdude: Device signature = 0x1e950f (probably m328p)
avrdude: NOTE: "flash" memory has been specified, an erase cycle will be performed
         To disable this feature, specify the -D option.
avrdude: erasing chip
avrdude: reading input file "/tmp/sketch-ending-in-ff.hex"
avrdude: writing flash (2185 bytes):
avrdude: 2185 bytes of flash written
avrdude: verifying flash memory against /tmp/sketch-ending-in-ff.hex:
avrdude: 2185 bytes of flash verified

avrdude done.  Thank you.

[mariusgreuel]

Besides the API I mentioned, looks good to me. I like the fact that you hardcoded the disableffopt for the Arduino programmer. One less trap to step in.

[mariusgreuel]

avr_mem_hiaddr(NULL) looks strange. How about creating a new API for this purpose? May I suggest something like
disable_trailing_ff_optimisation()? That way we could remove the comment.

[stefanrueger]

Not a prob. Can do so almost trivially by

#define disable_trailing_ff_optimisation() avr_mem_hiaddr(NULL)

in src/libavrdude.h in the line preceding the declaration of int avr_mem_hiaddr(AVRMEM * mem);

This (in my world) passes as a self-documenting API. OK?

[mariusgreuel]

As this PR claims to fix #918 : Let us not forget that the verify step should check the entire flash range, independently of programmers and options. If this is deferred to a separate PR, then #918 should stay open.

[dl8dtl]

which communicates with the host through the STK500 protocol and uses PDI with the atomic erase-and-write-page for a paged write

The STK500 protocol simply cannot do that. So, your testcase is invalid.

Just keep in mind, if you call it STK500, it must behave as if it were an STK500. If you'd do something similar on a real STK500, you'd end up in the same disaster: since you don't do a chip erase between your uploads (and there is no other erase command in the STK500 protocol), you'll end up in programming only those bits to 0 which are still at 1 - regardless of whether AVRDUDE sends all the 0xFFs or not.

As already told, I'm all for implementing page erases, but not by abusing existing protocols that have never been intended to handle that. It's not only bootloaders, it's also all modern AVRs with modern Atmel/Microchip tools that will benefit from that. If we do page erases then, we can safely ask the programmer to erase each page before programming it (and still have no need to send 0xFF pages ;-). Still, to avoid security surprises, I'd keep the default to chip erase before programming the flash, so page erases must be explicitly asked for (e.g. -d).

Having said this, I'm all for enabling that feature for the Arduino programmer – simply because I'm pretty sure they won't ever fix their bootloader anyway. I agree that -A would somewhat lose its meaning then, but we could nevertheless call that "Arduino mode" in the docs as an explanation, as the Arduino bootloader is one of the most widespread implementations that requires it, even though we just enable it then automatically if the programmer is called like that.

[dl8dtl]

Let us not forget that the verify step should check the entire flash range

I completely agree with that.

[stefanrueger]

Let us not forget that the verify step should check the entire flash range

I assume you do mean the entire input file, not the entire flash range?

Well, #918 is about the (actually, not particularly radical) idea of enabling AVRDUDE to send the full input file to the device under certain circumstances. This PR does that. And, when it does so, it also verifies the complete input file.

One of the 30-odd comments (to what I initially thought to be a relatively uncontroversial improvement) identified the additional orthogonal issue with trailing-0xff removal that the verification is only done on the actually programmed bytes.

However, I can deal with that, too. Intellectually, this is trivial: when reading the input file for verification do not remove trailing 0xff from it. The implementation is a bit more involved, owing to the need to pass down to fileio() the fact that a file read is for verification purposes. I suggest allocating an FIO_READ_FOR_VERIFY sibling to FIO_READ and FIO_WRITE and use this suitably to make that distinction. OK? An, in my view less elegant, alternative is to add a further argument to fileio().

[stefanrueger]

The STK500 protocol simply cannot do that. So, your testcase is invalid.

We seem to be disagreeing about the semantics of STK500's Cmnd_STK_PROG_PAGE command, which is described on page 11 of Atmel's 31 page Application Note on the STK500 protocol.

In my reading this command tries to write one page to be programmed to FLASH, not more, not less. So, my smr-0815x physical programmer carries out the semantics of that command faithfully on the target chip, exactly as Atmel's document says it does: it writes one page to flash.

What we disagree on is that I think this command is allowed to succeed, while you seem to be implying that the command must not succeed, if it happens to wish to flip a 0 to a 1.

Atmel's documentation does not appear to be so apodictic about that point!

Just in case you view my use of an ATxmega and the implementation with the physical PDI layer as a slight of hand, I could have based my thought experiment on the venerable ATmega328p: My physical STK500 programmer smr-0815p (note this is now the model's p version, not the x version) does the following upon the STK500 Cmnd_STK_PROG_PAGE command: it reads the target chip's full EEPROM and flash, modifies the to be written page in the copy, erases the target chip and writes all back.

Summarising, NAND memory can look like a normal memory (just that it's a bit more complicated, but not impossible, to change a 0 to a 1).

[dl8dtl]

We seem to be disagreeing about the semantics of STK500's Cmnd_STK_PROG_PAGE command

It's quite simple: get a real STK500, and try it.

(I can borrow you one if you want.)

[stefanrueger]

OK, now all done:

• Implemented the API disable_trailing_ff_removal() that does exactly what is says on the tin
• New option -A suppresses trailing-0xff removal from file and AVR read
• -D implies -A (chip erase not guaranteed, and who knows what the 109 programmers are up to)
• -c arduino also suppresses trailing-0xff removal in recognition of how optiboot actually works
• Verify now always verifies full input file against AVR: Note 2160 bytes written and 2185 verified in the following example

$ avrdude -qp atmega328p -c usbtiny -U flash:w:sketch-ending-in-ff.hex:i

avrdude: AVR device initialized and ready to accept instructions
avrdude: Device signature = 0x1e950f (probably m328p)
avrdude: NOTE: "flash" memory has been specified, an erase cycle will be performed
         To disable this feature, specify the -D option.
avrdude: erasing chip
avrdude: reading input file "sketch-ending-in-ff.hex"
avrdude: writing flash (2160 bytes):
avrdude: 2160 bytes of flash written
avrdude: verifying flash memory against sketch-ending-in-ff.hex:
avrdude: 2185 bytes of flash verified

avrdude done.  Thank you.

Future work: If/when discussion around SPM programmers concludes, one outcome might be that there is an option in .conf for programmers that triggers disable_trailing_ff_removal()

[stefanrueger]

It's quite simple: get a real STK500, and try it.

Hehe, there is a radical idea... I am more of the "Five months in the lab can save you two hours in the library" persuasion :)

[stefanrueger]

@mariusgreuel @dl8dtl
Any appetite to progress this PR? I think this does all that's needed to close Issue #918

[stefanrueger]

As a reminder: The merged PR adds the option -A to AVRDUDE to switch off trailing-0xff optimisation. Although that was mildly controversial at the time, it is an important feature. Another effect of the PR is that -c arduino now gets the full file to upload for optiboot and other bootloaders automatically, which removes a difficult-to-diagnose trap.