Skip to content

[mcumgr] Increment data_off after reading the SHA #98347

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 3, 2025
Merged

[mcumgr] Increment data_off after reading the SHA #98347

merged 1 commit into from
Nov 3, 2025

Conversation

@skalldri
Copy link
Contributor

@skalldri skalldri commented Oct 27, 2025 •
edited
Loading

This PR fixes a bug in img_mgmt_read_info() which causes it to read invalid TLV keys after it locates the TLV containing the image's SHA.

The bug occurs because, after reading the SHA with img_mgmt_read(image_slot, data_off, hash, IMAGE_SHA_LEN);, data_off is not incremented to account for the IMAGE_SHA_LEN.

Instead, it loops back around and begins interpreting the SHA data as additional TLVs.

The SHA is normally 32 bytes long, and a struct image_tlv is 4 bytes, so it normally reads an additional 8 garbage TLVs.

If the SHA contains the bytes 0xff or IMAGE_TLV_SHA in a position that aligns with a tlv.it_type, then the entire img_mgmt_read_info() function will fail due to checks in the loop.

The fix for this issue is to increment the data_off appropriately after reading the SHA data.

Fixes #98747

@github-actions
Copy link

github-actions bot commented Oct 27, 2025

Hello @skalldri, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

@zephyrbot zephyrbot added area: mcumgr size: XS A PR changing only a single line of code labels Oct 27, 2025
@skalldri
Copy link
Contributor Author

skalldri commented Oct 27, 2025 •
edited
Loading

Background:

I discovered this issue while developing a "simulated flash" driver for the Nordic NRF5340.

On the Nordic NRF5340, the mcuboot_primary_1 partition is backed not by real flash memory, but instead by a flash-simulator driver. Firmware updates are written to this flash-simulator RAM partition by MCUboot via the flash-simulator driver, and then the updates are picked up by the network-core bootloader for installation.

However, this doesn't allow MCUMgr to read information about the network core image. To support this, I've been developing my own simulated flash driver that detects reads to the MCUboot header / TLV regions and returns simulated headers and TLVs, rather than reading junk data from RAM.

This driver is very strict about what addresses it will allow reads to, and it detected invalid reads (ex: attempting to read the SHA data as a TLV) from the MCUMgr subsystem. I was then quickly able to discover this issue.

@skalldri skalldri force-pushed the fix-img-mgmt-read-info branch 2 times, most recently from 2b1a970 to 0581382 Compare October 27, 2025 18:25
@skalldri
mgmt: mcumgr: grp: img_mgmt: increment data_off after reading the SHA
bada5d1 
Fix a bug where the image's SHA would be interpreted as TLV headers due
to missing a data_off increment.

Signed-off-by: Stuart Alldritt <[email protected]>
@skalldri skalldri force-pushed the fix-img-mgmt-read-info branch from 0581382 to bada5d1 Compare October 27, 2025 22:21
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 27, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@nordicjm nordicjm added this to the v4.3.0 milestone Oct 28, 2025
ljd42
ljd42 requested changes Nov 1, 2025
View reviewed changes
Copy link
Contributor

@ljd42 ljd42 left a comment

Choose a reason for hiding this comment

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

From my understanding, the offset computation should only happen when hash!=NULL. Or am I missing something?

@cfriedt
Copy link
Member

cfriedt commented Nov 2, 2025

@skalldri - can you please create an issue for this bug fix?

@skalldri
Copy link
Contributor Author

skalldri commented Nov 2, 2025 •
edited
Loading

@cfriedt done! I created #98747 for this issue. I don't see how to associate this PR with the Issue.... is that something you're able to do?

EDIT: Never mind I figured it out!

@jhedberg jhedberg merged commit 8af3795 into zephyrproject-rtos:main Nov 3, 2025
28 checks passed
@github-actions
Copy link

github-actions bot commented Nov 3, 2025

Hi @skalldri!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nordicjm nordicjm nordicjm approved these changes

@ljd42 ljd42 ljd42 approved these changes

@de-nordic de-nordic Awaiting requested review from de-nordic

Assignees

@nordicjm nordicjm

Labels

area: mcumgr size: XS A PR changing only a single line of code

Projects

None yet

Milestone

v4.3.0

Development

Successfully merging this pull request may close these issues.

mcumgr: img_mgmt_read_info() incorrectly interprets the image hash as TLV headers

6 participants

@skalldri @cfriedt @nordicjm @ljd42 @jhedberg @zephyrbot