fw_update: check for existing candidate image before attempting download #743

hasheddan · 2025-02-03T14:19:45Z

Refactors fw_update logic to check whether the existing image in the candidate slot matches the desired update image in a received manifest. If so, we attempt to boot into the image without downloading it again.

Implementing involved some cleanup of the interface to fw_update platform ports. Namely, fw_update_post_download now returns a status, and fw_update_validate receives an image hash and size. The former is to allow for ports to return an error if they are unable to "finalize" a download. This is particularly relevant for esp_idf, as completing the download also implicitly performs image validation. The latter change is to allow for validation to be performed on a candidate image outside the context of the download of that image, which may have taken place prior to the most recent reboot.

Because ports can perform custom validation in fw_update_post_download and we implement hash validation in fw_update for downloaded images, the fw_update_validate implementation for a port is no longer invoked after download. Instead, it is now only invoked on receiving a new manifest to check whether the desired update image is already present. The primary reason for not also invoking fw_update_validate post download is that it is not implemented for all ports at this time, though it is also somewhat redundant with our existing hash check. There are two options that would allow for calling fw_update_validate post download and prior to download:

Provide implementations for all ports.
Allow for differentiating between "not supported" and "validation failed".

(1) feels like the right path long term, and given that we do not lose any functionality today by removing the post download call (i.e. ports previously just had no-op implementations), moving forward with the fw_update_post_download + fw_update's hash check seems like a reasonable course of action.

Work items that should follow this implementation:

We should update our OTA HIL testing to include downloading an image that will never connect to Golioth, then ensure that when we rollback and see the manifest again that we attempt to boot into it without download. The current tests effectively just validate that fw_update_validate always fails.
This implementation avoids downloading the same image over and over again, but we should also implement some sort of upgrade backoff to avoid booting into the new image over and over again. Continuously doing so results in flash wear from swapping the candidate image into the primary slot on upgrade and revert.

Updates fw_update_post_download to return a status and updates platform port implementations to match. This is in service of subsequently moving logic currently in fw_update_validate in some ports to fw_update_post_download. Signed-off-by: Daniel Mangum <[email protected]>

github-actions · 2025-02-03T14:20:39Z

Visit the preview URL for this PR (updated for commit e48d9d9):

https://golioth-firmware-sdk-doxygen-dev--pr743-fw-update-chec-de1cpdoh.web.app

