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

stub: Add support for .ucode EFI addons #32463

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

tfg13
Copy link

@tfg13 tfg13 commented Apr 24, 2024

Hello!

This is the promised followup for #31872, and .ucode section support to addon files.

I implemented largely as discussed with @poettering here: uapi-group/specifications#98

We allow one .ucode section per UKI, and addon file and then merge them in the following order:

  1. UKI embedded .ucode section
  2. Global addons
  3. UKI addons

Note this order is different from what was discussed in the thread above. The reason for that is that both cmdline and devicetree do it in this order, so I figured this is more consistent. (And makes sense IMHO, if one drops an addon file to change a (distro-)shipped UKI).

This is now also aligned with the measurement order.

[Edit]: This changed again, see below for context. The order is now UKI addons, Global addons, UKI embedded.

@tfg13
Copy link
Author

tfg13 commented Apr 26, 2024

I still need to do the manpages and docs, but looking for input on the questions above first

Edit: This changed, see next comment

@tfg13 tfg13 marked this pull request as ready for review April 26, 2024 11:08
@github-actions github-actions bot added the please-review PR is ready for (re-)review by a maintainer label Apr 26, 2024
@tfg13
Copy link
Author

tfg13 commented May 1, 2024

I changed the order now and updated the initial comment to reflect that. I think the order makes more sense this way, is the same as with cmdline and devicetree addons, and also matches the measurement order. Let me know what you think.

Copy link

github-actions bot commented May 1, 2024

Important

An -rc1 tag has been created and a release is being prepared, so please note that PRs introducing new features and APIs will be held back until the new version has been released.

@YHNdnzj
Copy link
Member

YHNdnzj commented May 1, 2024

I changed the order now and updated the initial comment to reflect that. I think the order makes more sense this way, is the same as with cmdline and devicetree addons, and also matches the measurement order. Let me know what you think.

Hmm, so I guess the original idea is that since kernel stops on first match, and addons are the way to override/extend what's set in the UKI, we still need the ucodes in addons to have higher priorities.

I'm not familiar with device trees, but for cmdline, the same option defined later overrides the former ones. So in that case appending what's from addon to the cmdline from UKI makes sense, but not so much for ucodes.

@YHNdnzj
Copy link
Member

YHNdnzj commented May 1, 2024

cc @poettering

@tfg13
Copy link
Author

tfg13 commented May 2, 2024

Hmm, so I guess the original idea is that since kernel stops on first match, and addons are the way to override/extend what's set in the UKI, we still need the ucodes in addons to have higher priorities.

I'm not familiar with device trees, but for cmdline, the same option defined later overrides the former ones. So in that case appending what's from addon to the cmdline from UKI makes sense, but not so much for ucodes.

Oh interesting point. I was thinking about how the final initrd filesystem looks like. But you make a good point, the initial cpio parser is more naive and will indeed stop on the first match.

Alright, let me invert this again. Do you have opinions on the measurement question? I assume we want to measure things in the order they apply in the end? The embedded .ucode section is measured as part of the general UKI measurements, which is much earlier. I guess I will skip it there, and then measure it later, last, as part of the ucode cpio merging logic.

@tfg13
Copy link
Author

tfg13 commented May 3, 2024

Ok, I reworked it again. The order for microcode updates is now:

  1. UKI addons
  2. Global addons
  3. UKI embedded section

They are also measured in this order.

The measurement part is a bit tricky and we might want to talk about it. Two things:

  • Since the embedded section is now last, I am skipping it in the initial measurement loop that otherwise measures all the UKI sections. It gets measured later in the code. (This techincally violates the comment in uki.h a bit, that says the order defined there is always the measurement order. I don't have a good alternative here, I think we must measure in the order the files will be used in the end, no?)
  • As is its right now, the measurement of UKIs with embedded .ucode sections is not correctly predictable for systemd-pcrlock. From what I can tell, this is also true for other addons like cmdline, (I think pcrlock doesn't full handle them, right?), but it might be worse here, since .ucode sections are fundamentally initrds, so they go into the initrd register.

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

Successfully merging this pull request may close these issues.

None yet

2 participants