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

22 Support shim fallback bootchain #106

Open
wants to merge 11 commits into
base: 22
Choose a base branch
from

Conversation

olivercalder
Copy link
Member

This PR adds fallback shim and grub EFI assets to EFI/ubuntu/, while leaving non-fallback assets in EFI/BOOT/.

This is part of larger work on the snapd side to explicitly set EFI boot variables on install, thus no longer relying on default EFI boot behavior, which can be affected by such things as attaching external media.

The corresponding snapd PR is:

For those internal to Canonical, the spec related to this work can be found at:

This is a revival of previous PRs which sought to add shim fallback support:

Similar changes should be ported to core24, and perhaps backported to core20 as well (likely not core18 or core16, though).

xnox and others added 4 commits September 25, 2023 00:20
The fallback grub EFI asset is already in EFI/ubuntu/, but the
non-fallback grub asset should be in EFI/BOOT/ as well.

The sources for these grub assets are identical.

This corresponds to work in snapd PR #13205.  See the following for more
details:
- canonical/snapd#13205

Signed-off-by: Oliver Calder <[email protected]>
content:
- source: grubx64.efi
target: EFI/boot/grubx64.efi
target: EFI/BOOT/grubx64.efi
Copy link
Collaborator

@valentindavid valentindavid Sep 25, 2023

Choose a reason for hiding this comment

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

This is right.

I was a bit confused on why we still need this. But shim is configured to boot \grubx64.efi (DEFAULT_LOADER). And generate_path_from_image_path from shim considers paths that do not start with \EFI\ to be relative.

I wonder if it would be better to sort these content entries by target directory. So it is easier to see what goes in EFI\BOOT and what goes in EFI\ubuntu.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, good to know. I totally agree about sorting, I'll fix that in a moment.

Copy link
Member Author

Choose a reason for hiding this comment

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

@alfonsosanchezbeato do you think grub${arch}.efi is required in EFI/BOOT/ because of this?

Choose a reason for hiding this comment

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

Afaict if the fallback binary is in EFI\boot, it will get run instead of grub, and even if it does not find any CSV file, grub*.efi will not be invoked, so I don't see it necessary. @valentindavid please correct if I am getting something wrong here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are right. This is how it seems documented.

target: EFI/BOOT/BOOTX64.efi
- source: fbx64.efi
target: EFI/BOOT/fbx64.efi
- source: mmx64.efi
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the mok manager should also go in EFI\ubuntu.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good. I don't really know what the mok manager is used for, and there was some consternation about adding it in the past: #56 (comment)

It's not one of the boot assets added by the snapd PR #13205 (https://github.com/snapcore/snapd/pull/13205/files#diff-dca334c34d6acf0ce9fb49a95fb836caafe064f7fd2eda43a2185d6d9c7e20e5R464-R476), so I'm not sure if it's something we actually need to add.

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would love to have it. The mok keys can be used to sign your own kernels. But it can also be used by anything that uses the secondary trusted keyring in the kernel: kernel modules, signed dm-verity, etc.

Also there is no security argument here, because there is a signed efi that anyone can install manually. We are just making it more annoying to install it for legitimate users.

If it causes some issues and would delay this PR, you do not have to add it.

But for sure, if we add it in EFI\BOOT, it should also go in EFI\ubuntu. It feels very weird to have it in only one. That would be, you can install machine owner keys, but only if in the bios, you force to boot the default efi bootloader (EFI\BOOT\bootx64.efi) instead of the configured BootOrder.

To sum up. Either add EFI\ubuntu\mmx64.efi or remove EFI\BOOT\mmx64.efi.

Copy link
Member

Choose a reason for hiding this comment

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

Probably we can live with MokManager only in EFI\ubuntu, as we have seen that grub will not ever run from EFI\boot if we have fbx64.efi. As MM is called either when shim tries to run grub but it has not a valid signature or when the OS modifies EFI variables related to the MoK - and the OS is expected to have been started by grub in EFI\ubuntu.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. I've removed it now, thanks!

@mwhudson
Copy link

This doesn't build because fb$arch.efi and mm$arch.efi get deleted in snapcraft.yaml, is that expected?

Copy link
Member

@alfonsosanchezbeato alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

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

This should be proposed to branch 24 first as it is quite a risky change. There should be also an assumes in snapcraft.yaml for the snapd version that will contain the changes needed for this to happen. Before that this PR cannot be merged.

gadget/gadget-amd64.yaml Outdated Show resolved Hide resolved
- source: grubx64.efi
target: EFI/boot/grubx64.efi
target: EFI/BOOT/grubx64.efi

Choose a reason for hiding this comment

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

Are we sure we need grub in EFI/BOOT? Afaiu reading shim code, shim would never run it if it finds EFI/BOOT/fbx64.efi

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you are right.

…hanged"

This reverts commit faf0d0e.

Signed-off-by: Oliver Calder <[email protected]>
The only grub which should ever be run is `EFI/ubuntu/grub${arch}.efi`.
In no case should `EFI/BOOT/grub${arch}.efi` ever be executed.

The possible boot patterns are:
1. If EFI boot variables are set correctly, boot the desired asset
   correctly (namely, `EFI/ubuntu/shim${arch}.efi`).
2. If the system supports fallback, then `EFI/BOOT/fb${arch}.efi` is
   run, which searches for `BOOT${arch}.CSV`, finds it in `EFI/ubuntu/`,
   and thus boots the corresponding assets in `EFI/ubuntu/`.
3. If fallback is not supported, the system directly boots
   `EFI/BOOT/BOOT${arch}.efi`.

Thus, `EFI/BOOT/grub${arch}.efi` should never be run, so it should be
removed.

Signed-off-by: Oliver Calder <[email protected]>
Since there is no grub binary in `EFI/boot/`, there is no need for mok
manager binaries in `EFI/boot/` either.

Signed-off-by: Oliver Calder <[email protected]>
@olivercalder olivercalder changed the title Support shim fallback bootchain 22 Support shim fallback bootchain Oct 5, 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.

5 participants