bootloader: Fix a few things related to systemd-boot installs

crates/lib/src/bootc_composefs/boot.rs

@@ -1145,6 +1145,109 @@ pub(crate) fn setup_composefs_uki_boot(
     Ok(boot_digest)
 }
 
+/// RAII guard that unmounts the ESP on drop.
+struct EspMountGuard(std::path::PathBuf);
+
+impl Drop for EspMountGuard {
+    fn drop(&mut self) {
+        if let Err(e) = rustix::mount::unmount(&self.0, rustix::mount::UnmountFlags::DETACH) {
+            tracing::warn!("Failed to unmount ESP from {}: {e:?}", self.0.display());
+        }
+    }
+}
+
+/// A composefs image attached to a temporary directory with the ESP mounted
+/// inside it, ready for bootloader installation.
+///
+/// The composefs image (a detached `fsmount(2)` fd with no VFS path) is
+/// attached to a tmpdir via `move_mount(2)`, giving us a real filesystem path
+/// that `mount(2)` and bootctl can use.  The ESP is then mounted at
+/// `<tmpdir>/efi` (if that directory exists in the image) or `<tmpdir>/boot`,
+/// per the Boot Loader Specification.
+///
+/// Drop order is significant: the ESP guard is declared first so it is dropped
+/// (unmounted) before the composefs tmpdir is unmounted.
+pub(crate) struct MountedImageRoot {
+    // Declared first: unmounted before `composefs` on drop.
+    _esp_guard: EspMountGuard,
+    composefs: TempMount,
+    pub(crate) esp_subdir: &'static str,
+}
+
+impl MountedImageRoot {
+    /// Find the ESP on `device`, attach the composefs image to a tmpdir, and
+    /// mount the ESP inside it.
+    // TODO: install to all ESPs on multi-device setups
+    #[context("Preparing image root for bootloader installation")]
+    pub(crate) fn new(
+        composefs_mnt_fd: std::os::fd::OwnedFd,
+        device: &bootc_blockdev::Device,
+    ) -> Result<Self> {
+        let roots = device.find_all_roots()?;
+        let mut esp_part = None;
+        for root in &roots {
+            if let Some(esp) = root.find_partition_of_esp_optional()? {
+                esp_part = Some(esp);
+                break;
+            }
+        }
+        let esp_part = esp_part.ok_or_else(|| anyhow!("ESP partition not found"))?;
+
+        // Attach the detached composefs fsmount fd to a real tmpdir path so
+        // that mount(2) and bootctl --root can work with it.
+        let composefs = TempMount::mount_fd(composefs_mnt_fd)
+            .context("Attaching composefs image to temporary directory")?;
+
+        // Per BLS: if only an ESP is present (no XBOOTLDR), mount it to /boot.
+        // If both ESP and XBOOTLDR are present, mount the ESP to /efi instead.
+        // /boot/efi is explicitly discouraged by the spec.
+        let esp_subdir = if composefs
+            .fd
+            .metadata("efi")
+            .map(|m| m.is_dir())
+            .unwrap_or(false)
+        {
+            "efi"
+        } else {
+            "boot"
+        };
+
+        let esp_mount_target = composefs.dir.path().join(esp_subdir);
+        rustix::mount::mount(
+            esp_part.path(),
+            &esp_mount_target,
+            "vfat",
+            MountFlags::NOEXEC | MountFlags::NOSUID,
+            Some(c"fmask=0177,dmask=0077"),
+        )
+        .with_context(|| format!("Mounting ESP at {}", esp_mount_target.display()))?;
+
+        Ok(Self {
+            _esp_guard: EspMountGuard(esp_mount_target),
+            composefs,
+            esp_subdir,
+        })
+    }
+
+    /// The composefs image as a capability-safe directory (for file reads).
+    pub(crate) fn dir(&self) -> &Dir {
+        &self.composefs.fd
+    }
+
+    /// Real filesystem path of the composefs tmpdir root.
+    pub(crate) fn root_path(&self) -> &std::path::Path {
+        self.composefs.dir.path()
+    }
+
+    /// Open the mounted ESP as a capability-safe directory.
+    pub(crate) fn open_esp_dir(&self) -> Result<Dir> {
+        self.composefs
+            .fd
+            .open_dir(self.esp_subdir)
+            .with_context(|| format!("Opening ESP at /{}", self.esp_subdir))
+    }
+}
+
 pub struct SecurebootKeys {
     pub dir: Dir,
     pub keys: Vec<Utf8PathBuf>,
@@ -1225,13 +1328,12 @@ pub(crate) async fn setup_composefs_boot(
     let entries =
         get_boot_resources(&fs, &*repo).context("Extracting boot entries from OCI image")?;
 
-    let mounted_fs = Dir::reopen_dir(
-        &repo
-            .mount(&id.to_hex())
-            .context("Failed to mount composefs image")?,
-    )?;
+    let composefs_mnt_fd = repo
+        .mount(&id.to_hex())
+        .context("Failed to mount composefs image")?;
+    let mounted_root = MountedImageRoot::new(composefs_mnt_fd, &root_setup.device_info)?;
 
-    let postfetch = PostFetchState::new(state, &mounted_fs)?;
+    let postfetch = PostFetchState::new(state, mounted_root.dir())?;
 
     let boot_uuid = root_setup
         .get_boot_uuid()?
@@ -1253,11 +1355,9 @@ pub(crate) async fn setup_composefs_boot(
         )?;
     } else {
         crate::bootloader::install_systemd_boot(
-            &root_setup.device_info,
-            &root_setup.physical_root_path,
+            &mounted_root,
             &state.config_opts,
-            None,
-            get_secureboot_keys(&mounted_fs, BOOTC_AUTOENROLL_PATH)?,
+            get_secureboot_keys(mounted_root.dir(), BOOTC_AUTOENROLL_PATH)?,
         )?;
     }
 
@@ -1280,7 +1380,7 @@ pub(crate) async fn setup_composefs_boot(
             repo,
             &id,
             entry,
-            &mounted_fs,
+            mounted_root.dir(),
         )?,
         BootType::Uki => setup_composefs_uki_boot(
             BootSetupType::Setup((&root_setup, &state, &postfetch)),

crates/lib/src/bootloader.rs

@@ -10,7 +10,7 @@ use fn_error_context::context;
 
 use bootc_mount as mount;
 
-use crate::bootc_composefs::boot::{SecurebootKeys, mount_esp};
+use crate::bootc_composefs::boot::{MountedImageRoot, SecurebootKeys};
 use crate::utils;
 
 /// The name of the mountpoint for efi (as a subdirectory of /boot, or at the toplevel)
@@ -218,66 +218,83 @@ pub(crate) fn install_via_bootupd(
     }
 }
 
-/// Install systemd-boot to the first ESP found among backing devices.
+/// Install systemd-boot using a pre-prepared boot root.
 ///
-/// On multi-device setups only the first ESP is installed to; additional
-/// ESPs on other backing devices are left untouched.
-// TODO: install to all ESPs on multi-device setups
+/// `bootctl install` is invoked with `--root` pointing at the composefs
+/// tmpdir (which already has the ESP mounted inside it), so bootctl reads
+/// `/etc/os-release` from the actual target OS.  Additional safeguards for
+/// sandbox environments (`podman run --read-only`, osbuild, etc.):
+///
+/// - `KERNEL_INSTALL_CONF_ROOT` redirects bootctl's entry-token write into
+///   the ESP (writable FAT) rather than the EROFS composefs root.
+/// - `--no-variables` (when `generic_image`) prevents EFI variable writes.
+/// - `SYSTEMD_RELAX_ESP_CHECKS=1` skips partition-type GUID validation.
 #[context("Installing bootloader")]
 pub(crate) fn install_systemd_boot(
-    device: &bootc_blockdev::Device,
-    _rootfs: &Utf8Path,
+    prepared_root: &MountedImageRoot,
     configopts: &crate::install::InstallConfigOpts,
-    _deployment_path: Option<&str>,
     autoenroll: Option<SecurebootKeys>,
 ) -> Result<()> {
-    let roots = device.find_all_roots()?;
-    let mut esp_part = None;
-    for root in &roots {
-        if let Some(esp) = root.find_partition_of_esp_optional()? {
-            esp_part = Some(esp);
-            break;
-        }
-    }
-    let esp_part = esp_part.ok_or_else(|| anyhow::anyhow!("ESP partition not found"))?;
-
-    let esp_mount = mount_esp(&esp_part.path()).context("Mounting ESP")?;
-    let esp_path = Utf8Path::from_path(esp_mount.dir.path())
-        .ok_or_else(|| anyhow::anyhow!("Failed to convert ESP mount path to UTF-8"))?;
-
     println!("Installing bootloader via systemd-boot");
 
-    let mut bootctl_args = vec!["install", "--esp-path", esp_path.as_str()];
+    let root_path = prepared_root
+        .root_path()
+        .to_str()
+        .ok_or_else(|| anyhow::anyhow!("composefs tmpdir path is not UTF-8"))?;
+    let esp_path_in_root = format!("/{}", prepared_root.esp_subdir);
+
+    let mut bootctl_args = vec![
+        "install",
+        "--root",
+        root_path,
+        "--esp-path",
+        esp_path_in_root.as_str(),
+        // Do NOT add --boot-path here. Passing it with the same value as
+        // --esp-path causes bootctl to treat it as XBOOTLDR and validate the
+        // partition-type GUID against the XBOOTLDR GUID, which fails on a
+        // normal ESP. Omitting it is correct when there is no separate
+        // XBOOTLDR partition.
+    ];
 
     if configopts.generic_image {
-        bootctl_args.extend(["--random-seed", "no"]);
+        bootctl_args.extend(["--random-seed", "no", "--no-variables"]);
     }
 
     Command::new("bootctl")
         .args(bootctl_args)
+        // Skip partition-type GUID validation; not reliably detectable in all
+        // installation environments.
+        .env("SYSTEMD_RELAX_ESP_CHECKS", "1")
+        // Redirect the entry-token write into the ESP (writable FAT) rather
+        // than the EROFS composefs root.  bootctl prepends --root to this
+        // path, so supply the root-relative path (e.g. "/boot").
+        .env("KERNEL_INSTALL_CONF_ROOT", &esp_path_in_root)
         .log_debug()
-        .run_inherited_with_cmd_context()?;
+        // Capture stderr so bootctl error messages appear in our error chain.
+        .run_capture_stderr()?;
 
     if let Some(SecurebootKeys { dir, keys }) = autoenroll {
-        let path = esp_path.join(SYSTEMD_KEY_DIR);
-        create_dir_all(&path)?;
-
-        let keys_dir = esp_mount
-            .fd
+        let esp_dir = prepared_root.open_esp_dir()?;
+        let keys_path = prepared_root
+            .root_path()
+            .join(prepared_root.esp_subdir)
+            .join(SYSTEMD_KEY_DIR);
+        create_dir_all(&keys_path).with_context(|| {
+            format!("Creating secureboot key directory {}", keys_path.display())
+        })?;
+
+        let keys_dir = esp_dir
             .open_dir(SYSTEMD_KEY_DIR)
-            .with_context(|| format!("Opening {path}"))?;
+            .with_context(|| format!("Opening {SYSTEMD_KEY_DIR}"))?;
 
         for filename in keys.iter() {
-            let p = path.join(&filename);
-
-            // create directory if it doesn't already exist
-            if let Some(parent) = p.parent() {
-                create_dir_all(parent)?;
-            }
-
-            dir.copy(&filename, &keys_dir, &filename)
-                .with_context(|| format!("Copying secure boot key: {p}"))?;
-            println!("Wrote Secure Boot key: {p}");
+            dir.copy(filename, &keys_dir, filename)
+                .with_context(|| format!("Copying secure boot key {filename:?}"))?;
+            println!(
+                "Wrote Secure Boot key: {}/{}",
+                keys_path.display(),
+                filename.as_str()
+            );
         }
         if keys.is_empty() {
             tracing::debug!("No Secure Boot keys provided for systemd-boot enrollment");