_{(expires Mon, 17 Feb 2025 22:52:02 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: a9993e61697a3983f3479e468bcb0b616f9a0578}

github-actions · 2025-02-03T15:57:17Z

Code Coverage

Package	Line Rate	Branch Rate	Health
include.golioth	75%	50%	✔
port.linux	60%	31%	➖
port.utils	58%	46%	➖
port.zephyr	53%	22%	➖
src	67%	30%	➖
Summary	66% (2715 / 4120)	30% (1123 / 3738)	➖

codecov · 2025-02-03T16:27:19Z

Codecov Report

Attention: Patch coverage is 0% with 68 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
port/zephyr/golioth_fw_zephyr.c	0.00%	33 Missing ⚠️
src/fw_update.c	0.00%	31 Missing ⚠️
port/linux/fw_update_linux.c	0.00%	4 Missing ⚠️

Files with missing lines	Coverage Δ
port/linux/fw_update_linux.c	`0.00% <0.00%> (ø)`
src/fw_update.c	`0.00% <0.00%> (ø)`
port/zephyr/golioth_fw_zephyr.c	`0.00% <0.00%> (ø)`

... and 5 files with indirect coverage changes

hasheddan · 2025-02-05T18:48:08Z

port/zephyr/golioth_fw_zephyr.c

+/*
+ * @note This is similar to Zephyr's flash_img_check() but uses the Golioth
+ * port's sha256 API.
+ */


In addition to being consistent with our sha256 implementation, this is also necessary because building with flash_img_check enabled with mbedtls is currently broken on nordic platforms due to zephyrproject-rtos/zephyr#85238

szczys

Great work @hasheddan!

szczys · 2025-02-05T22:10:17Z

port/linux/fw_update_linux.c

 {
-    return GOLIOTH_OK;
+    // TODO(hasheddan): support validating candidate firmware.
+    return GOLIOTH_ERR_FAIL;


Suggested change

return GOLIOTH_ERR_FAIL;

return GOLIOTH_ERR_NOT_IMPLEMENTED;

szczys · 2025-02-05T22:10:29Z

port/modus_toolbox/fw_update_mcuboot.c

-    // Nothing to do
-    return GOLIOTH_OK;
+    // TODO(hasheddan): support validating candidate firmware.
+    return GOLIOTH_ERR_FAIL;


Suggested change

return GOLIOTH_ERR_FAIL;

return GOLIOTH_ERR_NOT_IMPLEMENTED;

Ah nice! I didn't realize we already had this status. In theory this would allow us to also call fw_update_validate post download and just ignore result if GOLIOTH_ERR_NOT_IMPLEMENTED. However, I think I'd lean towards not doing that given our existing hash check post download (as mentioned in the PR body). Regardless, should definitely be using the status here! 👍🏻

szczys · 2025-02-05T22:18:20Z

port/zephyr/golioth_fw_zephyr.c

+
+    golioth_sys_sha256_t hash_ctx = golioth_sys_sha256_create();
+
+    buf_size = sizeof(_flash_img_context.buf);


Note for other reviewers: This buf size defaults to CONFIG_IMG_BLOCK_BUF_SIZE which defaults to 512.

szczys · 2025-02-05T22:22:04Z

port/esp_idf/fw_update_esp_idf.c

+    {
+        return GOLIOTH_ERR_FAIL;
+    }
+    if (memcmp(hash, data.image_digest, sizeof(data.image_digest)) != 0)


Do we need to verify the length of hash is the same as the length of data.image_digest?

We don't know the length of hash here -- it is somewhat implicit in the port API contract as implemented, so it may be worth making it more explicit that a 32 byte sha256 hash is being passed (at the very least in the documentation on fw_update_validate). In practice, because data.image_digest is hard-coded to 32 bytes and we always pass a GOLIOTH_OTA_COMPONENT_BIN_HASH_LEN (32) then they should always be same length.

I am still in process of testing the esp-idf fw_update_validate implementation though, so there may need to be some further changes made here.

Ah, thanks for the background.

In practice, because data.image_digest is hard-coded to 32 bytes and we always pass a GOLIOTH_OTA_COMPONENT_BIN_HASH_LEN (32) then they should always be same length.

Would it make sense to change the function as below?

enum golioth_status fw_update_validate(const uint8_t hash[GOLIOTH_OTA_COMPONENT_BIN_HASH_LEN], const size_t size)

I typically shy away from using arrays as function parameters in C because in practice they decay to a pointers. However, they can give the impression that, for example, sizeof(hash) in a function implementation would return the size of the array, but in reality it would return the size of the pointer. Some folks certainly argue the other side of this, so I'm open to discussion 🙂

hasheddan · 2025-02-06T20:29:58Z

port/esp_idf/fw_update_esp_idf.c

+        .offset = _update_partition->address,
+        .size = _update_partition->size,
+    };
+    if (esp_image_verify(ESP_IMAGE_VERIFY, &part_pos, &data) != ESP_OK)


I'll be changing this in a moment because esp_image_verify will use the appended digest of the image, rather than the actual hash of the bytes in the partition, so we'll end up with a mismatch with the hash in the manifest. This is similar to what we are avoiding in the Zephyr port with mcuboot.

ref: https://github.com/espressif/esp-idf/blob/c5865270b50529cd32353f588d8a917d89f3dba4/components/bootloader_support/src/esp_image_format.c#L311

sam-golioth · 2025-02-10T19:46:43Z

Because ports can perform custom validation in fw_update_post_download and we implement hash validation in fw_update for downloaded images, the fw_update_validate implementation for a port is no longer invoked after download. Instead, it is now only invoked on receiving a new manifest to check whether the desired update image is already present. The primary reason for not also invoking fw_update_validate post download is that it is not implemented for all ports at this time, though it is also somewhat redundant with our existing hash check. There are two options that would allow for calling fw_update_validate post download and prior to download:

Provide implementations for all ports.

Allow for differentiating between "not supported" and "validation failed".

(1) feels like the right path long term, and given that we do not lose any functionality today by removing the post download call (i.e. ports previously just had no-op implementations), moving forward with the fw_update_post_download + fw_update's hash check seems like a reasonable course of action.

I think removing fw_update_validate() from the post-download flow (along with the change to fw_update_post_download()) is actually the right long term move. The only thing we do between invoking fw_update_post_download() and fw_update_validate() is verify the SHA hash (as you point out), and that shouldn't affect the port's ability to validate the image. Having two callbacks there is actually somewhat confusing, and doesn't add anything. In the future, if we implement patching or compression, it could make sense to have hooks before and after the post-download processing steps, but we shouldn't be trying to pre-design that.

Given the above, it could make sense to rename fw_update_validate() to make its new purpose clearer.

sam-golioth

Great work @hasheddan! Overall looks good, left a few comments

sam-golioth · 2025-02-10T19:53:16Z

src/fw_update.c

+        {
+            GLTH_LOGE(TAG, "Failed to perform post download operations");
+            fw_download_failed(GOLIOTH_OTA_REASON_FIRMWARE_UPDATE_FAILED);
+            golioth_sys_sha256_destroy(download_ctx.sha);


We're destroying this SHA in a number of places. I think we can move the call golioth_sys_sha256_finish() to just after the block download finishes (i.e. before the call to back_increment()), and destroy the SHA context there. Then we can just have the calculated SHA on the stack which will be easier to manage.

ack! was avoiding making that change in this PR, but would love to go ahead and do so -- I think it probably makes the most sense after the err check block (i.e. don't need to finish if we are going to destroy), but happy to always do finish + destroy after back_increment if you find it cleaner 👍🏻

After the err check works for me!

sam-golioth · 2025-02-10T19:55:33Z

src/fw_update.c

-
-        if (is_last)
-        {
-            fw_update_post_download();


This is a lot easier to follow 👍

sam-golioth · 2025-02-10T19:57:27Z

include/golioth/fw_update.h

 ///
 /// @return GOLIOTH_OK - image validated
 /// @return Otherwise - error in validation, abort firmware update
-enum golioth_status fw_update_validate(void);
+enum golioth_status fw_update_validate(const uint8_t *hash, const size_t size);


It's clear in the function comment, but when only looking at the function signature it would be easy to mistake size to mean the size of the array passed in hash. Also const isn't necessary for arguments passed by value.

Suggested change

enum golioth_status fw_update_validate(const uint8_t *hash, const size_t size);

enum golioth_status fw_update_validate(const uint8_t *hash, size_t img_size);

whoops copy pasta on that second const -- happy to update to img_size as well 👍🏻

sam-golioth · 2025-02-10T20:01:42Z

include/golioth/fw_update.h

 ///
 /// @return GOLIOTH_OK - image validated
 /// @return Otherwise - error in validation, abort firmware update
-enum golioth_status fw_update_validate(void);


This commit (and the next four) won't build without the following five commits. We should preserve bisectability by either squashing these commits or just updating the function signatures in this commit (without the functional changes).

Yeah, commit history needs to be adjusted prior to merge -- pushed small commits while iterating.

sam-golioth · 2025-02-10T20:07:13Z

src/fw_update.c

@@ -424,6 +453,18 @@ static void fw_update_thread(void *arg)
                continue;
            }

+            if (fw_update_validate(_component_ctx.target_component.hash,
+                                   _component_ctx.target_component.size)
+                == GOLIOTH_OK)


I'm sure this is what clang-format requires, but I'm not a fan haha. If you want to disable formatting for this line and replace it with something cleaner, I wouldn't be opposed.

sam-golioth · 2025-02-10T20:08:54Z

src/fw_update.c

@@ -604,14 +630,11 @@ static void fw_update_thread(void *arg)
        /* Download successful. Reset backoff */
        backoff_reset(&_component_ctx);

-        int countdown = 5;
-        while (countdown > 0)
+        if (fw_change_image_and_reboot() != GOLIOTH_OK)


This reports the DOWNLOADING state to the cloud and changes the boot image, so we don't need to do that in lines 616 - 628 anymore.

I believe we still need the DOWNLOADED report, but agree we can remove the UPDATING and boot image swap 👍🏻

Yep! I mean to write UPDATING - agreed we need to keep DOWNLOADED.

hasheddan · 2025-02-10T21:40:31Z

Given the above, it could make sense to rename fw_update_validate() to make its new purpose clearer.

@sam-golioth cool! At one point I had it as fw_update_check_candidate() -- any thoughts on that or alternatives?

sam-golioth · 2025-02-10T21:45:08Z

Given the above, it could make sense to rename fw_update_validate() to make its new purpose clearer.

@sam-golioth cool! At one point I had it as fw_update_check_candidate() -- any thoughts on that or alternatives?

Sounds good to me!

Moves the fw_update_post_download invocation out of the block download callback. This makes it more obvious when and if post download operations are invoked. Signed-off-by: Daniel Mangum <[email protected]>

Updates the esp_idf port by moving the OTA completion logic from validation into post download. While esp_idf OTA support does perform some validation when completing a download, the primary purpose is closing the OTA download context. This frees up fw_update_validate to be modified to allow for calling outside the context of a specific download. Signed-off-by: Daniel Mangum <[email protected]>

Update fw_update_validate to fw_update_check_candidate and pass an image hash and size such that image validateion is not coupled to any state maintained in ports. Ports should be able to validate an image in a candidate slot without needing to have downloaded since last reboot. Make a corresponding update in fw_update thread to check whether the requested firmware update image is already present in candidate slot. If so, download is skipped and we attempt to reboot into existing candidate slot image. Note that the call to fw_update_validate is now removed from the post download flow and not replaced in the same call site by fw_update_check_image. fw_update_validate was previously a no-op for most platform ports, and for those that did implement checks (esp_idf), the functionality was moved to fw_update_post_download, which is still called following an image download. Signed-off-by: Daniel Mangum <[email protected]>

Adds support for validating candidate firmware images by hash and size to the Zephyr firmware update port. Signed-off-by: Daniel Mangum <[email protected]>

Adds support for validating candidate firmware images by hash and size to the esp_idf firmware update port. Signed-off-by: Daniel Mangum <[email protected]>

hasheddan · 2025-02-10T22:54:33Z

src/fw_update.c

 static enum golioth_status fw_verify_component_hash(
-    struct download_progress_context *ctx,
+    const uint8_t calc_sha256[GOLIOTH_OTA_COMPONENT_BIN_HASH_LEN],
    const uint8_t server_sha256[GOLIOTH_OTA_COMPONENT_BIN_HASH_LEN])


As mentioned earlier in review, I don't really love using arrays in function signatures, but given that is the existing pattern here, I opted to remain consistent.

It's gives a false sense of compiler security, but from a readability standpoint I think it's the clearest way in C to communicate to the user that their input must be this exact size, no more and no less.

Fair enough. Given that this function is only visible to this compilation unit I am less worried about it here anyway 👍🏻

sam-golioth

🎉

hasheddan force-pushed the fw_update/check-pending-image branch from 1d7a337 to 0a23494 Compare February 3, 2025 14:27

hasheddan force-pushed the fw_update/check-pending-image branch from f297bfd to b352821 Compare February 5, 2025 18:45

hasheddan commented Feb 5, 2025

View reviewed changes

hasheddan force-pushed the fw_update/check-pending-image branch 2 times, most recently from 9c6508e to 1f0926e Compare February 5, 2025 18:58

hasheddan marked this pull request as ready for review February 5, 2025 19:29

hasheddan requested review from szczys, sam-golioth and mniestroj February 5, 2025 19:29

szczys reviewed Feb 5, 2025

View reviewed changes

hasheddan commented Feb 6, 2025

View reviewed changes

hasheddan force-pushed the fw_update/check-pending-image branch 2 times, most recently from c9f0a69 to bea64d7 Compare February 6, 2025 22:56

hasheddan requested a review from szczys February 7, 2025 13:28

szczys approved these changes Feb 7, 2025

View reviewed changes

sam-golioth reviewed Feb 10, 2025

View reviewed changes

hasheddan force-pushed the fw_update/check-pending-image branch 2 times, most recently from 88b73eb to 178808e Compare February 10, 2025 22:39

hasheddan added 5 commits February 10, 2025 17:51

fw_update: move fw_update_post_download out of callback

26a8181

Moves the fw_update_post_download invocation out of the block download callback. This makes it more obvious when and if post download operations are invoked. Signed-off-by: Daniel Mangum <[email protected]>

zephyr: fw_update: add support for validating candidate images

2f3c34d

Adds support for validating candidate firmware images by hash and size to the Zephyr firmware update port. Signed-off-by: Daniel Mangum <[email protected]>

esp_idf: fw_update: add support for validating candidate images

e48d9d9

Adds support for validating candidate firmware images by hash and size to the esp_idf firmware update port. Signed-off-by: Daniel Mangum <[email protected]>

hasheddan force-pushed the fw_update/check-pending-image branch from 178808e to e48d9d9 Compare February 10, 2025 22:51

hasheddan commented Feb 10, 2025

View reviewed changes

hasheddan requested a review from sam-golioth February 10, 2025 22:58

sam-golioth approved these changes Feb 11, 2025

View reviewed changes

hasheddan merged commit e97dae0 into main Feb 11, 2025
135 of 140 checks passed

hasheddan deleted the fw_update/check-pending-image branch February 11, 2025 20:25


		golioth_sys_sha256_t hash_ctx = golioth_sys_sha256_create();

		buf_size = sizeof(_flash_img_context.buf);

	enum golioth_status fw_update_validate(const uint8_t *hash, const size_t size);
	enum golioth_status fw_update_validate(const uint8_t *hash, size_t img_size);

fw_update: check for existing candidate image before attempting download #743

fw_update: check for existing candidate image before attempting download #743

Uh oh!

Conversation

hasheddan commented Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Uh oh!

codecov bot commented Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szczys left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-golioth commented Feb 10, 2025

Uh oh!

sam-golioth left a comment

Choose a reason for hiding this comment

Uh oh!

sam-golioth Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasheddan commented Feb 10, 2025

Uh oh!

sam-golioth commented Feb 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasheddan commented Feb 3, 2025 •

edited

Loading

github-actions bot commented Feb 3, 2025 •

edited

Loading

github-actions bot commented Feb 3, 2025 •

edited

Loading

codecov bot commented Feb 3, 2025 •

edited

Loading

sam-golioth Feb 10, 2025 •

edited

Loading