Skip to content
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.

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

imgtool: dumpinfo: Fixed support for the custom alignment #1793

Closed
wants to merge 1 commit into from

Conversation

mingulov
Copy link
Contributor

@mingulov mingulov commented Sep 4, 2023

Fixed support for the custom alignment (in case if BOOT_MAGIC_2 is provided). Current version fails to parse it with an error:

ValueError: invalid literal for int() with base 0: b'...

Using the same solution as 'swap_size' parsing as a fix.

Fixed support for the custom alignment (in case if BOOT_MAGIC_2
is provided). Current version fails to parse it with an error:

ValueError: invalid literal for int() with base 0: b'...

Using the same solution as 'swap_size' parsing as a fix.

Signed-off-by: Denis Mingulov <denis@mingulov.com>
Copy link
Member

@utzig utzig left a comment

Choose a reason for hiding this comment

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

The proposal solution doesn't seem correct to me. For swap_size it was OK because it is always written in little endian, but max align is written according to the given endiannes, as can be seen here: https://github.com/mcu-tools/mcuboot/blob/main/scripts/imgtool/image.py#L179

int(trailer_magic[:2], 0) is obviously wrong because it is converting a string representing a number which is not what we have here.

One simple way to do it, is to convert in both little and big endian, maybe just get a min of those, and check the value against the list of valid max align values, eg:

ma = trailer_magic[:2]
max_align = min(int.from_bytes(ma, "little"), int.from_bytes(ma, "big"))
assert max_align in [8, 16, 32], "Invalid alignment"

Well, something like that...

@mingulov
Copy link
Contributor Author

mingulov commented Sep 6, 2023

Thank you.

I have checked deeper, unfortunately dumpinfo does not support big endian images at all (or to be more precise - images with non sys.byteorder endianness), it fails much earlier due to the 1st data parsing:
https://github.com/mcu-tools/mcuboot/blob/main/scripts/imgtool/dumpinfo.py#L71

_header = struct.unpack('IIHHIIBBHI', b[:28])

(incorrect protected_tlv_size, img_size will trigger struct.error).

So there is no need to implement separate support for 'max_align' only, but at that line it should not be "little" at least, as it should be that "default sys.byteorder".

Almost the same issue with 'verify' command: https://github.com/mcu-tools/mcuboot/blob/main/scripts/imgtool/image.py#L629

Might be I will take a look, can Big Endian images just started to be supported properly by dumpinfo / verify.

@mingulov
Copy link
Contributor Author

mingulov commented Sep 7, 2023

After more checks - it would be better to do it properly and not re-write again when add proper support for little/big endianness.

@mingulov mingulov closed this Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants