Skip to content

Add support for MBN header v7 #5

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
Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Add support for MBN header v7 #5

wants to merge 11 commits into from

Conversation

185264646
Copy link

It's tested on a IPQ5424 based board without secure boot enabled. Not sure if it's compatible with snapdragon SoCs.

stephan-gh
stephan-gh reviewed Jun 30, 2025
View reviewed changes
Copy link
Member

@stephan-gh stephan-gh left a comment

Choose a reason for hiding this comment

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

Thanks a lot, this is looking great already!

@185264646
Copy link
Author

Thanks a lot, this is looking great already!

I guess so as long as fused SoC will not be supported by this tool. I guess the user of this tool wouldn't care about that too much.

@stephan-gh
Copy link
Member

I guess so as long as fused SoC will not be supported by this tool. I guess the user of this tool wouldn't care about that too much.

The whole tool is intentionally designed to do the absolute minimum necessary to support developer use cases. Even for the older header versions it doesn't know how to properly generate signatures for example. Better than having no tool at all :)

185264646 added 3 commits July 2, 2025 03:22
@185264646
Add support for MBN v7
c8c91ee 
MBN header v7 is found to be used in some most recent QCA SoCs (e.g.
IPQ5424). Add support for that.
@185264646
README.md: add some supported SoCs
fa9f143
@185264646
hashseg: only sets metadata to 120B for MBN v6
90d5460 
Not all SoCs sets metadata size to 120B. Do it only for V6 MBNs. Leave
it empty to mimic the behavior of IPQ5424(MBN v7).
@185264646
Copy link
Author

It there anything pending? All the information in current code can be got from open-source projects. I can't see any obvious risk now.

@stephan-gh
Copy link
Member

Thanks, I need to find time to try and test this. The changes seem fine, I'll handle the rest and merge this soon.

stephan-gh added 7 commits July 6, 2025 18:00
@stephan-gh
hashseg: Fix MBNv7 total_size and clarify descriptions
0818b15 
At the moment, we model the "common metadata" as part of the MBNv7 header,
so the common_metadata_size is part of the header size (included in the
FORMAT.size). This means we should not add it to the "total_size", or it
ends up in there twice.

Remove it from total_size, add an assertion to ensure the total_size always
matches what we actually produce, and clarify the descriptions a bit to
make that more clear.
@stephan-gh
hashseg: Add missing QC signature/cert chain size for v7
7c9f40e 
We don't set these anywhere at the moment, but for consistency with v6 the
sizes should be set correctly and included in total_size.
@stephan-gh
hashseg: Fix v7 metadata order
2167726 
Given v6 it seems more likely the metadata from QC comes before the OEM
metadata. Also, comparing metadata headers from different firmware images
only the second metadata varies across OEMs, so the second one is likely
the OEM metadata. For qtestsign, this currently makes no functional
difference, since we don't generate any QC metadata.
@stephan-gh
hashseg: Set correct hash_table_size for v7
94f2fd6 
"hash_table_size" in v7 seems to be essentially the "hash_size" in older
MBN header versions. We should set it to the total size of all hashes we
include in the header. The "_HashSegment" base class does that already when
the field is called "hash_size", so let's just rename it to make that work.
@stephan-gh
hashseg: Generate empty OEM 2.0 metadata for MBNv7
a577759 
Having non-empty OEM metadata for MBNv7 seems to be required on some
platforms. The definition with the individual fields is even available in
Coreboot, but for now it seems enough to generate a zeroed block like we do
for v6.
@stephan-gh
hashseg: Clarify ELF PHDR flag definition based on Coreboot
c371ef9 
Looks like the flags in the ELF PHDR used by QC represent certain fields
that we can access with masks and shifts. The access type used for the hash
segment on newer platforms seems to have changed, so we no longer manage to
drop the hash segment in that case.

Fix this by checking just the relevant bits and use the chance to clarify
the bits we use.
@stephan-gh
qtestsign.py: Add a few more IDs found upstream in linux-firmware
4169de1
@stephan-gh
Copy link
Member

I managed to test this successfully on a different platform after some more fixes/cleanup. In particular:

  • Some size calculations were wrong
  • I need to fill the hash_table_size field with the total size of all hashes
  • I need to generate at least a zeroed out metadata block

Could you try if these changes are still working for you? If not, we might need to fill the metadata with more values and/or add some extra options to make that conditional.

