Use ACL to protect MBR and Bootloader against accidental FLASH overwrites by the user application#384
Use ACL to protect MBR and Bootloader against accidental FLASH overwrites by the user application#384ejtagle wants to merge 7 commits intoadafruit:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds flash memory protection for the Master Boot Record (MBR) and bootloader regions to prevent accidental overwrites by user applications. The protection uses hardware features (ACL or BPROT depending on the chip) and can be disabled via the SKIP_FLASH_PROT build flag.
Changes:
- Implements
bootloader_util_flash_protect()function with ACL and BPROT support for nRF52 chips - Adds protection setup in
bootloader_util_app_start()before jumping to user application - Adds SKIP_FLASH_PROT build option to both Makefile and CMakeLists.txt
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| lib/sdk11/components/libraries/bootloader_dfu/bootloader_util.c | Implements flash protection function and applies protection to MBR and bootloader regions before starting user application |
| Makefile | Adds SKIP_FLASH_PROT build flag to conditionally disable flash protection |
| CMakeLists.txt | Adds SKIP_FLASH_PROT CMake option for consistency with Makefile |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/sdk11/components/libraries/bootloader_dfu/bootloader_util.c
Outdated
Show resolved
Hide resolved
lib/sdk11/components/libraries/bootloader_dfu/bootloader_util.c
Outdated
Show resolved
Hide resolved
lib/sdk11/components/libraries/bootloader_dfu/bootloader_util.c
Outdated
Show resolved
Hide resolved
| #ifndef SKIP_FLASH_PROT | ||
| uint32_t bootloader_util_flash_protect(uint32_t address, uint32_t size, bool read_protect) | ||
| { | ||
| if ((size & (CODE_PAGE_SIZE - 1)) || (address > BOOTLOADER_SETTINGS_ADDRESS)) |
There was a problem hiding this comment.
The parameter validation logic has issues. First, it doesn't check if size is zero, which would cause incorrect behavior. Second, the address validation (address > BOOTLOADER_SETTINGS_ADDRESS) doesn't make sense for protecting the MBR at address 0. Consider validating that size > 0 and checking that address + size doesn't overflow or exceed valid flash memory bounds.
| if ((size & (CODE_PAGE_SIZE - 1)) || (address > BOOTLOADER_SETTINGS_ADDRESS)) | |
| uint32_t end_addr = address + size; | |
| if ((size == 0) || | |
| (size & (CODE_PAGE_SIZE - 1)) || | |
| (end_addr < address) || | |
| (address > BOOTLOADER_SETTINGS_ADDRESS)) |
lib/sdk11/components/libraries/bootloader_dfu/bootloader_util.c
Outdated
Show resolved
Hide resolved
lib/sdk11/components/libraries/bootloader_dfu/bootloader_util.c
Outdated
Show resolved
Hide resolved
- clean up code - add illustrated memory layout for quick ref
|
The SDK12+ also enables write protection of the MBR/Bootloader from the Bootloader itself (if you inspect the main() function of this SDK12+ based secure bootloader (https://github.com/kaidyth/nrf52_bootloader/blob/master/main.c , lines 216 to 221), the first 2 lines executed do that), but this means the bootloader itself cannot be updated from itself, and/or the MBR cannot be initialized the first time, so those lines do not make any sense to me at all (yeah, it would be great to protect the bootloader/softdevice from unintended corruption from the bootloader code itself, but once the write protection is enabled, according to the technical docs, it is impossible to disable) |
|
perfect, thank you @ejtagle . I did want to implement this but couldn't make it in time. I am glad that you could pull this off.
self-protection is always an wanted feature for bootloader. Bootloader update should only be done by bootloader itself. So I actually remove the SKIP option, as long as we tested and verified this works as expected.
yeah, we need more people testing
I agree, we don't need to protect softdevice. Bootloader actually consider SD as application.
we are all good. |
since we only enable ACL when launching application. We can keep the self-update feature right ? please make sure you test it on actual hardware. |
Yes, the ACL is only enabled when launching application, all the other update cases are handled inside the bootloader (so, before the ACL is setup), or using reboots (for softdevice/bootloader updates) (the ACL is cleared by a reboot), so everything remains exactly the same in the update process. The only thing that happens is that the user application is forbidden to corrupt the MBR/bootloader, something desirable because it makes the boards always recoverable So far, so good. 3 boards, several updates, no issues at all |
This pull request makes the bootloader protect the FLASH areas where the MBR and the Bootloader is stored against overwriting by the user application. This should address the following issue: #21
I am not absolutely sure if we want or not this feature... Basically, this makes impossible to write a user application that updates the bootloader, so, for some specific user scenarios, we actually don't want this feature.
But, for most of the others, i think this feature is desirable, as, let's be honest here, most users are just amateur coders and a simple error in their flash writing routine will make the device unrecoverable without using a JTAG programmer...
The other thing is that this protection is enabled by default, changing the "de facto" behaviour of the previous bootloader versions... so, I am not sure... I think enabling the protection is simply better for most of the cases
I have tested this very quickly, so, also, i would appreciate if anyone can test it a little bit more.
Why not protecting the softdevice ? ... I think protecting the bootloader would be enough, but, protecting the softdevice is also possible (I don't know if the softdevice itself writes to FLASH ... that is something that should be tested, but it is suspicious SDK12+ secure bootloaders do not protect the softdevice, so, I just followed what they did there
I am open to suggestions and changes, or even dropping the pull request. For my specific use case, enabling protection seems the most sensible thing to do, and also, on the newer SDK12+ secure bootloaders, this protection is always done ..