Handle parsing protected TLV region#114
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the MCUboot image inspection/parsing logic to handle images that contain both protected and regular TLV regions (common with Zephyr + TF-M signed images), and adds a regression test using a TF-M fixture.
Changes:
- Extend
ImageInfoparsing to read an optional protected TLV region before the regular TLV region. - Add
protected_tlv_info/protected_tlvstoImageInfoand include protected TLVs inget_tlv()lookups. - Add a test fixture-based regression test to validate protected TLV parsing.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/smpclient/mcuboot.py |
Adds protected TLV parsing and updates TLV lookup/printing to account for protected TLVs. |
tests/test_mcuboot_tools.py |
Adds a regression test for protected TLV parsing using a TF-M signed image fixture. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
JPHutchins
left a comment
JPHutchins
left a comment
There was a problem hiding this comment.
Thanks for this important update!
| tlv_header = ImageTLV.load_from(f) | ||
| tlvs.append(ImageTLVValue(header=tlv_header, value=f.read(tlv_header.len))) | ||
| # The mcuboot design doc says that optional protected TLV entries come before regular TLV entries | ||
| protected_tlvs = [] |
There was a problem hiding this comment.
Presumably missing typing.
| @staticmethod | ||
| def parse_tlvs(f: BytesIO, tlvs_end_offset: int) -> list[ImageTLVValue]: | ||
| """Parse TLVs from the current position in the file up to the given byte offset.""" | ||
| tlvs: list[ImageTLVValue] = [] | ||
| while f.tell() < tlvs_end_offset: | ||
| tlv_header = ImageTLV.load_from(f) | ||
| tlvs.append(ImageTLVValue(header=tlv_header, value=f.read(tlv_header.len))) | ||
|
|
||
| return tlvs |
There was a problem hiding this comment.
I think that I agree with abstracting this to reduce repetition, but I don't love that it has the side effect of mutating the file read position. Consider inlining again (even though it's repetition) or documenting the side effect.
Maybe some cleverness can be done with an iterator or generator, probably not worth the effort.
There was a problem hiding this comment.
I agree, I reworked it to take in a bytes object and just operate off of that.
JPHutchins
left a comment
JPHutchins
left a comment
There was a problem hiding this comment.
Thanks! LMK when you're ready to merge and I'll do so.
I think that linting must be disabled on this file, so instead of iterating, or asking you to rebase, I'm going to swing back around with a rework, confidence given by the integration test that was added.
|
@JPHutchins Feel free to take it whenever convenient, though I am willing to continue trying to satiate the Microsoft AI beast if you'd like 😛 |
Zephyr images built with TF-M get an MCUboot trailer generated with both protected and regular TLV entries. This PR modifies the MCUboot image parsing to properly recognize and parse these entries.