sys/riotboot: add digest to header #17938

fjmolinas · 2022-04-14T06:27:08Z

Contribution description

This PR adds the firmware digest to the riotboot_hdr. If riotboot_slot_verify_sha256 is included then riotboot_slot_validate will also verify the digest.

This increases the riotboot header size by 36 bytes, but this does not change firmware size since the header is already 256 aligned.

Enabling this feature will prevent booting into a corrupted firmware, and can be also used at runtime to check the state of the running firmware (something useful for example for firmware deployed in space, e.g. ThingSat)

This PR is somewhat of an API change, since it changes the location of the checksum. I could also change the location of the extra field to be after the checksum, and modify riotboot_hdr_checksum accordingly.

Testing procedure

tests/riotboot passes with or without, riotboot_slot_verify_slot

sys/include/riotboot/hdr.h

fjmolinas · 2022-04-22T07:45:25Z

@benpicco @kaspar030 I split some of the commits but did as suggested keeping a chcksum_legacy field at the same position, but is there a chance where the compiler could re-align the struct members?

benpicco · 2022-04-22T07:58:20Z

is there a chance where the compiler could re-align the struct members?

No. There are also no 'gaps' between members, so no arch dependent padding will be inserted - you could use __attribute__((packed)) to ensure this, but the result will be the same.

kaspar030 · 2022-04-22T12:26:47Z

LGTM.

fjmolinas · 2022-04-22T16:40:13Z

The failure is unrelated, but will wait for ACK before re-building @kaspar030 @benpicco

benpicco · 2022-04-25T19:28:37Z

I don't have much time to test this currently, but did you try updating a board with an old riotboot and firmware with an image that uses the new format?

fjmolinas · 2022-04-26T06:31:03Z

Flash current riotboot master:

$ git log -n 1
commit c56e28a7e6e5aa4c9360a23b0f71f0248ca33171 (HEAD, upstream/master)
Merge: 65c15f7276 684cabff71
Author: Francisco <femolina@uc.cl>
Date:   Mon Apr 25 08:46:00 2022 +0200

    Merge pull request #17992 from aabadie/pr/sys/cpp_kconfig

    sys/cpp11-compat: add kconfig support and add Kconfig for cpp11 test applications
$  RIOT_CI_BUILD=1 BOARD=dwm1001 make -C bootloaders/riotboot flash -j
Building application "riotboot" for "dwm1001" with MCU "nrf52".

   text    data     bss     dec     hex filename
   1680       0     616    2296     8f8 /home/francisco/workspace/RIOT/bootloaders/riotboot/bin/dwm1001/riotboot.elf
/home/francisco/workspace/RIOT/dist/tools/jlink/jlink.sh flash /home/francisco/workspace/RIOT/bootloaders/riotboot/bin/dwm1001/riotboot.bin

Flash new applications (new headers), both slots boot correctly.

$git log -n 1
commit 72258f91bab2fe41d368b1710d4589bc4a4d3c2e (HEAD -> pr_riotboot_digest, origin/pr_riotboot_digest)
Author: Francisco Molina <femolina@uc.cl>
Date:   Fri Apr 22 09:32:15 2022 +0200

    tests/riotboot_hdr: update test
$BOARD=dwm1001 make -C tests/riotboot riotboot/flash-slot0
$BOARD=dwm1001 make -C tests/riotboot riotboot/flash-slot1

