Skip to content

efi: Install grub efi modules needed for UKIs (in case they are not b… #1165

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

Open
alexlarsson wants to merge 1 commit into
base: main
Choose a base branch
from
+52 −0
Open

efi: Install grub efi modules needed for UKIs (in case they are not b… #1165

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions src/efi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,15 @@ pub(crate) const SHIM: &str = "shimx64.efi";
#[cfg(target_arch = "riscv64")]
pub(crate) const SHIM: &str = "shimriscv64.efi";

#[cfg(target_arch = "aarch64")]
const GRUB_EFI_MODULE_DIR: &str = "arm64-efi";

#[cfg(target_arch = "x86_64")]
const GRUB_EFI_MODULE_DIR: &str = "x86_64-efi";

@Johan-Liebert1 Johan-Liebert1 Oct 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder if this is correct. In the image quay.io/fedora/fedora-bootc:rawhide I see

/usr/lib/grub$ uname -m
x86_64
/usr/lib/grub$ ls -a
.  ..  i386-pc

@alexlarsson alexlarsson Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thats the 32bit modules. The 64bit ones are in the grub2-efi-x64-modules rpm, which you probably don't need as the x86-64 grub includes a ton of modules.

@Johan-Liebert1 Johan-Liebert1 Oct 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My question was regarding this directory itself. In my non-coreos system I see both i386-pc and x86_64-efi inside of /usr/lib/grub, but on coreos and bootc images I only see i386-pc. Wouldn't this always skip the module copy on x86-64 systems?

@alexlarsson alexlarsson Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The grub2-efi-x64-modules rpm is not installed in the coreos x86-64 images, because it is not needed, because on this arch the modules are built-in (on fedora at least). So, indeed, this would skip the copy on x86-64, but that is fine. The problem is on aarch64 where these modules are not built in. Or any non-fedora system where we don't know what is built in or not.


#[cfg(target_arch = "riscv64")]
const GRUB_EFI_MODULE_DIR: &str = "riscv64-efi";

/// The mount path for uefi
const EFIVARFS: &str = "/sys/firmware/efi/efivars";

Expand All @@ -63,6 +72,38 @@ const STUB_INFO_VAR_STR: &str = "StubInfo-4a67b082-0a4c-41cf-b6c7-440b29bb8c4f";
/// The options of cp command for installation
const OPTIONS: &[&str] = &["-rp", "--reflink=auto"];

// GRUB needs FAT filesystem and chainloading support to be able to boot UKIs.
// Both may be packaged as external modules (not builtin), so copy them in to be safe.

@cgwalters cgwalters Oct 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But this should xref https://bugzilla.redhat.com/show_bug.cgi?id=2545182 right?

And isn't fat always builtin on relevant platforms?

I'm not opposed to ~fast tracking the grub change here but I think we should eventually remove it and not be second guessing the grub setup

@alexlarsson alexlarsson Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't understand what you mean by second-guessing the grub setup? Like, which modules are built-in or not is a build time decision that can differ based on who created the image (distro, version, arch, etc).

Are you saying that bootupd should enforce os vendors to build in these modules and break if they are not?

@cgwalters cgwalters Oct 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

First: This is a complex, messy issue - and thank you for working on it!

Are you saying that bootupd should enforce os vendors to build in these modules and break if they are not?

No - because we are obviously shipping and supporting non-UKI systems too? Why should bootupd strictly enforce that the chain module is present?

I'm just saying: If the OS vendor expects UKIs to be used, they should do so in their grub config. Right?

@alexlarsson alexlarsson Oct 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

How can they enforce that the chain module is present? Obviously they can ship it in the original image, just like they ship the grub efi binary. However, the problem is that (at least when this is used with bootc) that file will then not then be available to the grub efi binary during boot where it is looking (i.e. in /boot/grub2/...). Given that bootupd is what is responsible for copying other things into /boot, it seems to make sense to copy these?

I mean, I get your point that its a bit weird to hardcode these two modules, but how else can an image supply content in /boot?

@alexlarsson alexlarsson Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I mean, we could maybe look into some modules directory in /usr/lib/efi/grub2 for additional modules to install?

@cgwalters cgwalters Oct 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Umm...I think one issue here is the logic seems to be bypassing the "filetree" stuff we have to handle merging content - if grub stops shipping a module we'll just leak it right?

I think that may be a root problem here.

@alexlarsson alexlarsson Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, clearly this approach is not quite correct. But I don't really see what the ideal solution should be.

@cgwalters cgwalters Oct 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Well...one thing going on is "grub-cc" which is basically getting us to a "single binary" like approach that systemd-boot has which dramatically simplifies things.

I think we should encourage that...

But failing that I think we need parity with dnf upgrade grub...are module changes installed there? If so we need to handle that. If not, that should be fixed and we ensure our "payload scan" pulls in that content in the same way.

const GRUB_EFI_MODULES: [&str; 2] = ["fat.mod", "chain.mod"];

/// Copy the external modules for booting UKIs to /boot when the image provides
/// them. A GRUB EFI binary may already contain these modules.
fn install_grub_efi_modules(source_root: &Dir, target_root: &Dir) -> Result<()> {
let target_dir = format!("boot/{}/{GRUB_EFI_MODULE_DIR}", grubconfigs::GRUB2DIR);

for module in GRUB_EFI_MODULES {
let source = format!("usr/lib/grub/{GRUB_EFI_MODULE_DIR}/{module}");
if !source_root.try_exists(&source)? {
continue;
}

target_root.create_dir_all(&target_dir)?;
let target = target_root.open_dir(&target_dir)?;
target
.atomic_replace_with(module, |f| -> std::io::Result<()> {
let mut source = source_root.open(&source)?;
std::io::copy(&mut source, f)?;
f.get_ref()
.as_file()
.set_permissions(source.metadata()?.permissions())?;
Ok(())
})
.with_context(|| format!("Installing GRUB module {module}"))?;
}

Ok(())
}

/// Check if the given path is a mount point via statx(MOUNT_ROOT).
fn is_mount_point(path: &Path) -> Result<bool> {
use rustix::fs::{AtFlags, StatxAttributes, StatxFlags};
Expand Down Expand Up @@ -476,6 +517,9 @@ impl Component for Efi {
drop(efidir);
self.unmount().context("unmount after adopt")?;
}
if get_bootloader()? == Bootloader::Grub {
install_grub_efi_modules(&rootcxt.sysroot, &rootcxt.sysroot)?;
}
Ok(Some(InstalledContent {
meta: updatemeta.clone(),
filetree: Some(updatef),
Expand Down Expand Up @@ -573,6 +617,11 @@ impl Component for Efi {
}
}
}
if bootloader == Bootloader::Grub {
let dest_root = Dir::open_ambient_dir(dest_root, ambient_authority())
.with_context(|| format!("opening destination root {dest_root}"))?;
install_grub_efi_modules(&src_dir, &dest_root)?;
}
Ok(InstalledContent {
meta,
filetree: Some(ft),
Expand Down Expand Up @@ -639,6 +688,9 @@ impl Component for Efi {
drop(destdir);
self.unmount().context("unmount after update")?;
}
if bootloader == Bootloader::Grub {
install_grub_efi_modules(&rootcxt.sysroot, &rootcxt.sysroot)?;
}

let adopted_from = None;
Ok(InstalledContent {
Expand Down
Loading