Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[STM32F4] Add bootloader overwrite self-protection #189

Merged

Conversation

KarlK90
Copy link
Contributor

@KarlK90 KarlK90 commented Jan 25, 2022

For review read the commits in separation

Driven by a remark that bootloader protection is still pending I implemented it for the STM32F4 family.

For broader use a new board level function is introduced board_flash_protect_bootloader(). All other ports are currently implemented as stubs.

On the STM32F4 family tinyuf2 occupies the first two flash sectors. The overwrite protection is enabled by setting the option bytes nWRP register in the flash peripheral. This is done when booting the board to always prevent tinyuf2 being overwritten. Also this is also only done for the bits that haven't been already processed to not wear out the memory.

@hathach I'm not sure if this logic should be included for all ports and moved to tinyuf2's main.c?

By default the overwrite protection is disabled by setting PROTECT_BOOTLOADER to 0.

Disabling the write protection again i.e. changing the nWRP to 1 again, can be done with a simple program called tinyuf2-unlocker that can be uploaded as an UF2 image or has do be done externally via DFU or SWD with STM32CubeProg or openocd.

@KarlK90 KarlK90 marked this pull request as draft January 26, 2022 08:18
@KarlK90 KarlK90 force-pushed the feature/add-stm32f4-bootloader-protection branch from c133e1d to 21ab90b Compare January 26, 2022 11:10
@KarlK90 KarlK90 marked this pull request as ready for review January 26, 2022 11:12
@KarlK90
Copy link
Contributor Author

KarlK90 commented Jan 26, 2022

Ready for review.

@KarlK90 KarlK90 force-pushed the feature/add-stm32f4-bootloader-protection branch 2 times, most recently from 767cb48 to 23a32aa Compare January 26, 2022 20:25
@KarlK90
Copy link
Contributor Author

KarlK90 commented Jan 26, 2022

I have written a small program that is apply-able as a UF2 image that will unprotected the flash sectors again and jump to the built-in DFU bootloader of the STM32F411. This acts as unlocker key to update or overwrite the whole flash.

The project is called tinyuf2-unlocker

@hathach
Copy link
Member

hathach commented Feb 1, 2022

Hi thank you for your PR. I am currently on TET (Lunar New Year) holiday. Will check this out later on

@KarlK90
Copy link
Contributor Author

KarlK90 commented Feb 1, 2022

Thanks, have a great holiday!

@KarlK90 KarlK90 force-pushed the feature/add-stm32f4-bootloader-protection branch 2 times, most recently from cd92c9c to 4058572 Compare February 8, 2022 11:10
@sigprof
Copy link

sigprof commented Feb 10, 2022

Hmm, is reserving the whole 64KB block for the bootloader intended? Some of these 16KB flash sectors could be useful for storing some firmware settings (unfortunately, on STM32F4 other flash sectors are much larger, therefore rewriting them at runtime is problematic). One of recent QMK PRs actually tried to add a board which uses tinyuf2, but assumes that only sectors 0 and 1 are occupied by the bootloader, and therefore uses sector 2 for EEPROM emulation. Setting the protection flags for sectors 0…3 irrespective of the actual bootloader size would break such usage.

@ladyada
Copy link
Member

ladyada commented Feb 10, 2022

for eeprom emulation they should use the last block in memory

@megamind4089
Copy link

@KarlK90 Thanks for this PR.
I thought, the linker uses only 32 KB of flash which is first 2 sectors and use the next 2 sectors for EEPROM emu.

https://github.com/adafruit/tinyuf2/blob/master/ports/stm32f4/linker/stm32f4.ld#L47

Am i missing something ?

@KarlK90 KarlK90 force-pushed the feature/add-stm32f4-bootloader-protection branch 2 times, most recently from 83d561a to 80dfdb9 Compare February 11, 2022 00:09
@KarlK90
Copy link
Contributor Author

KarlK90 commented Feb 11, 2022

@KarlK90 Thanks for this PR. I thought, the linker uses only 32 KB of flash which is first 2 sectors and use the next 2 sectors for EEPROM emu.

https://github.com/adafruit/tinyuf2/blob/master/ports/stm32f4/linker/stm32f4.ld#L47