2022-04-26 08:24:57,036 # main(): This is RIOT! (Version: 2022.07-devel-17-g72258-pr_riotboot_digest)
2022-04-26 08:24:57,037 # Hello riotboot!
2022-04-26 08:24:57,041 # You are running RIOT on a(n) dwm1001 board.
2022-04-26 08:24:57,044 # This board features a(n) nrf52 MCU.
2022-04-26 08:24:57,047 # riotboot_test: running from slot 0
2022-04-26 08:24:57,050 # Image magic_number: 0x544f4952
2022-04-26 08:24:57,052 # Image Version: 0x62679028
2022-04-26 08:24:57,055 # Image start address: 0x00002400
2022-04-26 08:24:57,057 # Image size: 0x000032b4
2022-04-26 08:24:57,059 # Header chksum: 0x233a958c
2022-04-26 08:24:57,066 # Header digest: c24d17aefa0c10eafd3c7f4a489507d432fecc4b37b9b107c0a0eb613739a4c2
2022-04-26 08:24:57,066 #
> 2022-04-26 08:25:51,813 # main(): This is RIOT! (Version: 2022.07-devel-17-g72258-pr_riotboot_digest)
2022-04-26 08:25:51,814 # Hello riotboot!
2022-04-26 08:25:51,818 # You are running RIOT on a(n) dwm1001 board.
2022-04-26 08:25:51,821 # This board features a(n) nrf52 MCU.
2022-04-26 08:25:51,824 # riotboot_test: running from slot 1
2022-04-26 08:25:51,827 # Image magic_number: 0x544f4952
2022-04-26 08:25:51,829 # Image Version: 0x6267906b
2022-04-26 08:25:51,832 # Image start address: 0x00041400
2022-04-26 08:25:51,834 # Image size: 0x000032b4
2022-04-26 08:25:51,836 # Header chksum: 0x993ee409
2022-04-26 08:25:51,844 # Header digest: becbfb05393f285f816f4026cb6c987a850f61e6b9051610cc8da17ebf84dcef

Flash new riotboot, application still boots:

$BOARD=dwm1001 make -C tests/riotboot riotboot/flash-bootloader
2022-04-26 08:26:57,536 # Hello riotboot!
2022-04-26 08:26:57,539 # You are running RIOT on a(n) dwm1001 board.
2022-04-26 08:26:57,542 # This board features a(n) nrf52 MCU.
2022-04-26 08:26:57,545 # riotboot_test: running from slot 1
2022-04-26 08:26:57,548 # Image magic_number: 0x544f4952
2022-04-26 08:26:57,550 # Image Version: 0x6267906b
2022-04-26 08:26:57,553 # Image start address: 0x00041400
2022-04-26 08:26:57,555 # Image size: 0x000032b4
2022-04-26 08:26:57,557 # Header chksum: 0x993ee409
2022-04-26 08:26:57,565 # Header digest: becbfb05393f285f816f4026cb6c987a850f61e6b9051610cc8da17ebf84dcef
2022-04-26 08:26:57,565 #
>

Flash application with old headers, invalidates the slot (switches back to slot0), flashing the other slot results in no application booting, as epected.

$ BOARD=dwm1001 make -C tests/riotboot riotboot/flash-slot1
> 2022-04-26 08:28:04,165 # main(): This is RIOT! (Version: 2022.07-devel-17-g72258-pr_riotboot_digest)
2022-04-26 08:28:04,166 # Hello riotboot!
2022-04-26 08:28:04,170 # You are running RIOT on a(n) dwm1001 board.
2022-04-26 08:28:04,174 # This board features a(n) nrf52 MCU.
2022-04-26 08:28:04,176 # riotboot_test: running from slot 0
2022-04-26 08:28:04,179 # Image magic_number: 0x544f4952
2022-04-26 08:28:04,181 # Image Version: 0x62679028
2022-04-26 08:28:04,184 # Image start address: 0x00002400
2022-04-26 08:28:04,186 # Image size: 0x000032b4
2022-04-26 08:28:04,189 # Header chksum: 0x233a958c
2022-04-26 08:28:04,196 # Header digest: c24d17aefa0c10eafd3c7f4a489507d432fecc4b37b9b107c0a0eb613739a4c2
2022-04-26 08:28:04,196 #
$ BOARD=dwm1001 make -C tests/riotboot riotboot/flash-slot0
# nothing

Fash old bootloader again:

$ BOARD=dwm1001 make -C tests/riotboot riotboot/flash-bootloader
> 2022-04-26 08:29:55,513 # main(): This is RIOT! (Version: 2022.07-devel-185-gc56e2-HEAD)
2022-04-26 08:29:55,514 # Hello riotboot!
2022-04-26 08:29:55,518 # You are running RIOT on a(n) dwm1001 board.
2022-04-26 08:29:55,521 # This board features a(n) nrf52 MCU.
2022-04-26 08:29:55,524 # riotboot_test: running from slot 0
2022-04-26 08:29:55,527 # Image magic_number: 0x544f4952
2022-04-26 08:29:55,529 # Image Version: 0x62679134
2022-04-26 08:29:55,532 # Image start address: 0x00002400
2022-04-26 08:29:55,534 # Header chksum: 0x1183b53d
2022-04-26 08:29:55,534 #

fjmolinas

Spotted a couple of nitpicks, I'll address when the CI is a bit more idle.