@185264646
Copy link
Author

I managed to test this successfully on a different platform after some more fixes/cleanup. In particular:

* Some size calculations were wrong

* I need to fill the `hash_table_size` field with the total size of all hashes

* I need to generate at least a zeroed out metadata block

Could you try if these changes are still working for you? If not, we might need to fill the metadata with more values and/or add some extra options to make that conditional.

i'll report the result in the next week to make sure it's not broken for the board i'm working on. Luckily EDL is working so i'm not worried about getting it bricked. :)

@185264646
Copy link
Author

185264646 commented Jul 11, 2025 •
edited
Loading

Ah. I've just found that they(qca people) actually open-sourced the script which i've found in the QSDK.
here you are: https://git.codelinaro.org/clo/qsdk/oss/system/tools/meta/-/tree/NHSS.QSDK.13.0.5.r2/scripts?ref_type=heads
mbn_tool.py is the critical library which contains the field names.
elftombn.py is the CLI tool to convert ELF to MBN.
There are also some other interesting scripts there, might be helpful for some other purposes.

See https://git.codelinaro.org/clo/qsdk/oss/system/tools/meta/-/blame/NHSS.QSDK.13.0.5.r2/scripts/mbn_tools.py?ref_type=heads#L620 for MBN v7 definition.

I would like to revert to use the old field names in mbn_tool.py instead. I can handle all modifications if @stephan-gh agree.
You may also try the tool for your device and image.

Of course this repo should be added to comment for reference.

@stephan-gh
Copy link
Member

It's good to know that the QSDK script is also open-source. I think the field names we have right are fine though and don't need to be changed:

  • For code_size vs hash_table_size vs hash_size it's obvious after my testing that this indeed represents the size of all hashes and not the actual "code" size. There used to be a code_size field in older MBN versions, so I think this is just misnamed in the QCA script.
  • major/minor vs common_metadata_major_version/common_metadata_minor_version: This isn't the major/minor version of the MBN header, but the separate "common metadata" header, so I'd say the name we have right now is slightly more clear.
  • software_id/secondary_software_id vs sw_id/sec_sw_id: This is just a different abbreviation, no real difference.

What changes would you like to make exactly?

Personally, I would just keep it as-is at this point. Have you tried if my changes still work on your IPQ board?

@stephan-gh
Copy link
Member

Also note that the various open-source versions of "mbn_tools" in Coreboot, QCA etc all seem to be limited to one particular use case (= building Coreboot, or U-Boot/whatever for QCA etc). They don't have a particularly complete or accurate implementation of the MBN format either. This is why qtestsign is usually based on the combination of multiple sources (the old signlk tool, Coreboot, etc), some guessing plus lots of experimentation.

@185264646
Copy link
Author

It's good to know that the QSDK script is also open-source. I think the field names we have right are fine though and don't need to be changed:

* For `code_size` vs `hash_table_size` vs `hash_size` it's obvious after my testing that this indeed represents the size of all hashes and not the actual "code" size. There used to be a `code_size` field in older MBN versions, so I think this is just misnamed in the QCA script.

* `major`/`minor` vs `common_metadata_major_version`/`common_metadata_minor_version`: This isn't the major/minor version of the MBN header, but the separate "common metadata" header, so I'd say the name we have right now is slightly more clear.

* `software_id`/`secondary_software_id` vs `sw_id`/`sec_sw_id`: This is just a different abbreviation, no real difference.

What changes would you like to make exactly?

Personally, I would just keep it as-is at this point. Have you tried if my changes still work on your IPQ board?

Okay. I've just tested. Your commits are fine for unfused IPQ5424 based device.

@stephan-gh
Copy link
Member

Are there any changes you would still like to do to this PR? Otherwise, I'll merge it. It's good to go from my side.

@185264646
Copy link
Author

185264646 commented Jul 12, 2025 •
edited
Loading

I generally can't see anything wrong in these commits.
Could you please add the link i've mentioned (QSDK scripts repo)? After that I think this can be merged.

@stephan-gh
Copy link
Member

I have added a link to the README. I see no real reason to mention in within the comment in the code right now, because we've effectively taken all names etc from the Coreboot source.

Does this look good to you?

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

@stephan-gh stephan-gh stephan-gh left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@185264646 @stephan-gh