Am i missing something ?

No you are completely right, I took the number from the QMK linker file which starts the usable flash sections at 64kb offset. I didn't cross reference it with the tinyuf2 linker script. Fixed the bootloader protection by only locking the first two sectors.

@KarlK90 KarlK90 force-pushed the feature/add-stm32f4-bootloader-protection branch from 80dfdb9 to f126649 Compare February 11, 2022 00:24
@KarlK90
Copy link
Contributor Author

KarlK90 commented Feb 11, 2022

Although the tinyuf2 bootloader assumes that the main app starts at a 64kb offset, so technically there is a 32kb gap...

https://github.com/adafruit/tinyuf2/blob/master/ports/stm32f4/boards.h#L38

@KarlK90 KarlK90 force-pushed the feature/add-stm32f4-bootloader-protection branch from f126649 to 544e302 Compare February 11, 2022 18:05
Tinyuf2 occupies the first two flash sectors in the whole stm32f4 family.
These are write protected by setting the option bytes nWRP in the flash
peripheral when booting the board. This is done to always prevent tinyuf2
from overwriting itself by accident. By default the write protection is
disabled by setting PROTECT_BOOTLOADER to 0.

Disabling the write protection again can be done via DFU or SWD with
STM32CubeProg or openocd.
@KarlK90 KarlK90 force-pushed the feature/add-stm32f4-bootloader-protection branch from 544e302 to 1633e6c Compare March 8, 2022 11:04
@KarlK90 KarlK90 changed the title Add bootloader overwrite self-protection for STM32F4 boards [STM32F4] Add bootloader overwrite self-protection Mar 28, 2022
@KarlK90
Copy link
Contributor Author

KarlK90 commented Jun 1, 2022

Friendly ping @hathach

@ladyada ladyada requested a review from hathach June 5, 2022 20:20
Copy link
Member

@hathach hathach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for late response, after TET, I kind of forget about this. This is a great PR, everything look great. And can be merged as it is, though I would like to include the tinyuf2-unlocker in this PR as well if possible.

The reason is for self-update via uf2, basically an special that contains bootloader binary in an array bytes and write that to the bootloader flash region. Currently it is not supported on stm32f4 port, but I think this is a good chance to also include it to

I have checked out your tinyuf2-unlocker but unfortunately I don't understand rust. Would you mind adding the unlock code, ideally it would be

void board_self_update(const uint8_t * bootloader_bin, uint32_t bootloader_len)
{
  (void) bootloader_bin;
  (void) bootloader_len;

  board_flash_protect_bootloader(false); // unlock

  // write booloader_bin to sector 0,1 since we are currently running as application (this can be its own PR later on).

  board_flash_protect_bootloader(true); // lock
}

@sigprof
Copy link

sigprof commented Jun 11, 2022

If you are considering to make board_flash_protect_bootloader(false) a generic API, note that it is actually impossible to implement for some MCUs. In fact, STM32F4xx almost looks like an outlier, because for many other STM32 chips (F103, F303, L4xx, L072, probably some more) the only way to make new option bytes take effect is to perform a system reset (or an “option byte reload” procedure, which itself does a system reset); at least that's what the official documentation says. So the implementation of board_self_update() suggested above won't work for those MCUs.

Implementing the self-update procedure on MCUs like that would still be possible, but would require that the bootloader does not reapply the flash protection when the firmware reboot is requested (this seems to be the case with the current code, because board_flash_init() is not called if check_dfu_mode() returns false). There would also be some potential for infinite loop if the disabled protection status does not stick for any reason.

@hathach
Copy link
Member

hathach commented Jun 15, 2022

@sigprof thank for your input, do you think we can use one of the DBL_TAP_MAGIC_PROTECTION_OFF as way to go around the infinite loop and continue to with the update once protection if off. It is definitely need some handshake between self-update app and bootloader.

However, I think we should stick with simplest way that work with F4 first, and generalize/refactor to other family later on. Although It may take more effort overall, it will ensure we don't write more code than we need.

Copy link
Member

@hathach hathach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the board_flash_protect_bootloader to take bool argument for protect/un-protect. I am trying to also get self-update to work with boot protection as well, however, I realize that self-update need quite a bit of additional changes and requires an cmsis f4 update to address the issue with vector table. I will merge this first and make an PR for self-update afterwards. Thank you very much for your effort and patience on this PR.

PS: esp32 build failed is already resolved in master, it would be ok to merge.

@hathach hathach merged commit cca7dde into adafruit:master Jun 22, 2022
@KarlK90
Copy link
Contributor Author

KarlK90 commented Jun 22, 2022

Sorry for late response, after TET, I kind of forget about this.

No problem, hope you had a good TET 🎉

I will merge this first and make an PR for self-update afterwards. Thank you very much for your effort and patience on this PR.

I was pretty busy in the last weeks that is why I didn't respond earlier. Thanks for the merge and taking over from here! Happy that I could provide a good starting point for the self-update feature.

@hathach
Copy link
Member

hathach commented Jun 22, 2022

@KarlK90 no problem at all, I fully understand the busy with other works well enough. Btw, I struggled a bit with old cmsis f4, since its system_stm32f4xx.c reset the vector table (SCB->VTOR) to default 0x08000000 (incorrect). Just update to latest cmsis, the later skip statement if not defined

https://github.com/STMicroelectronics/cmsis_device_f4/blob/2615e866fa48fe1ff1af9e31c348813f2b19e7ec/Source/Templates/system_stm32f4xx.c#L181

I am putting thing up together, self-update may not be related to protected bootloader, but we must get them running together before enabling it. Otherwise we will have a chance to lock the bootloader which could require user to run various code to unlock/flash and risk bricking the board.

This was referenced Jun 22, 2022
@myst729
Copy link

myst729 commented Sep 2, 2022

It's frustrating when I try to erase tinyuf2 after flashed it. No tool provided in the first place. Follow tinyuf2-unlocker guide but the build artifact doesn't work at all. Try to disable write protection and erase via STM32Cube, the blackpill drive pops again. Any ideas?

@myst729
Copy link

myst729 commented Sep 3, 2022

@hathach

default 0x08000000 (incorrect)

This is what I encounter now. Unfortunately I only have one CMSIS-DAP so I cannot update its firmware (which must be done from the host over another downloader). I found pyocd could erase listed sectors with --sector erase option:

pyocd erase -t stm32f401cdux  -s 0x08000000

Is this achievable, and which sector shall I pass?


pyocd flash -t stm32f401cdux dg_f401_default.bin                                                                                                   [21:29:55]
0001939:INFO:load_cmd:Loading /Users/leo/Downloads/dg_f401_default.bin
[                                                  ]   0%0007318:CRITICAL:__main__:flash erase sector failure (address 0x08000000; result code 0x1)
Traceback (most recent call last):
  File "/usr/local/lib/python3.9/site-packages/pyocd/__main__.py", line 150, in run
    status = cmd.invoke()
  File "/usr/local/lib/python3.9/site-packages/pyocd/subcommands/load_cmd.py", line 117, in invoke
    programmer.program(filename,
  File "/usr/local/lib/python3.9/site-packages/pyocd/flash/file_programmer.py", line 171, in program
    self._loader.commit()
  File "/usr/local/lib/python3.9/site-packages/pyocd/flash/loader.py", line 289, in commit
    perf = builder.program(chip_erase=chipErase,
  File "/usr/local/lib/python3.9/site-packages/pyocd/flash/builder.py", line 509, in program
    flash_operation = self._sector_erase_program_double_buffer(progress_cb)
  File "/usr/local/lib/python3.9/site-packages/pyocd/flash/builder.py", line 911, in _sector_erase_program_double_buffer
    self.flash.erase_sector(sector.addr)
  File "/usr/local/lib/python3.9/site-packages/pyocd/flash/flash.py", line 374, in erase_sector
    raise FlashEraseFailure('flash erase sector failure', address=address, result_code=result)
pyocd.core.exceptions.FlashEraseFailure: flash erase sector failure (address 0x08000000; result code 0x1)

@hathach
Copy link
Member

hathach commented Sep 8, 2022

@myst729 please open a new issue for your problem, this PR is merged and closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants