Nordic Flash protect

[FoamyGuy]

Adds mechanisms for preventing writing to out of bounds flash pages that we don't own.

• CIRCUITPY_NRF_FLASH_PROTECT flag off by default, when enabled uses the nrfs ACL to block writes to regions not owned by CircuitPython
• flash_page_is_ours() enforcer function for NVM to block writes for out of bounds regions.
• blocks_in_range() bounds check in internal_flash

These came up while working on the SP-1 because it's flash layout has a settings page for the custom bootloader that sits at the end after NVM and CIRCUITPY. SWD is not easy to access on the device and there isn't a published copy of the bootloader firmware to recover from if things go sideways due to an unintended write to the settings.

I tested this build successfully with CIRCUITPY_NRF_FLASH_PROTECT=1 on a CircuitPlayground bluefruit

[dhalbert]

I don't understand why you need this mechanism. When are writes being done outside the flash areas that we are allowed to write in? We can write in the filesystem area (if it's an internal filesystem), the NVM area, and the BLE_CONFIG area.

You can reserve areas in flash by augmenting ports/nordic/boards/common.template.ld.

Also, if you use the ACL mechanism, would that not also block the custom bootloader from writing to that area of flash?

[FoamyGuy]

I do not know of a specific case where out of bounds writes were occurring. I added it as a precaution due to the SP-1 bootloader settings page falling directly after the filesystem region, and it did not seem to me to have existing bounds check.

I will look into ports/nordic/boards/common.template.ld, thank you. I hadn't seen this before perhaps it can be used to add the same protection, I will try.

Also, if you use the ACL mechanism, would that not also block the custom bootloader from writing to that area of flash?

My understanding is that the protection lasts until reset. So this would prevent the bootloader from writing to it's settings after CircuitPython has started running. But from a cold boot, the bootloader goes first and can make any writes that it wants before launching CircuitPython. I don't believe that the bootloader is trying to make any writes after launching CircuitPython, though everything I have learned about it is gleaned from it's behavior only. There is no code published for it.

[FoamyGuy]

After looking in to ports/nordic/boards/common.template.ld here is my understanding: common.template.ld reverse flash at link time only. The protections added by the PR are for runtime writes. The reservation from common.template.ld is in place for the SP-1, but doesn't protect it from the following situations which these changes cover:

• supervisor_flash_write_blocks() with an out-of-range LBA. Important because filesystem falls directly before bootloader settings. Off by one lands in the settings page.
• nrf_nvm_safe_flash_page_write() from microcontroller.nvm with a bad page address.
• A stray pointer write, or another bus master, while NVMC is in write-enable.

[dhalbert]

I understand this is a "belt and suspenders" protection against stray writes. But I think the LLM's thinking on this has misplaced priorities. The cases you listed would all be due to code errors. All the other things like LBA should already be range-checked. But I still think you should reserve the area by adding a region in common.template.ld. Then it would not be possible to accidentally overflow, say, the NVM region into the bootloader settings region. There would be bounds-checking done at compile time as is done with the other regions now.

Maybe the smashing of the bootloader region you saw was due to the size of this bootloader settings region not being taken into account by the size calculations of the other regions.

The CircuitPython firmware area would also be in danger of these supposed stray writes, but I have never seen it being smashed while running, except maybe when we had errors in the initial flash map setup. And this would be true on non-nordic boards as well.

By the way, the flash_page_is_ours() routine does not take the BLE_CONFIG area into account.

If you really want to set ACL regions, I think you could do that more at compile time. The ACL routine would use a small table of bottom and top boundaries for each region to protect and would just set the ACL to read-only on those regions. I think you could dispense with a lot of other run-time code here, like flash_page_is ours().

[FoamyGuy]

Okay, thank you. Will close this and use a reservation in common.template.ld