makefiles/pseudomodules.inc.mk

sys/include/riotboot/hdr.h

This increases the riotboot header size by 36 bytes, but this does not change firmware size since the header is already 256 aligned. An additional 4bytes are also added to keep a checksum_legacy field at the same position as the previous checksum to be compatible with previous riotboot_hdr versions.

benpicco · 2022-04-30T13:38:27Z

sys/include/riotboot/hdr.h

@@ -52,6 +57,9 @@ typedef struct {
    uint32_t magic_number;      /**< Header magic number (always "RIOT")              */
    uint32_t version;           /**< Integer representing the partition version       */
    uint32_t start_addr;        /**< Address after the allocated space for the header */
+    uint32_t chksum_legacy;     /**< Checksum of riotboot_hdr digest excluded         */
+    uint32_t img_size;          /**< Firmware size                                    */


I was thinking about how we can avoid the chksum_legacy situation in the future, how about

Suggested change

uint32_t img_size; /**< Firmware size */

uint32_t img_size; /**< Firmware size */

uint16_t header_len; /**< Size of the riotboot header, excluding the checksum */

uint16_t flags;

What's the idea behind flags?

There will be two padding bytes anyway, might as well put them to some use

I'd use packed structs or call it reserved or padding with a comment why it's there. Or do we already have flags?

If there is not use yet, I think reserver makes sense, changing the name later would be free. The idea would be to use the header_len field to known what to checksum?

The idea would be to use the header_len field to known what to checksum?

Yes that's the idea

Something to consider though if the hdr_len field is modified, it could eventually calculate the checksum upon random memory regions

The same goes for img_size. The implementation checking the image has to check if bounds aren't exceeded.

benpicco

Please squash!

fjmolinas · 2022-05-24T19:00:21Z

Please squash!

I'm not squashing yet because #17938 (comment) makes me think that riotboot_slot_verify_sha256 or the caller (at least in the bootloader) should check that this is not out of bounds, same for the haeder. I'm dealing with a paper deadline so I can't take a look until next week, sorry for the delay.

fjmolinas · 2022-05-31T14:59:27Z

I added a check on the image area to verify, but I have second thoughts regarding this header_len, it seems weird to verify the checksum of contents the bootloader is not aware of, but more critically of a size, the bootloader is not aware of. Based on this I would rather define the header size, right now it's 60 bytes, and for cortex-m, it's at least 256Bytes for alignment reasons. So could it be better to instead have:

#ifndef RIOTBOOT_HEADER_MAX_LEN
#define RIOTBOOT_HEADER_MAX_LEN 256
#endif

typedef struct {
    uint32_t magic_number;      /**< Header magic number (always "RIOT")              */
    uint32_t version;           /**< Integer representing the partition version       */
    uint32_t start_addr;        /**< Address after the allocated space for the header */
    uint32_t chksum_legacy;     /**< Checksum of riotboot_hdr digest excluded         */
    uint32_t img_size;          /**< Firmware size                                    */
    uint8_t digest[SHA256_DIGEST_LENGTH]; /**< sh256 of image                         */
    uint8_t reserved[RIOTBOOT_HEADER_MAX_LEN - 56];
    uint32_t chksum;            /**< Checksum of riotboot_hdr                         */
} riotboot_hdr_t;

benpicco · 2022-05-31T15:03:32Z

Since riotboot_hdr_t is sometimes put on the stack, I would be against it always requiring 256 bytes.

The bootloader currently does not verify digest but checksum it anyway. With the header length field we can avoid breaking changes if we want to use different hash algorithms or add other fields in the future.

fjmolinas · 2022-06-01T06:08:04Z

Since riotboot_hdr_t is sometimes put on the stack, I would be against it always requiring 256 bytes.

Hmm true

The bootloader currently does not verify digest but checksum it anyway. With the header length field we can avoid breaking changes if we want to use different hash algorithms or add other fields in the future.

Do you think we need to worry about wrong header length and eventually checksumming invalid regions? This is my main concern, read to non accessible regions.

benpicco · 2022-06-01T09:45:33Z

Hm would it help to limit the header length to 256 bytes / uint8?

fjmolinas requested a review from kaspar030 April 14, 2022 06:27

fjmolinas requested review from smlng, leandrolanzieri, aabadie and MichelRottleuthner as code owners April 14, 2022 06:27

github-actions bot added Area: build system Area: Build system Area: sys Area: System Area: tests Area: tests and testing framework Area: tools Area: Supplementary tools labels Apr 14, 2022

fjmolinas force-pushed the pr_riotboot_digest branch from bb8362a to 87963cb Compare April 14, 2022 06:30

fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 14, 2022

benpicco reviewed Apr 17, 2022

View reviewed changes

sys/include/riotboot/hdr.h Show resolved Hide resolved

fjmolinas force-pushed the pr_riotboot_digest branch 2 times, most recently from 3623df9 to 2fed5fa Compare April 22, 2022 07:42

fjmolinas force-pushed the pr_riotboot_digest branch from 2fed5fa to 7fb264f Compare April 22, 2022 07:46

fjmolinas force-pushed the pr_riotboot_digest branch from 7fb264f to 3a93045 Compare April 22, 2022 11:49

sys/riotboot: split verify_sha256 from flashwrite

297467e

fjmolinas force-pushed the pr_riotboot_digest branch from 3a93045 to 3da7e0c Compare April 25, 2022 06:37

fjmolinas added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 25, 2022

fjmolinas force-pushed the pr_riotboot_digest branch from 3da7e0c to 72258f9 Compare April 25, 2022 14:36

fjmolinas added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Apr 26, 2022

benpicco approved these changes Apr 26, 2022

View reviewed changes

fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 26, 2022

fjmolinas commented Apr 27, 2022

View reviewed changes

fjmolinas added 3 commits April 29, 2022 16:42

tests/riotboot: include riotboot_slot_verify_sha256

b9ea4d5

tests/riotboot_hdr: update test

f47ce92

fjmolinas force-pushed the pr_riotboot_digest branch from 388c338 to f47ce92 Compare April 29, 2022 14:42

benpicco reviewed Apr 30, 2022

View reviewed changes

fjmolinas added 2 commits May 18, 2022 09:11

fixup! sys/riotboot: add digest to header

6be210a

fixup! tests/riotboot_hdr: update test

4e9dad6

benpicco approved these changes May 23, 2022

View reviewed changes

fjmolinas added 2 commits May 31, 2022 16:45

cpu/cortexm_common: export ROM_LEN as CFLAG

630dbe2

sys/riotboot/verify_sha256: check that region to verify is in ROM

d586e5b

fjmolinas requested a review from bergzand as a code owner May 31, 2022 14:48

github-actions bot added Area: cpu Area: CPU/MCU ports Platform: ARM Platform: This PR/issue effects ARM-based platforms labels May 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sys/riotboot: add digest to header #17938

sys/riotboot: add digest to header #17938

fjmolinas commented Apr 14, 2022

fjmolinas commented Apr 22, 2022

benpicco commented Apr 22, 2022

kaspar030 commented Apr 22, 2022

fjmolinas commented Apr 22, 2022

benpicco commented Apr 25, 2022

fjmolinas commented Apr 26, 2022

fjmolinas left a comment

benpicco Apr 30, 2022

jue89 May 5, 2022

benpicco May 5, 2022

jue89 May 5, 2022

fjmolinas May 17, 2022

benpicco May 17, 2022

fjmolinas May 18, 2022

jue89 May 18, 2022

benpicco left a comment

fjmolinas commented May 24, 2022

fjmolinas commented May 31, 2022

benpicco commented May 31, 2022 •

edited

Loading

fjmolinas commented Jun 1, 2022

benpicco commented Jun 1, 2022 •

edited

Loading

sys/riotboot: add digest to header #17938

Are you sure you want to change the base?

sys/riotboot: add digest to header #17938

Conversation

fjmolinas commented Apr 14, 2022

Contribution description

Testing procedure

fjmolinas commented Apr 22, 2022

benpicco commented Apr 22, 2022

kaspar030 commented Apr 22, 2022

fjmolinas commented Apr 22, 2022

benpicco commented Apr 25, 2022

fjmolinas commented Apr 26, 2022

fjmolinas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benpicco left a comment

Choose a reason for hiding this comment

fjmolinas commented May 24, 2022

fjmolinas commented May 31, 2022

benpicco commented May 31, 2022 • edited Loading

fjmolinas commented Jun 1, 2022

benpicco commented Jun 1, 2022 • edited Loading

benpicco commented May 31, 2022 •

edited

Loading

benpicco commented Jun 1, 2022 •

edited

Loading