From 13d127facd56b1397d4a9d2cdbbc83fbce5eed9f Mon Sep 17 00:00:00 2001 From: bfjelds Date: Mon, 1 Jun 2026 14:17:10 -0700 Subject: [PATCH 01/22] feat(uki): activate correct verity addon for target A/B slot (ACL only) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ACL images ship with PARTUUID-based verity addons — templates for both A and B slots stored in acl/uki-addons/ on the ESP, with slot A active by default. During an A/B update, trident must swap the active addon to match the target slot so the new UKI boots with the correct verity partition identity. Add activate_verity_addon_for_target_volume() which: - Checks for ACL verity addon templates on the image ESP - Copies the correct slot template into the staged addon directory - Is a silent no-op for non-ACL images (no template dir) - Errors if template dir exists but the selected slot is missing Called from copy_file_artifacts() after stage_uki_on_esp(), gated on ctx.image_distro().is_acl() to ensure only ACL images are affected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/trident/src/engine/boot/uki.rs | 270 ++++++++++++++++++++++++++ crates/trident/src/subsystems/esp.rs | 15 ++ 2 files changed, 285 insertions(+) diff --git a/crates/trident/src/engine/boot/uki.rs b/crates/trident/src/engine/boot/uki.rs index 4b5728d6c..7f4b47a58 100644 --- a/crates/trident/src/engine/boot/uki.rs +++ b/crates/trident/src/engine/boot/uki.rs @@ -396,6 +396,81 @@ pub fn find_previous_uki(esp_dir_path: &Path) -> Result { } } +/// Path within the image ESP where ACL stores verity addon templates for A/B slots. +const VERITY_ADDON_TEMPLATES_DIR: &str = "acl/uki-addons"; + +/// Filename of the active verity addon placed in the UKI's `.extra.d/` directory. +const VERITY_ADDON_FILENAME: &str = "verity.addon.efi"; + +/// After staging the UKI, activate the correct verity addon for the target +/// A/B volume. ACL images ship with slot-A active by default and include +/// templates for both slots in `acl/uki-addons/` on the ESP image. +/// +/// This is ACL-specific: if no verity addon templates exist on the image +/// (i.e. a non-ACL image), this function is a silent no-op. However, if +/// templates exist but the selected slot's template is missing, an error +/// is returned to prevent booting with the wrong slot's PARTUUIDs. +pub fn activate_verity_addon_for_target_volume( + image_esp_mount: &Path, + mount_point: &Path, + esp_mount_path: &Path, + target_volume: AbVolumeSelection, +) -> Result<(), Error> { + let template_dir = image_esp_mount.join(VERITY_ADDON_TEMPLATES_DIR); + if !template_dir.exists() { + // Image does not use PARTUUID-based verity addons (non-ACL or older ACL). + trace!( + "No verity addon template directory at '{}', skipping", + template_dir.display() + ); + return Ok(()); + } + + let template_name = match target_volume { + AbVolumeSelection::VolumeA => "verity-a.addon.efi", + AbVolumeSelection::VolumeB => "verity-b.addon.efi", + }; + + let template_path = template_dir.join(template_name); + ensure!( + template_path.exists(), + "Verity addon template '{}' not found in '{}' — cannot activate {:?}", + template_name, + template_dir.display(), + target_volume + ); + + let staging_addon_dir = join_relative(mount_point, esp_mount_path) + .join(UKI_DIRECTORY) + .join(TMP_UKI_ADDON_DIR_NAME); + + if !staging_addon_dir.exists() { + fs::create_dir_all(&staging_addon_dir).with_context(|| { + format!( + "Failed to create staged addon directory '{}'", + staging_addon_dir.display() + ) + })?; + } + + let dest = staging_addon_dir.join(VERITY_ADDON_FILENAME); + debug!( + "Activating verity addon for {:?}: '{}' → '{}'", + target_volume, + template_path.display(), + dest.display() + ); + fs::copy(&template_path, &dest).with_context(|| { + format!( + "Failed to copy verity addon template '{}' to '{}'", + template_path.display(), + dest.display() + ) + })?; + + Ok(()) +} + /// Construct the previous UKI filename and set it as the default boot entry. pub fn use_previous_uki_as_default(esp_dir_path: &Path) -> Result<(), TridentError> { let previous_uki_entry_path = find_previous_uki(esp_dir_path)?; @@ -858,4 +933,199 @@ mod tests { assert!(uki_dir.join("vmlinuz-100-azla2.efi").exists()); assert!(!uki_dir.join("vmlinuz-100-azla2.efi.extra.d").exists()); } + + // ── activate_verity_addon_for_target_volume tests ────────────────────── + + /// Helper: creates a mock image ESP with verity addon templates. + fn setup_image_with_verity_templates( + image_esp: &Path, + ) -> (PathBuf, PathBuf) { + let template_dir = image_esp.join(VERITY_ADDON_TEMPLATES_DIR); + fs::create_dir_all(&template_dir).unwrap(); + let a_path = template_dir.join("verity-a.addon.efi"); + let b_path = template_dir.join("verity-b.addon.efi"); + fs::write(&a_path, b"verity-a-content").unwrap(); + fs::write(&b_path, b"verity-b-content").unwrap(); + (a_path, b_path) + } + + /// Activating for VolumeA copies verity-a template into the staged addon dir. + #[test] + fn test_activate_verity_addon_volume_a() { + let image_esp = tempdir().unwrap(); + setup_image_with_verity_templates(image_esp.path()); + + let mount_point = tempdir().unwrap(); + prepare_esp_for_uki(mount_point.path(), Path::new(DEFAULT_ESP_MOUNT_POINT_PATH)) + .unwrap(); + + // Create the staged addon dir as stage_uki_on_esp would + let staged_addon_dir = join_relative(mount_point.path(), DEFAULT_ESP_MOUNT_POINT_PATH) + .join(UKI_DIRECTORY) + .join(TMP_UKI_ADDON_DIR_NAME); + fs::create_dir_all(&staged_addon_dir).unwrap(); + + activate_verity_addon_for_target_volume( + image_esp.path(), + mount_point.path(), + Path::new(DEFAULT_ESP_MOUNT_POINT_PATH), + AbVolumeSelection::VolumeA, + ) + .unwrap(); + + let active = staged_addon_dir.join(VERITY_ADDON_FILENAME); + assert!(active.exists()); + assert_eq!(fs::read(&active).unwrap(), b"verity-a-content"); + } + + /// Activating for VolumeB copies verity-b template into the staged addon dir. + #[test] + fn test_activate_verity_addon_volume_b() { + let image_esp = tempdir().unwrap(); + setup_image_with_verity_templates(image_esp.path()); + + let mount_point = tempdir().unwrap(); + prepare_esp_for_uki(mount_point.path(), Path::new(DEFAULT_ESP_MOUNT_POINT_PATH)) + .unwrap(); + + let staged_addon_dir = join_relative(mount_point.path(), DEFAULT_ESP_MOUNT_POINT_PATH) + .join(UKI_DIRECTORY) + .join(TMP_UKI_ADDON_DIR_NAME); + fs::create_dir_all(&staged_addon_dir).unwrap(); + + activate_verity_addon_for_target_volume( + image_esp.path(), + mount_point.path(), + Path::new(DEFAULT_ESP_MOUNT_POINT_PATH), + AbVolumeSelection::VolumeB, + ) + .unwrap(); + + let active = staged_addon_dir.join(VERITY_ADDON_FILENAME); + assert!(active.exists()); + assert_eq!(fs::read(&active).unwrap(), b"verity-b-content"); + } + + /// No template directory at all → silent no-op (backward compat with non-ACL). + #[test] + fn test_activate_verity_addon_no_template_dir() { + let image_esp = tempdir().unwrap(); + // No acl/uki-addons/ directory + + let mount_point = tempdir().unwrap(); + prepare_esp_for_uki(mount_point.path(), Path::new(DEFAULT_ESP_MOUNT_POINT_PATH)) + .unwrap(); + + // Should succeed silently + activate_verity_addon_for_target_volume( + image_esp.path(), + mount_point.path(), + Path::new(DEFAULT_ESP_MOUNT_POINT_PATH), + AbVolumeSelection::VolumeA, + ) + .unwrap(); + + // No addon dir should have been created + let staged_addon_dir = join_relative(mount_point.path(), DEFAULT_ESP_MOUNT_POINT_PATH) + .join(UKI_DIRECTORY) + .join(TMP_UKI_ADDON_DIR_NAME); + assert!(!staged_addon_dir.exists()); + } + + /// Template directory exists but selected slot template is missing → error. + #[test] + fn test_activate_verity_addon_missing_selected_template() { + let image_esp = tempdir().unwrap(); + let template_dir = image_esp.path().join(VERITY_ADDON_TEMPLATES_DIR); + fs::create_dir_all(&template_dir).unwrap(); + // Only write verity-a, not verity-b + fs::write(template_dir.join("verity-a.addon.efi"), b"a-content").unwrap(); + + let mount_point = tempdir().unwrap(); + prepare_esp_for_uki(mount_point.path(), Path::new(DEFAULT_ESP_MOUNT_POINT_PATH)) + .unwrap(); + + let result = activate_verity_addon_for_target_volume( + image_esp.path(), + mount_point.path(), + Path::new(DEFAULT_ESP_MOUNT_POINT_PATH), + AbVolumeSelection::VolumeB, + ); + + assert!(result.is_err()); + assert!( + result.unwrap_err().to_string().contains("verity-b.addon.efi"), + "Error should mention the missing template" + ); + } + + /// Creates the staged addon dir when templates exist but no addon dir was staged. + #[test] + fn test_activate_verity_addon_creates_addon_dir() { + let image_esp = tempdir().unwrap(); + setup_image_with_verity_templates(image_esp.path()); + + let mount_point = tempdir().unwrap(); + prepare_esp_for_uki(mount_point.path(), Path::new(DEFAULT_ESP_MOUNT_POINT_PATH)) + .unwrap(); + + // Do NOT pre-create the staged addon dir + activate_verity_addon_for_target_volume( + image_esp.path(), + mount_point.path(), + Path::new(DEFAULT_ESP_MOUNT_POINT_PATH), + AbVolumeSelection::VolumeB, + ) + .unwrap(); + + let staged_addon_dir = join_relative(mount_point.path(), DEFAULT_ESP_MOUNT_POINT_PATH) + .join(UKI_DIRECTORY) + .join(TMP_UKI_ADDON_DIR_NAME); + assert!(staged_addon_dir.join(VERITY_ADDON_FILENAME).exists()); + assert_eq!( + fs::read(staged_addon_dir.join(VERITY_ADDON_FILENAME)).unwrap(), + b"verity-b-content" + ); + } + + /// Other addons in the staged dir are preserved when activating verity addon. + #[test] + fn test_activate_verity_addon_preserves_other_addons() { + let image_esp = tempdir().unwrap(); + setup_image_with_verity_templates(image_esp.path()); + + let mount_point = tempdir().unwrap(); + prepare_esp_for_uki(mount_point.path(), Path::new(DEFAULT_ESP_MOUNT_POINT_PATH)) + .unwrap(); + + let staged_addon_dir = join_relative(mount_point.path(), DEFAULT_ESP_MOUNT_POINT_PATH) + .join(UKI_DIRECTORY) + .join(TMP_UKI_ADDON_DIR_NAME); + fs::create_dir_all(&staged_addon_dir).unwrap(); + // Pre-existing addon that should not be touched + fs::write( + staged_addon_dir.join("firstboot.addon.efi"), + b"firstboot-data", + ) + .unwrap(); + + activate_verity_addon_for_target_volume( + image_esp.path(), + mount_point.path(), + Path::new(DEFAULT_ESP_MOUNT_POINT_PATH), + AbVolumeSelection::VolumeA, + ) + .unwrap(); + + // Verity addon should be activated + assert_eq!( + fs::read(staged_addon_dir.join(VERITY_ADDON_FILENAME)).unwrap(), + b"verity-a-content" + ); + // Other addon should be untouched + assert_eq!( + fs::read(staged_addon_dir.join("firstboot.addon.efi")).unwrap(), + b"firstboot-data" + ); + } } diff --git a/crates/trident/src/subsystems/esp.rs b/crates/trident/src/subsystems/esp.rs index e3073aa8b..36879aaaf 100644 --- a/crates/trident/src/subsystems/esp.rs +++ b/crates/trident/src/subsystems/esp.rs @@ -290,6 +290,21 @@ fn copy_file_artifacts( // Copy the UKI from the image into the ESP directory uki::stage_uki_on_esp(temp_mount_dir, mount_point, &ctx.esp_mount_path)?; + + // For ACL A/B images, activate the verity addon matching the target slot. + // The image ships with slot A's addon active; this swaps it when updating + // to slot B (or confirms slot A for clean installs). Non-ACL images have + // no template directory and this is a no-op. + if ctx.image_distro().is_acl() { + if let Some(target_volume) = ctx.get_ab_update_volume() { + uki::activate_verity_addon_for_target_volume( + temp_mount_dir, + mount_point, + &ctx.esp_mount_path, + target_volume, + )?; + } + } } else { // In non-UKI mode, bail if grub_noprefix.efi is not found in the image. ensure!( From 63009d35f1213c99b743134b3e90da908d82876e Mon Sep 17 00:00:00 2001 From: bfjelds Date: Mon, 1 Jun 2026 16:40:54 -0700 Subject: [PATCH 02/22] Skip A/B duplicate FS UUID check for ACL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ACL uses identical FS UUIDs across A/B slots by design — partitions are distinguished by PARTUUID instead. The within-image uniqueness check is unaffected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/trident/src/subsystems/storage/osimage.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/crates/trident/src/subsystems/storage/osimage.rs b/crates/trident/src/subsystems/storage/osimage.rs index e699f00ac..4279bdd76 100644 --- a/crates/trident/src/subsystems/storage/osimage.rs +++ b/crates/trident/src/subsystems/storage/osimage.rs @@ -189,6 +189,9 @@ fn validate_filesystems(os_image: &OsImage, ctx: &EngineContext) -> Result<(), T /// Validates that all filesystems within an OS image have unique FS UUIDs. Additionally, validates /// that A/B volume pairs have distinct FS UUIDs. +/// +/// The A/B cross-check is skipped for ACL, which uses identical FS UUIDs across A/B slots by +/// design (partitions are distinguished by PARTUUID instead). fn validate_filesystem_uniqueness( os_image: &OsImage, ctx: &EngineContext, @@ -204,8 +207,9 @@ fn validate_filesystem_uniqueness( } } - // For A/B Update, check that no A/B volumes share filesystem UUIDs - if ctx.servicing_type == ServicingType::AbUpdate { + // For A/B Update, check that no A/B volumes share filesystem UUIDs. + // ACL uses the same FS UUID for both A/B slots — partitions are identified by PARTUUID. + if ctx.servicing_type == ServicingType::AbUpdate && !ctx.image_distro().is_acl() { if let Some(ab) = &ctx.spec.storage.ab_update { for pair in ab.volume_pairs.iter() { if let Some(mp_info) = ctx.spec.storage.device_id_to_mount_point_info(&pair.id) { From c7da44d99da5f59bd9b4a1958e059d3f178fc86d Mon Sep 17 00:00:00 2001 From: bfjelds Date: Mon, 1 Jun 2026 17:03:24 -0700 Subject: [PATCH 03/22] Fix rustfmt formatting in verity addon tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/trident/src/engine/boot/uki.rs | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/crates/trident/src/engine/boot/uki.rs b/crates/trident/src/engine/boot/uki.rs index 7f4b47a58..4b8414b98 100644 --- a/crates/trident/src/engine/boot/uki.rs +++ b/crates/trident/src/engine/boot/uki.rs @@ -937,9 +937,7 @@ mod tests { // ── activate_verity_addon_for_target_volume tests ────────────────────── /// Helper: creates a mock image ESP with verity addon templates. - fn setup_image_with_verity_templates( - image_esp: &Path, - ) -> (PathBuf, PathBuf) { + fn setup_image_with_verity_templates(image_esp: &Path) -> (PathBuf, PathBuf) { let template_dir = image_esp.join(VERITY_ADDON_TEMPLATES_DIR); fs::create_dir_all(&template_dir).unwrap(); let a_path = template_dir.join("verity-a.addon.efi"); @@ -956,8 +954,7 @@ mod tests { setup_image_with_verity_templates(image_esp.path()); let mount_point = tempdir().unwrap(); - prepare_esp_for_uki(mount_point.path(), Path::new(DEFAULT_ESP_MOUNT_POINT_PATH)) - .unwrap(); + prepare_esp_for_uki(mount_point.path(), Path::new(DEFAULT_ESP_MOUNT_POINT_PATH)).unwrap(); // Create the staged addon dir as stage_uki_on_esp would let staged_addon_dir = join_relative(mount_point.path(), DEFAULT_ESP_MOUNT_POINT_PATH) @@ -985,8 +982,7 @@ mod tests { setup_image_with_verity_templates(image_esp.path()); let mount_point = tempdir().unwrap(); - prepare_esp_for_uki(mount_point.path(), Path::new(DEFAULT_ESP_MOUNT_POINT_PATH)) - .unwrap(); + prepare_esp_for_uki(mount_point.path(), Path::new(DEFAULT_ESP_MOUNT_POINT_PATH)).unwrap(); let staged_addon_dir = join_relative(mount_point.path(), DEFAULT_ESP_MOUNT_POINT_PATH) .join(UKI_DIRECTORY) @@ -1013,8 +1009,7 @@ mod tests { // No acl/uki-addons/ directory let mount_point = tempdir().unwrap(); - prepare_esp_for_uki(mount_point.path(), Path::new(DEFAULT_ESP_MOUNT_POINT_PATH)) - .unwrap(); + prepare_esp_for_uki(mount_point.path(), Path::new(DEFAULT_ESP_MOUNT_POINT_PATH)).unwrap(); // Should succeed silently activate_verity_addon_for_target_volume( @@ -1042,8 +1037,7 @@ mod tests { fs::write(template_dir.join("verity-a.addon.efi"), b"a-content").unwrap(); let mount_point = tempdir().unwrap(); - prepare_esp_for_uki(mount_point.path(), Path::new(DEFAULT_ESP_MOUNT_POINT_PATH)) - .unwrap(); + prepare_esp_for_uki(mount_point.path(), Path::new(DEFAULT_ESP_MOUNT_POINT_PATH)).unwrap(); let result = activate_verity_addon_for_target_volume( image_esp.path(), @@ -1054,7 +1048,10 @@ mod tests { assert!(result.is_err()); assert!( - result.unwrap_err().to_string().contains("verity-b.addon.efi"), + result + .unwrap_err() + .to_string() + .contains("verity-b.addon.efi"), "Error should mention the missing template" ); } @@ -1066,8 +1063,7 @@ mod tests { setup_image_with_verity_templates(image_esp.path()); let mount_point = tempdir().unwrap(); - prepare_esp_for_uki(mount_point.path(), Path::new(DEFAULT_ESP_MOUNT_POINT_PATH)) - .unwrap(); + prepare_esp_for_uki(mount_point.path(), Path::new(DEFAULT_ESP_MOUNT_POINT_PATH)).unwrap(); // Do NOT pre-create the staged addon dir activate_verity_addon_for_target_volume( @@ -1095,8 +1091,7 @@ mod tests { setup_image_with_verity_templates(image_esp.path()); let mount_point = tempdir().unwrap(); - prepare_esp_for_uki(mount_point.path(), Path::new(DEFAULT_ESP_MOUNT_POINT_PATH)) - .unwrap(); + prepare_esp_for_uki(mount_point.path(), Path::new(DEFAULT_ESP_MOUNT_POINT_PATH)).unwrap(); let staged_addon_dir = join_relative(mount_point.path(), DEFAULT_ESP_MOUNT_POINT_PATH) .join(UKI_DIRECTORY) From bd612ecb80c6483ecec1c1c0e45b9e71ca9cc0b3 Mon Sep 17 00:00:00 2001 From: bfjelds Date: Mon, 1 Jun 2026 18:31:54 -0700 Subject: [PATCH 04/22] Include UKI addons in findUkiEntries COSI metadata Scan each UKI's .extra.d/ directory for *.addon.efi files and extract their .cmdline PE sections. Addons are stored as a new field on the boot entry so the COSI metadata captures the full effective cmdline (main UKI + addons). Both Go (mkcosi) and Rust (metadata deserialization) updated. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/trident/src/osimage/cosi/metadata.rs | 13 ++++++ tools/cmd/mkcosi/generator/generator.go | 46 +++++++++++++++++++++ tools/cmd/mkcosi/metadata/metadata.go | 6 +++ 3 files changed, 65 insertions(+) diff --git a/crates/trident/src/osimage/cosi/metadata.rs b/crates/trident/src/osimage/cosi/metadata.rs index dad49ff56..c5ca32156 100644 --- a/crates/trident/src/osimage/cosi/metadata.rs +++ b/crates/trident/src/osimage/cosi/metadata.rs @@ -360,6 +360,19 @@ pub(crate) struct BootloaderEntry { #[allow(dead_code)] pub cmdline: String, + + #[allow(dead_code)] + #[serde(default)] + pub addons: Vec, +} + +#[derive(Debug, Deserialize, Clone, Eq, PartialEq)] +pub(crate) struct UkiAddon { + #[allow(dead_code)] + pub path: String, + + #[allow(dead_code)] + pub cmdline: String, } #[derive(Debug, Deserialize, Clone, Eq, PartialEq, Display)] diff --git a/tools/cmd/mkcosi/generator/generator.go b/tools/cmd/mkcosi/generator/generator.go index ac50b49fe..418271765 100644 --- a/tools/cmd/mkcosi/generator/generator.go +++ b/tools/cmd/mkcosi/generator/generator.go @@ -1085,17 +1085,63 @@ func findUkiEntries(espMountPath string, espMountPoint string) []metadata.System continue } + // Scan for UKI addons in .extra.d/ + addons := findUkiAddons(ukiDir, name, espMountPoint) + entries = append(entries, metadata.SystemDBootEntry{ Type: metadata.SystemDBootEntryTypeUkiStandalone, Path: absFsPath, Kernel: kernel, Cmdline: cmdline, + Addons: addons, }) } return entries } +// findUkiAddons scans the .extra.d/ directory for UKI addon files +// (*.addon.efi) and extracts their .cmdline PE sections. systemd-stub loads +// these addons at boot and appends their cmdline args to the UKI's own cmdline. +func findUkiAddons(ukiDir string, ukiName string, espMountPoint string) []metadata.UkiAddon { + addonDirName := ukiName + ".extra.d" + addonHostDir := filepath.Join(ukiDir, addonDirName) + + addonEntries, err := os.ReadDir(addonHostDir) + if err != nil { + // No .extra.d directory is normal for UKIs without addons + return nil + } + + var addons []metadata.UkiAddon + for _, ae := range addonEntries { + if ae.IsDir() { + continue + } + aName := ae.Name() + if !strings.HasSuffix(strings.ToLower(aName), ".addon.efi") { + continue + } + + addonHostPath := filepath.Join(addonHostDir, aName) + addonFsPath := filepath.Join(espMountPoint, "EFI", "Linux", addonDirName, aName) + + cmdline := extractUkiSection(addonHostPath, ".cmdline") + if cmdline == "" { + log.WithField("path", addonFsPath).Debug("Skipping addon with no .cmdline section") + continue + } + + log.WithField("path", addonFsPath).Debug("Found UKI addon") + addons = append(addons, metadata.UkiAddon{ + Path: addonFsPath, + Cmdline: cmdline, + }) + } + + return addons +} + // extractUkiSection extracts a named PE section from a UKI .efi file using // objcopy. The UKI is first copied to a writable temp directory because objcopy // creates a temporary file next to the input, which fails on read-only mounts. diff --git a/tools/cmd/mkcosi/metadata/metadata.go b/tools/cmd/mkcosi/metadata/metadata.go index 03a4f9f2e..b24639169 100644 --- a/tools/cmd/mkcosi/metadata/metadata.go +++ b/tools/cmd/mkcosi/metadata/metadata.go @@ -65,6 +65,12 @@ type SystemDBootEntry struct { Path string `json:"path"` Cmdline string `json:"cmdline"` Kernel string `json:"kernel"` + Addons []UkiAddon `json:"addons,omitempty"` +} + +type UkiAddon struct { + Path string `json:"path"` + Cmdline string `json:"cmdline"` } type Compression struct { From d722c07870c259d1f1077e3f53386516eeb3c911 Mon Sep 17 00:00:00 2001 From: bfjelds Date: Mon, 1 Jun 2026 21:22:40 -0700 Subject: [PATCH 05/22] Search UKI addon cmdlines for usrhash= parameter With PARTUUID-based verity addons, usrhash= moved from the main UKI cmdline to the verity addon cmdline. Update extractUsrhashFromUKIEntries to also search addon cmdlines when looking for the root hash. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tools/cmd/mkcosi/generator/cih.go | 13 ++++++++++--- tools/cmd/mkcosi/generator/generator.go | 2 +- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/tools/cmd/mkcosi/generator/cih.go b/tools/cmd/mkcosi/generator/cih.go index d6681d28f..16089dc25 100644 --- a/tools/cmd/mkcosi/generator/cih.go +++ b/tools/cmd/mkcosi/generator/cih.go @@ -268,9 +268,9 @@ func populateCIHFilesystemMetadata(cosiMeta *metadata.MetadataJson, partInfos [] return nil } -// extractUsrhashFromUKIEntries searches the UKI boot entries for a -// "usrhash=" kernel command-line parameter and returns the hash value. -// Returns an empty string if not found. +// extractUsrhashFromUKIEntries searches the UKI boot entries and their addons +// for a "usrhash=" kernel command-line parameter and returns the hash +// value. Returns an empty string if not found. func extractUsrhashFromUKIEntries(entries []metadata.SystemDBootEntry) string { for _, entry := range entries { for _, field := range strings.Fields(entry.Cmdline) { @@ -278,6 +278,13 @@ func extractUsrhashFromUKIEntries(entries []metadata.SystemDBootEntry) string { return after } } + for _, addon := range entry.Addons { + for _, field := range strings.Fields(addon.Cmdline) { + if after, found := strings.CutPrefix(field, "usrhash="); found { + return after + } + } + } } return "" } diff --git a/tools/cmd/mkcosi/generator/generator.go b/tools/cmd/mkcosi/generator/generator.go index 418271765..289b9c4b4 100644 --- a/tools/cmd/mkcosi/generator/generator.go +++ b/tools/cmd/mkcosi/generator/generator.go @@ -1132,7 +1132,7 @@ func findUkiAddons(ukiDir string, ukiName string, espMountPoint string) []metada continue } - log.WithField("path", addonFsPath).Debug("Found UKI addon") + log.WithField("path", addonFsPath).Debugf("Found UKI addon: %s", cmdline) addons = append(addons, metadata.UkiAddon{ Path: addonFsPath, Cmdline: cmdline, From 420556843857ed803ee6e21e5597560d330411b6 Mon Sep 17 00:00:00 2001 From: bfjelds Date: Wed, 3 Jun 2026 14:47:35 -0700 Subject: [PATCH 06/22] Handle ACL BTRFS UUID collision during A/B update mount When staging an A/B update on ACL (Azure Container Linux) UKI images, the COSI image may share BTRFS filesystem UUIDs with the active OS. BTRFS maintains a kernel-global UUID registry and refuses to mount a filesystem whose UUID is already registered by another mounted device, causing the staging verity device mount to fail. This change detects the UUID collision by checking the well-known ACL USR-A/USR-B partition UUIDs (by PARTUUID) before the mount loop. When a collision is detected, it bind-mounts the active /usr into the newroot instead of attempting to mount the staging verity device. This is safe because: - USR is verity-protected and read-only - Matching UUIDs means identical filesystem content - The chroot only reads from /usr during provisioning - After reboot, initramfs sets up the correct verity device normally Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/trident/src/engine/newroot.rs | 94 +++++++++++++++++++++++++++- 1 file changed, 93 insertions(+), 1 deletion(-) diff --git a/crates/trident/src/engine/newroot.rs b/crates/trident/src/engine/newroot.rs index e21d1fa96..a8ceed9d9 100644 --- a/crates/trident/src/engine/newroot.rs +++ b/crates/trident/src/engine/newroot.rs @@ -11,7 +11,10 @@ use log::{debug, error, trace, warn}; use sys_mount::{MountBuilder, MountFlags}; use osutils::{files, filesystems::MountFileSystemType, findmnt::FindMnt, lsblk, mount, path}; -use sysdefs::filesystems::{KernelFilesystemType, RealFilesystemType}; +use sysdefs::{ + filesystems::{KernelFilesystemType, RealFilesystemType}, + osuuid::OsUuid, +}; use trident_api::{ config::{FileSystem, HostConfiguration}, constants::{ @@ -164,6 +167,9 @@ impl NewrootMount { } } + // Check for ACL BTRFS UUID collision before mounting. + let acl_collision_uuid = detect_acl_btrfs_uuid_collision(update_volume); + // Mount all block devices in the newroot mount_points_map(host_config) .iter() @@ -202,6 +208,38 @@ impl NewrootMount { let fs_type = block_device.fstype.and_then(|fs_type| KernelFilesystemType::from(fs_type.as_str()).try_as_real()); + // ACL-specific: if the staging device has a BTRFS filesystem UUID that + // collides with the active USR partition, bind-mount from the host's + // /usr instead. The verity-protected filesystem is read-only and the + // content is identical when UUIDs match, so the bind mount provides + // equivalent content for chroot provisioning. + if let Some(ref collision_uuid) = acl_collision_uuid { + if fs_type == Some(RealFilesystemType::Btrfs) + && block_device.fsuuid.as_ref() == Some(collision_uuid) + { + let active_usr = Path::new("/usr"); + warn!( + "Block device '{}' has BTRFS filesystem UUID '{}' which collides \ + with the active ACL USR partition. Bind-mounting '{}' to '{}' instead.", + target_id, + collision_uuid, + active_usr.display(), + target_path.display() + ); + do_bind_mount(active_usr, &target_path, MountFlags::RDONLY) + .with_context(|| { + format!( + "Failed to bind mount '{}' to '{}' \ + for ACL BTRFS UUID collision workaround", + active_usr.display(), + target_path.display(), + ) + })?; + self.add_mount(target_path.clone()); + return Ok(()); + } + } + // If a filesystem is of type NTFS and the device is already mounted, need to use a // private bind mount instead, b/c NTFS doesn't support multiple mounts. match (should_be_bind_mounted(fs_type), block_device.mountpoint) { @@ -339,6 +377,60 @@ fn should_be_bind_mounted(fs_type: Option) -> bool { } } +// ACL (Azure Container Linux) UKI disk layout defines fixed PARTUUIDs for +// the USR A/B data partitions. These are from acl-scripts disk_layout_uki.json. +const ACL_USR_A_PARTUUID: &str = "7130c94a-213a-4e5a-8e26-6cce9662f132"; +const ACL_USR_B_PARTUUID: &str = "e03dd35c-7c2d-4a47-b3fe-27f15780a57c"; + +/// Detects a BTRFS filesystem UUID collision on ACL's USR A/B partitions. +/// +/// BTRFS maintains a kernel-global UUID registry and refuses to mount a filesystem +/// whose UUID is already registered by another mounted device. During A/B updates +/// where the COSI image shares filesystem UUIDs with the active OS, the staging +/// verity device cannot be mounted. +/// +/// This function checks whether the active and update USR partitions (identified by +/// their well-known ACL PARTUUIDs) have the same BTRFS filesystem UUID. If so, it +/// returns the colliding UUID so the caller can substitute a bind mount from the +/// active `/usr`. +/// +/// Returns `None` if: +/// - The system is not ACL (PARTUUIDs not found) +/// - The partitions don't have BTRFS filesystems +/// - The filesystem UUIDs are different (no collision) +fn detect_acl_btrfs_uuid_collision(update_volume: AbVolumeSelection) -> Option { + let (active_partuuid, update_partuuid) = match update_volume { + AbVolumeSelection::VolumeA => (ACL_USR_B_PARTUUID, ACL_USR_A_PARTUUID), + AbVolumeSelection::VolumeB => (ACL_USR_A_PARTUUID, ACL_USR_B_PARTUUID), + }; + + let active_path = format!("/dev/disk/by-partuuid/{active_partuuid}"); + let update_path = format!("/dev/disk/by-partuuid/{update_partuuid}"); + + let active_dev = lsblk::get(&active_path).ok()?; + let update_dev = lsblk::get(&update_path).ok()?; + + if active_dev.fstype.as_deref()? != "btrfs" { + return None; + } + if update_dev.fstype.as_deref()? != "btrfs" { + return None; + } + + let active_uuid = active_dev.fsuuid?; + let update_uuid = update_dev.fsuuid?; + + if active_uuid == update_uuid { + debug!( + "ACL BTRFS UUID collision detected: active and update USR partitions \ + share filesystem UUID '{active_uuid}'" + ); + Some(active_uuid) + } else { + None + } +} + /// Returns an ordered map of mount points to their corresponding FileSystem objects. fn mount_points_map(host_config: &HostConfiguration) -> BTreeMap<&Path, &FileSystem> { host_config From 596a696b1bbf9a9fa266a8dbd3e78f3663e5fc69 Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Wed, 3 Jun 2026 22:52:58 +0000 Subject: [PATCH 07/22] Verify verity root hash before ACL BTRFS UUID bind-mount When the bind-mount workaround activates for ACL BTRFS UUID collisions, compare the staging USR verity root hash (from COSI metadata) against the active USR root hash (from /proc/cmdline usrhash= parameter) to cryptographically prove the filesystems are byte-identical. If the staging hash is available but the active hash cannot be read or does not match, the bind-mount is refused and the normal mount path proceeds (which will fail with the BTRFS UUID error, as expected for genuinely different content). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- atomic_write.patch | 235 +++++++++++++++++++++ crates/trident/src/engine/ab_update.rs | 14 +- crates/trident/src/engine/clean_install.rs | 2 + crates/trident/src/engine/newroot.rs | 92 ++++++-- mic-baremetal-to-qemu-guest.yaml | 18 ++ 5 files changed, 342 insertions(+), 19 deletions(-) create mode 100644 atomic_write.patch create mode 100644 mic-baremetal-to-qemu-guest.yaml diff --git a/atomic_write.patch b/atomic_write.patch new file mode 100644 index 000000000..63f61c4a0 --- /dev/null +++ b/atomic_write.patch @@ -0,0 +1,235 @@ +diff --git a/crates/osmodifier/src/modules.rs b/crates/osmodifier/src/modules.rs +index ce46c95f..e3174c61 100644 +--- a/crates/osmodifier/src/modules.rs ++++ b/crates/osmodifier/src/modules.rs +@@ -12,6 +12,8 @@ use trident_api::config::{LoadMode, Module}; + + use crate::OsModifierContext; + ++use osutils::files::atomic_write_file; ++ + const MODULES_LOAD_DIR: &str = "/etc/modules-load.d"; + const MODULES_LOAD_CONF: &str = "/etc/modules-load.d/modules-load.conf"; + const MODPROBE_DIR: &str = "/etc/modprobe.d"; +@@ -184,8 +186,7 @@ fn write_config(path: &std::path::Path, lines: &[String]) -> Result<(), Error> { + s.push('\n'); + s + }; +- fs::write(path, &content) +- .with_context(|| format!("Failed to write config to '{}'", path.display())) ++ atomic_write_file(path, &content) + } + + #[cfg_attr(not(test), allow(unused_imports, dead_code))] +diff --git a/crates/osmodifier/src/users.rs b/crates/osmodifier/src/users.rs +index 68fda4ab..e37619d9 100644 +--- a/crates/osmodifier/src/users.rs ++++ b/crates/osmodifier/src/users.rs +@@ -14,6 +14,8 @@ use crate::{ + OsModifierContext, + }; + ++use osutils::files::atomic_write_file; ++ + // Shadow file field indices (0-based, colon-delimited). + const SHADOW_FIELD_PASSWORD: usize = 1; + const SHADOW_FIELD_LAST_CHANGE: usize = 2; +@@ -576,71 +578,4 @@ fn set_startup_command(ctx: &OsModifierContext, username: &str, cmd: &str) -> Re + } + + atomic_write_file(&passwd_path, &result) +-} +- +-/// Atomically write a file by writing to a temp file and renaming. +-/// This prevents corruption from crashes mid-write. +-/// +-/// Preserves permissions and uid/gid ownership from the original file. +-/// Note: SELinux labels and extended attributes are not preserved because +-/// osmodifier runs inside the target root before SELinux enforcement. +-fn atomic_write_file(path: &std::path::Path, content: &str) -> Result<(), Error> { +- use std::io::Write as IoWrite; +- use std::os::unix::fs::MetadataExt; +- +- let parent = path.parent().context("Cannot determine parent directory")?; +- +- let mut tmp = tempfile::NamedTempFile::new_in(parent) +- .with_context(|| format!("Failed to create temp file in '{}'", parent.display()))?; +- +- tmp.write_all(content.as_bytes()) +- .with_context(|| format!("Failed to write temp file for '{}'", path.display()))?; +- +- tmp.flush() +- .with_context(|| format!("Failed to flush temp file for '{}'", path.display()))?; +- +- // fsync the temp file before rename to ensure data is on disk. Without +- // this, a power loss between rename and dirty-page flush could leave the +- // file zero-length (e.g., /etc/shadow → locked out of all accounts). +- tmp.as_file() +- .sync_all() +- .with_context(|| format!("Failed to fsync temp file for '{}'", path.display()))?; +- +- // Preserve ownership and permissions from the original file if it exists. +- // Ownership must be set before permissions because chown can clear +- // setuid/setgid bits. +- if let Ok(metadata) = fs::metadata(path) { +- use std::os::fd::AsFd; +- nix::unistd::fchown( +- tmp.as_file().as_fd(), +- Some(nix::unistd::Uid::from_raw(metadata.uid())), +- Some(nix::unistd::Gid::from_raw(metadata.gid())), +- ) +- .with_context(|| { +- format!( +- "Failed to set ownership on temp file for '{}'", +- path.display() +- ) +- })?; +- +- fs::set_permissions(tmp.path(), metadata.permissions()).with_context(|| { +- format!( +- "Failed to set permissions on temp file for '{}'", +- path.display() +- ) +- })?; +- } +- +- tmp.persist(path) +- .with_context(|| format!("Failed to atomically replace '{}'", path.display()))?; +- +- // Sync parent directory to ensure the rename (directory entry update) is +- // durable. Without this, the old file could reappear after power loss. +- if let Some(parent) = path.parent() { +- if let Ok(dir) = std::fs::File::open(parent) { +- let _ = dir.sync_all(); +- } +- } +- +- Ok(()) +-} ++} +\ No newline at end of file +diff --git a/crates/osutils/src/files.rs b/crates/osutils/src/files.rs +index 5a3a3950..ae52f7f1 100644 +--- a/crates/osutils/src/files.rs ++++ b/crates/osutils/src/files.rs +@@ -192,6 +192,71 @@ where + }) + } + ++/// Atomically replace `path` with `content`. ++/// ++/// Writes to a temp file in the same directory, fsyncs, preserves ownership ++/// and permissions from the original file (if it exists), then renames. This ++/// guarantees that readers never see a partial write. ++pub fn atomic_write_file(path: &Path, content: &str) -> Result<(), Error> { ++ use std::os::unix::fs::MetadataExt; ++ ++ let parent = path.parent().context("Cannot determine parent directory")?; ++ ++ let mut tmp = tempfile::NamedTempFile::new_in(parent) ++ .with_context(|| format!("Failed to create temp file in '{}'", parent.display()))?; ++ ++ tmp.write_all(content.as_bytes()) ++ .with_context(|| format!("Failed to write temp file for '{}'", path.display()))?; ++ ++ tmp.flush() ++ .with_context(|| format!("Failed to flush temp file for '{}'", path.display()))?; ++ ++ // fsync the temp file before rename to ensure data is on disk. Without ++ // this, a power loss between rename and dirty-page flush could leave the ++ // file zero-length. ++ tmp.as_file() ++ .sync_all() ++ .with_context(|| format!("Failed to fsync temp file for '{}'", path.display()))?; ++ ++ // Preserve ownership and permissions from the original file if it exists. ++ // Ownership must be set before permissions because chown can clear ++ // setuid/setgid bits. ++ if let Ok(metadata) = fs::metadata(path) { ++ use std::os::fd::AsFd; ++ nix::unistd::fchown( ++ tmp.as_file().as_fd(), ++ Some(nix::unistd::Uid::from_raw(metadata.uid())), ++ Some(nix::unistd::Gid::from_raw(metadata.gid())), ++ ) ++ .with_context(|| { ++ format!( ++ "Failed to set ownership on temp file for '{}'", ++ path.display() ++ ) ++ })?; ++ ++ fs::set_permissions(tmp.path(), metadata.permissions()).with_context(|| { ++ format!( ++ "Failed to set permissions on temp file for '{}'", ++ path.display() ++ ) ++ })?; ++ } ++ ++ tmp.persist(path) ++ .with_context(|| format!("Failed to atomically replace '{}'", path.display()))?; ++ ++ // Sync parent directory to ensure the rename (directory entry update) is ++ // durable. Without this, the old file could reappear after power loss. ++ if let Some(parent) = path.parent() { ++ if let Ok(dir) = fs::File::open(parent) { ++ let _ = dir.sync_all(); ++ } ++ } ++ ++ Ok(()) ++} ++ + #[cfg(test)] + mod tests { + use super::*; +@@ -346,4 +411,48 @@ mod tests { + ); + }); + } ++ ++ #[test] ++ fn test_atomic_write_creates_new_file() { ++ let tmp = tempdir().unwrap(); ++ let path = tmp.path().join("new_file.conf"); ++ ++ atomic_write_file(&path, "hello\n").unwrap(); ++ ++ assert_eq!(fs::read_to_string(&path).unwrap(), "hello\n"); ++ } ++ ++ #[test] ++ fn test_atomic_write_replaces_existing_file() { ++ let tmp = tempdir().unwrap(); ++ let path = tmp.path().join("existing.conf"); ++ fs::write(&path, "old content\n").unwrap(); ++ ++ atomic_write_file(&path, "new content\n").unwrap(); ++ ++ assert_eq!(fs::read_to_string(&path).unwrap(), "new content\n"); ++ } ++ ++ #[test] ++ fn test_atomic_write_preserves_permissions() { ++ let tmp = tempdir().unwrap(); ++ let path = tmp.path().join("perms.conf"); ++ fs::write(&path, "original\n").unwrap(); ++ fs::set_permissions(&path, Permissions::from_mode(0o640)).unwrap(); ++ ++ atomic_write_file(&path, "updated\n").unwrap(); ++ ++ let mode = fs::metadata(&path).unwrap().permissions().mode() & 0o777; ++ assert_eq!(mode, 0o640, "Expected mode 0640, got {mode:04o}"); ++ } ++ ++ #[test] ++ fn test_atomic_write_empty_content() { ++ let tmp = tempdir().unwrap(); ++ let path = tmp.path().join("empty.conf"); ++ ++ atomic_write_file(&path, "").unwrap(); ++ ++ assert_eq!(fs::read_to_string(&path).unwrap(), ""); ++ } + } diff --git a/crates/trident/src/engine/ab_update.rs b/crates/trident/src/engine/ab_update.rs index 8b9c9edcd..53cf3ca11 100644 --- a/crates/trident/src/engine/ab_update.rs +++ b/crates/trident/src/engine/ab_update.rs @@ -1,4 +1,4 @@ -use std::{path::PathBuf, time::Instant}; +use std::{path::{Path, PathBuf}, time::Instant}; use log::{debug, info, warn}; @@ -59,6 +59,17 @@ pub(super) fn stage_update( verity::stop_trident_servicing_devices(&ctx.spec).structured(ServicingError::CleanupVerity)?; storage::initialize_block_devices(&ctx)?; + + // Extract the staging USR verity root hash from the COSI image metadata. + // This is used to cryptographically verify that the active and staging USR + // partitions have identical content before allowing a bind-mount workaround + // for the BTRFS kernel UUID collision on ACL. + let staging_usr_roothash = ctx.image.as_ref().and_then(|img| { + img.filesystems() + .find(|fs| fs.mount_point == Path::new("/usr")) + .and_then(|fs| fs.verity.as_ref().map(|v| v.roothash.clone())) + }); + let newroot_mount = NewrootMount::create_and_mount( &ctx.spec, &ctx.partition_paths, @@ -66,6 +77,7 @@ pub(super) fn stage_update( .structured(InternalError::Internal( "No update volume despite there being an A/B update in progress", ))?, + staging_usr_roothash.as_deref(), )?; engine::provision(subsystems, &ctx, newroot_mount.path())?; diff --git a/crates/trident/src/engine/clean_install.rs b/crates/trident/src/engine/clean_install.rs index 765e5852a..64892363a 100644 --- a/crates/trident/src/engine/clean_install.rs +++ b/crates/trident/src/engine/clean_install.rs @@ -213,6 +213,7 @@ fn stage_clean_install( host_config, &ctx.partition_paths, AbVolumeSelection::VolumeA, + None, )?; ctx.install_index = install_index::next_install_index(newroot_mount.path(), ctx.esp_mount_path.as_path())?; @@ -302,6 +303,7 @@ pub(crate) fn finalize_clean_install( .structured(InternalError::Internal( "No update volume despite there being a clean install in progress", ))?, + None, )?, }; diff --git a/crates/trident/src/engine/newroot.rs b/crates/trident/src/engine/newroot.rs index a8ceed9d9..54d2541be 100644 --- a/crates/trident/src/engine/newroot.rs +++ b/crates/trident/src/engine/newroot.rs @@ -62,6 +62,7 @@ impl NewrootMount { host_config: &HostConfiguration, partition_paths: &BTreeMap, update_volume: AbVolumeSelection, + staging_usr_roothash: Option<&str>, ) -> Result { // Get the path where the newroot should be mounted let new_root_path = get_new_root_path(); @@ -76,7 +77,7 @@ impl NewrootMount { let mut newroot_mount = NewrootMount::new(new_root_path); newroot_mount - .mount_newroot_partitions(host_config, partition_paths, update_volume) + .mount_newroot_partitions(host_config, partition_paths, update_volume, staging_usr_roothash) .message("Failed to mount all partitions in newroot")?; // Mount tmpfs for /tmp and /run @@ -139,6 +140,7 @@ impl NewrootMount { host_config: &HostConfiguration, partition_paths: &BTreeMap, update_volume: AbVolumeSelection, + staging_usr_roothash: Option<&str>, ) -> Result<(), TridentError> { let mut block_device_paths = partition_paths.clone(); @@ -168,7 +170,7 @@ impl NewrootMount { } // Check for ACL BTRFS UUID collision before mounting. - let acl_collision_uuid = detect_acl_btrfs_uuid_collision(update_volume); + let acl_collision_uuid = detect_acl_btrfs_uuid_collision(update_volume, staging_usr_roothash); // Mount all block devices in the newroot mount_points_map(host_config) @@ -398,7 +400,10 @@ const ACL_USR_B_PARTUUID: &str = "e03dd35c-7c2d-4a47-b3fe-27f15780a57c"; /// - The system is not ACL (PARTUUIDs not found) /// - The partitions don't have BTRFS filesystems /// - The filesystem UUIDs are different (no collision) -fn detect_acl_btrfs_uuid_collision(update_volume: AbVolumeSelection) -> Option { +fn detect_acl_btrfs_uuid_collision( + update_volume: AbVolumeSelection, + staging_usr_roothash: Option<&str>, +) -> Option { let (active_partuuid, update_partuuid) = match update_volume { AbVolumeSelection::VolumeA => (ACL_USR_B_PARTUUID, ACL_USR_A_PARTUUID), AbVolumeSelection::VolumeB => (ACL_USR_A_PARTUUID, ACL_USR_B_PARTUUID), @@ -420,15 +425,63 @@ fn detect_acl_btrfs_uuid_collision(update_volume: AbVolumeSelection) -> Option { + let staging_normalized = staging_hash.trim().to_lowercase(); + let active_normalized = active_hash.trim().to_lowercase(); + if staging_normalized == active_normalized { + debug!( + "Verity root hash verification passed: active and staging USR \ + partitions have matching root hash ({}...)", + &staging_normalized[..staging_normalized.len().min(16)] + ); + } else { + warn!( + "Verity root hash mismatch: active USR has '{}...', staging has '{}...'. \ + Refusing bind-mount despite UUID collision.", + &active_normalized[..active_normalized.len().min(16)], + &staging_normalized[..staging_normalized.len().min(16)] + ); + return None; + } + } + None => { + warn!( + "Cannot read active USR verity root hash from /proc/cmdline. \ + Refusing bind-mount despite UUID collision." + ); + return None; + } + } + } + + Some(active_uuid) +} + +/// Reads the active USR verity root hash from `/proc/cmdline`. +/// +/// ACL UKI images include a `usrhash=` parameter in the kernel command +/// line (contributed by the verity addon). Returns `None` if the parameter +/// is not present or `/proc/cmdline` cannot be read. +fn read_active_usr_roothash() -> Option { + let cmdline = fs::read_to_string("/proc/cmdline").ok()?; + cmdline + .split_whitespace() + .find_map(|field| field.strip_prefix("usrhash=")) + .map(|hash| hash.to_owned()) } /// Returns an ordered map of mount points to their corresponding FileSystem objects. @@ -920,7 +973,7 @@ mod functional_test { let mut newroot_mount = NewrootMount::new(mount_point.to_owned()); newroot_mount - .mount_newroot_partitions(&ctx.spec, &ctx.partition_paths, AbVolumeSelection::VolumeA) + .mount_newroot_partitions(&ctx.spec, &ctx.partition_paths, AbVolumeSelection::VolumeA, None) .unwrap(); // Validate that the device has been successfully mounted @@ -1019,7 +1072,7 @@ mod functional_test { // Test recursive mounting let mut newroot_mount2 = NewrootMount::new(root_mount_dir.path().to_owned()); newroot_mount2 - .mount_newroot_partitions(&ctx.spec, &ctx.partition_paths, AbVolumeSelection::VolumeA) + .mount_newroot_partitions(&ctx.spec, &ctx.partition_paths, AbVolumeSelection::VolumeA, None) .unwrap(); assert!(root_mount_dir @@ -1127,7 +1180,8 @@ mod functional_test { .mount_newroot_partitions( &ctx.spec, &ctx.partition_paths, - AbVolumeSelection::VolumeA + AbVolumeSelection::VolumeA, + None, ) .unwrap_err() .kind(), @@ -1150,7 +1204,8 @@ mod functional_test { .mount_newroot_partitions( &ctx.spec, &ctx.partition_paths, - AbVolumeSelection::VolumeA + AbVolumeSelection::VolumeA, + None, ) .unwrap_err() .kind(), @@ -1243,7 +1298,8 @@ mod functional_test { .mount_newroot_partitions( &ctx.spec, &ctx.partition_paths, - AbVolumeSelection::VolumeA + AbVolumeSelection::VolumeA, + None, ) .expect_err( "Expected mount_new_root to fail because of populated directory as path" @@ -1335,7 +1391,7 @@ mod functional_test { let mut newroot_mount = NewrootMount::new(temp_mount_dir.path().to_owned()); // Mount NTFS partition newroot_mount - .mount_newroot_partitions(&ctx.spec, &ctx.partition_paths, AbVolumeSelection::VolumeA) + .mount_newroot_partitions(&ctx.spec, &ctx.partition_paths, AbVolumeSelection::VolumeA, None) .unwrap(); // If device is a file, fetch the name of loop device that was mounted at mount point; @@ -1374,7 +1430,7 @@ mod functional_test { let mut newroot_mount2 = NewrootMount::new(temp_mount_dir2.path().to_owned()); // Re-mount the NTFS partition newroot_mount2 - .mount_newroot_partitions(&ctx.spec, &ctx.partition_paths, AbVolumeSelection::VolumeA) + .mount_newroot_partitions(&ctx.spec, &ctx.partition_paths, AbVolumeSelection::VolumeA, None) .unwrap(); // Validate that the device has been successfully mounted diff --git a/mic-baremetal-to-qemu-guest.yaml b/mic-baremetal-to-qemu-guest.yaml new file mode 100644 index 000000000..1224bd64c --- /dev/null +++ b/mic-baremetal-to-qemu-guest.yaml @@ -0,0 +1,18 @@ +os: + bootloader: + resetType: hard-reset + hostname: azure-linux + kernelCommandLine: + extraCommandLine: + - console=tty0 + - console=ttyS0 + selinux: + mode: disabled + packages: + remove: + - dracut-hostonly + - dracut-megaraid + - selinux-policy + install: + - dracut-virtio + - qemu-guest-agent \ No newline at end of file From 1b9435cda40fd31290cf2a0987cf711b519ddf35 Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Wed, 3 Jun 2026 22:56:43 +0000 Subject: [PATCH 08/22] Add forceAbUpdate internal param to bypass SHA384 check When internalParams.forceAbUpdate is true, trident will proceed with an A/B update even when the old and new OS image SHA384 hashes match. This is useful for testing A/B update flows repeatedly with the same COSI file. Usage in trident-config.yaml: internalParams: forceAbUpdate: true Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/trident/src/engine/context/image.rs | 16 +++++++++++----- crates/trident_api/src/constants.rs | 4 ++++ 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/crates/trident/src/engine/context/image.rs b/crates/trident/src/engine/context/image.rs index 963842dc3..3788c6573 100644 --- a/crates/trident/src/engine/context/image.rs +++ b/crates/trident/src/engine/context/image.rs @@ -2,6 +2,7 @@ use log::debug; use trident_api::{ config::{HostConfigurationDynamicValidationError, ImageSha384}, + constants::internal_params::FORCE_AB_UPDATE, error::{InvalidInputError, TridentError}, }; @@ -24,11 +25,16 @@ impl EngineContext { } // Update if the sha384 has changed (including if one is 'ignored'), or both are ignored but - // the URL has changed. - (Some(old_os_image), Some(new_os_image)) => Ok(old_os_image.sha384 - != new_os_image.sha384 - || old_os_image.sha384 == ImageSha384::Ignored - && old_os_image.url != new_os_image.url), + // the URL has changed. Also update if forceAbUpdate is set. + (Some(old_os_image), Some(new_os_image)) => { + if self.spec.internal_params.get_flag(FORCE_AB_UPDATE) { + debug!("Force A/B update requested via internalParams"); + return Ok(true); + } + Ok(old_os_image.sha384 != new_os_image.sha384 + || old_os_image.sha384 == ImageSha384::Ignored + && old_os_image.url != new_os_image.url) + } (Some(_), None) => { // Return an error if the old spec requests an OS image but the new spec does not. diff --git a/crates/trident_api/src/constants.rs b/crates/trident_api/src/constants.rs index a0ccac7d8..8f62f2af7 100644 --- a/crates/trident_api/src/constants.rs +++ b/crates/trident_api/src/constants.rs @@ -227,6 +227,10 @@ pub mod internal_params { /// Block Trident from closing encrypted volumes at the start of provisioning. pub const NO_CLOSE_ENCRYPTED_VOLUMES: &str = "noCloseEncryptedVolumes"; + /// Force an A/B update even when the OS image SHA384 has not changed. + /// Useful for testing A/B update flows with the same COSI repeatedly. + pub const FORCE_AB_UPDATE: &str = "forceAbUpdate"; + /// Block Trident from transitioning to the new OS after finalizing. pub const NO_TRANSITION: &str = "noTransition"; From 5a00ebda726f7012269c742b81dffda12b13a2ac Mon Sep 17 00:00:00 2001 From: bfjelds Date: Wed, 3 Jun 2026 16:04:53 -0700 Subject: [PATCH 09/22] Fix rustfmt formatting in ab_update.rs and newroot.rs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/trident/src/engine/ab_update.rs | 5 +++- crates/trident/src/engine/newroot.rs | 38 ++++++++++++++++++++++---- 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/crates/trident/src/engine/ab_update.rs b/crates/trident/src/engine/ab_update.rs index 53cf3ca11..ba7a2925f 100644 --- a/crates/trident/src/engine/ab_update.rs +++ b/crates/trident/src/engine/ab_update.rs @@ -1,4 +1,7 @@ -use std::{path::{Path, PathBuf}, time::Instant}; +use std::{ + path::{Path, PathBuf}, + time::Instant, +}; use log::{debug, info, warn}; diff --git a/crates/trident/src/engine/newroot.rs b/crates/trident/src/engine/newroot.rs index 54d2541be..5f9a58006 100644 --- a/crates/trident/src/engine/newroot.rs +++ b/crates/trident/src/engine/newroot.rs @@ -77,7 +77,12 @@ impl NewrootMount { let mut newroot_mount = NewrootMount::new(new_root_path); newroot_mount - .mount_newroot_partitions(host_config, partition_paths, update_volume, staging_usr_roothash) + .mount_newroot_partitions( + host_config, + partition_paths, + update_volume, + staging_usr_roothash, + ) .message("Failed to mount all partitions in newroot")?; // Mount tmpfs for /tmp and /run @@ -170,7 +175,8 @@ impl NewrootMount { } // Check for ACL BTRFS UUID collision before mounting. - let acl_collision_uuid = detect_acl_btrfs_uuid_collision(update_volume, staging_usr_roothash); + let acl_collision_uuid = + detect_acl_btrfs_uuid_collision(update_volume, staging_usr_roothash); // Mount all block devices in the newroot mount_points_map(host_config) @@ -973,7 +979,12 @@ mod functional_test { let mut newroot_mount = NewrootMount::new(mount_point.to_owned()); newroot_mount - .mount_newroot_partitions(&ctx.spec, &ctx.partition_paths, AbVolumeSelection::VolumeA, None) + .mount_newroot_partitions( + &ctx.spec, + &ctx.partition_paths, + AbVolumeSelection::VolumeA, + None, + ) .unwrap(); // Validate that the device has been successfully mounted @@ -1072,7 +1083,12 @@ mod functional_test { // Test recursive mounting let mut newroot_mount2 = NewrootMount::new(root_mount_dir.path().to_owned()); newroot_mount2 - .mount_newroot_partitions(&ctx.spec, &ctx.partition_paths, AbVolumeSelection::VolumeA, None) + .mount_newroot_partitions( + &ctx.spec, + &ctx.partition_paths, + AbVolumeSelection::VolumeA, + None, + ) .unwrap(); assert!(root_mount_dir @@ -1391,7 +1407,12 @@ mod functional_test { let mut newroot_mount = NewrootMount::new(temp_mount_dir.path().to_owned()); // Mount NTFS partition newroot_mount - .mount_newroot_partitions(&ctx.spec, &ctx.partition_paths, AbVolumeSelection::VolumeA, None) + .mount_newroot_partitions( + &ctx.spec, + &ctx.partition_paths, + AbVolumeSelection::VolumeA, + None, + ) .unwrap(); // If device is a file, fetch the name of loop device that was mounted at mount point; @@ -1430,7 +1451,12 @@ mod functional_test { let mut newroot_mount2 = NewrootMount::new(temp_mount_dir2.path().to_owned()); // Re-mount the NTFS partition newroot_mount2 - .mount_newroot_partitions(&ctx.spec, &ctx.partition_paths, AbVolumeSelection::VolumeA, None) + .mount_newroot_partitions( + &ctx.spec, + &ctx.partition_paths, + AbVolumeSelection::VolumeA, + None, + ) .unwrap(); // Validate that the device has been successfully mounted From f3918abefca92c721534de6de2356510c13bb815 Mon Sep 17 00:00:00 2001 From: bfjelds Date: Wed, 3 Jun 2026 16:50:06 -0700 Subject: [PATCH 10/22] Remove stray files accidentally committed Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- atomic_write.patch | 235 ------------------------------- mic-baremetal-to-qemu-guest.yaml | 18 --- 2 files changed, 253 deletions(-) delete mode 100644 atomic_write.patch delete mode 100644 mic-baremetal-to-qemu-guest.yaml diff --git a/atomic_write.patch b/atomic_write.patch deleted file mode 100644 index 63f61c4a0..000000000 --- a/atomic_write.patch +++ /dev/null @@ -1,235 +0,0 @@ -diff --git a/crates/osmodifier/src/modules.rs b/crates/osmodifier/src/modules.rs -index ce46c95f..e3174c61 100644 ---- a/crates/osmodifier/src/modules.rs -+++ b/crates/osmodifier/src/modules.rs -@@ -12,6 +12,8 @@ use trident_api::config::{LoadMode, Module}; - - use crate::OsModifierContext; - -+use osutils::files::atomic_write_file; -+ - const MODULES_LOAD_DIR: &str = "/etc/modules-load.d"; - const MODULES_LOAD_CONF: &str = "/etc/modules-load.d/modules-load.conf"; - const MODPROBE_DIR: &str = "/etc/modprobe.d"; -@@ -184,8 +186,7 @@ fn write_config(path: &std::path::Path, lines: &[String]) -> Result<(), Error> { - s.push('\n'); - s - }; -- fs::write(path, &content) -- .with_context(|| format!("Failed to write config to '{}'", path.display())) -+ atomic_write_file(path, &content) - } - - #[cfg_attr(not(test), allow(unused_imports, dead_code))] -diff --git a/crates/osmodifier/src/users.rs b/crates/osmodifier/src/users.rs -index 68fda4ab..e37619d9 100644 ---- a/crates/osmodifier/src/users.rs -+++ b/crates/osmodifier/src/users.rs -@@ -14,6 +14,8 @@ use crate::{ - OsModifierContext, - }; - -+use osutils::files::atomic_write_file; -+ - // Shadow file field indices (0-based, colon-delimited). - const SHADOW_FIELD_PASSWORD: usize = 1; - const SHADOW_FIELD_LAST_CHANGE: usize = 2; -@@ -576,71 +578,4 @@ fn set_startup_command(ctx: &OsModifierContext, username: &str, cmd: &str) -> Re - } - - atomic_write_file(&passwd_path, &result) --} -- --/// Atomically write a file by writing to a temp file and renaming. --/// This prevents corruption from crashes mid-write. --/// --/// Preserves permissions and uid/gid ownership from the original file. --/// Note: SELinux labels and extended attributes are not preserved because --/// osmodifier runs inside the target root before SELinux enforcement. --fn atomic_write_file(path: &std::path::Path, content: &str) -> Result<(), Error> { -- use std::io::Write as IoWrite; -- use std::os::unix::fs::MetadataExt; -- -- let parent = path.parent().context("Cannot determine parent directory")?; -- -- let mut tmp = tempfile::NamedTempFile::new_in(parent) -- .with_context(|| format!("Failed to create temp file in '{}'", parent.display()))?; -- -- tmp.write_all(content.as_bytes()) -- .with_context(|| format!("Failed to write temp file for '{}'", path.display()))?; -- -- tmp.flush() -- .with_context(|| format!("Failed to flush temp file for '{}'", path.display()))?; -- -- // fsync the temp file before rename to ensure data is on disk. Without -- // this, a power loss between rename and dirty-page flush could leave the -- // file zero-length (e.g., /etc/shadow → locked out of all accounts). -- tmp.as_file() -- .sync_all() -- .with_context(|| format!("Failed to fsync temp file for '{}'", path.display()))?; -- -- // Preserve ownership and permissions from the original file if it exists. -- // Ownership must be set before permissions because chown can clear -- // setuid/setgid bits. -- if let Ok(metadata) = fs::metadata(path) { -- use std::os::fd::AsFd; -- nix::unistd::fchown( -- tmp.as_file().as_fd(), -- Some(nix::unistd::Uid::from_raw(metadata.uid())), -- Some(nix::unistd::Gid::from_raw(metadata.gid())), -- ) -- .with_context(|| { -- format!( -- "Failed to set ownership on temp file for '{}'", -- path.display() -- ) -- })?; -- -- fs::set_permissions(tmp.path(), metadata.permissions()).with_context(|| { -- format!( -- "Failed to set permissions on temp file for '{}'", -- path.display() -- ) -- })?; -- } -- -- tmp.persist(path) -- .with_context(|| format!("Failed to atomically replace '{}'", path.display()))?; -- -- // Sync parent directory to ensure the rename (directory entry update) is -- // durable. Without this, the old file could reappear after power loss. -- if let Some(parent) = path.parent() { -- if let Ok(dir) = std::fs::File::open(parent) { -- let _ = dir.sync_all(); -- } -- } -- -- Ok(()) --} -+} -\ No newline at end of file -diff --git a/crates/osutils/src/files.rs b/crates/osutils/src/files.rs -index 5a3a3950..ae52f7f1 100644 ---- a/crates/osutils/src/files.rs -+++ b/crates/osutils/src/files.rs -@@ -192,6 +192,71 @@ where - }) - } - -+/// Atomically replace `path` with `content`. -+/// -+/// Writes to a temp file in the same directory, fsyncs, preserves ownership -+/// and permissions from the original file (if it exists), then renames. This -+/// guarantees that readers never see a partial write. -+pub fn atomic_write_file(path: &Path, content: &str) -> Result<(), Error> { -+ use std::os::unix::fs::MetadataExt; -+ -+ let parent = path.parent().context("Cannot determine parent directory")?; -+ -+ let mut tmp = tempfile::NamedTempFile::new_in(parent) -+ .with_context(|| format!("Failed to create temp file in '{}'", parent.display()))?; -+ -+ tmp.write_all(content.as_bytes()) -+ .with_context(|| format!("Failed to write temp file for '{}'", path.display()))?; -+ -+ tmp.flush() -+ .with_context(|| format!("Failed to flush temp file for '{}'", path.display()))?; -+ -+ // fsync the temp file before rename to ensure data is on disk. Without -+ // this, a power loss between rename and dirty-page flush could leave the -+ // file zero-length. -+ tmp.as_file() -+ .sync_all() -+ .with_context(|| format!("Failed to fsync temp file for '{}'", path.display()))?; -+ -+ // Preserve ownership and permissions from the original file if it exists. -+ // Ownership must be set before permissions because chown can clear -+ // setuid/setgid bits. -+ if let Ok(metadata) = fs::metadata(path) { -+ use std::os::fd::AsFd; -+ nix::unistd::fchown( -+ tmp.as_file().as_fd(), -+ Some(nix::unistd::Uid::from_raw(metadata.uid())), -+ Some(nix::unistd::Gid::from_raw(metadata.gid())), -+ ) -+ .with_context(|| { -+ format!( -+ "Failed to set ownership on temp file for '{}'", -+ path.display() -+ ) -+ })?; -+ -+ fs::set_permissions(tmp.path(), metadata.permissions()).with_context(|| { -+ format!( -+ "Failed to set permissions on temp file for '{}'", -+ path.display() -+ ) -+ })?; -+ } -+ -+ tmp.persist(path) -+ .with_context(|| format!("Failed to atomically replace '{}'", path.display()))?; -+ -+ // Sync parent directory to ensure the rename (directory entry update) is -+ // durable. Without this, the old file could reappear after power loss. -+ if let Some(parent) = path.parent() { -+ if let Ok(dir) = fs::File::open(parent) { -+ let _ = dir.sync_all(); -+ } -+ } -+ -+ Ok(()) -+} -+ - #[cfg(test)] - mod tests { - use super::*; -@@ -346,4 +411,48 @@ mod tests { - ); - }); - } -+ -+ #[test] -+ fn test_atomic_write_creates_new_file() { -+ let tmp = tempdir().unwrap(); -+ let path = tmp.path().join("new_file.conf"); -+ -+ atomic_write_file(&path, "hello\n").unwrap(); -+ -+ assert_eq!(fs::read_to_string(&path).unwrap(), "hello\n"); -+ } -+ -+ #[test] -+ fn test_atomic_write_replaces_existing_file() { -+ let tmp = tempdir().unwrap(); -+ let path = tmp.path().join("existing.conf"); -+ fs::write(&path, "old content\n").unwrap(); -+ -+ atomic_write_file(&path, "new content\n").unwrap(); -+ -+ assert_eq!(fs::read_to_string(&path).unwrap(), "new content\n"); -+ } -+ -+ #[test] -+ fn test_atomic_write_preserves_permissions() { -+ let tmp = tempdir().unwrap(); -+ let path = tmp.path().join("perms.conf"); -+ fs::write(&path, "original\n").unwrap(); -+ fs::set_permissions(&path, Permissions::from_mode(0o640)).unwrap(); -+ -+ atomic_write_file(&path, "updated\n").unwrap(); -+ -+ let mode = fs::metadata(&path).unwrap().permissions().mode() & 0o777; -+ assert_eq!(mode, 0o640, "Expected mode 0640, got {mode:04o}"); -+ } -+ -+ #[test] -+ fn test_atomic_write_empty_content() { -+ let tmp = tempdir().unwrap(); -+ let path = tmp.path().join("empty.conf"); -+ -+ atomic_write_file(&path, "").unwrap(); -+ -+ assert_eq!(fs::read_to_string(&path).unwrap(), ""); -+ } - } diff --git a/mic-baremetal-to-qemu-guest.yaml b/mic-baremetal-to-qemu-guest.yaml deleted file mode 100644 index 1224bd64c..000000000 --- a/mic-baremetal-to-qemu-guest.yaml +++ /dev/null @@ -1,18 +0,0 @@ -os: - bootloader: - resetType: hard-reset - hostname: azure-linux - kernelCommandLine: - extraCommandLine: - - console=tty0 - - console=ttyS0 - selinux: - mode: disabled - packages: - remove: - - dracut-hostonly - - dracut-megaraid - - selinux-policy - install: - - dracut-virtio - - qemu-guest-agent \ No newline at end of file From e04b9e0d9b6a217a196b2cfdfba2149466c70322 Mon Sep 17 00:00:00 2001 From: bfjelds Date: Wed, 3 Jun 2026 17:03:31 -0700 Subject: [PATCH 11/22] Validate ACL duplicate FS UUIDs with verity root hash comparison Replace the blanket ACL skip in validate_filesystem_uniqueness() with proper validation. When a duplicate FS UUID is found during A/B update on ACL, the update is only allowed if: 1. The duplicate is on the /usr mount point 2. The staging COSI has a verity root hash 3. The active system has a usrhash= in /proc/cmdline 4. The normalized hashes match (merkle tree proof of identical content) If COSI partition metadata is available, also validates that the staging USR partition has a known ACL PARTUUID. Extracts ACL constants and read_active_usr_roothash() into a shared engine::acl module used by both osimage.rs and newroot.rs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/trident/src/engine/acl.rs | 24 +++ crates/trident/src/engine/mod.rs | 1 + crates/trident/src/engine/newroot.rs | 21 +-- .../trident/src/subsystems/storage/osimage.rs | 143 +++++++++++++++++- crates/trident_api/src/error.rs | 10 ++ 5 files changed, 173 insertions(+), 26 deletions(-) create mode 100644 crates/trident/src/engine/acl.rs diff --git a/crates/trident/src/engine/acl.rs b/crates/trident/src/engine/acl.rs new file mode 100644 index 000000000..62b6db3e8 --- /dev/null +++ b/crates/trident/src/engine/acl.rs @@ -0,0 +1,24 @@ +//! ACL (Azure Container Linux) UKI-specific constants and helpers. +//! +//! ACL uses fixed PARTUUIDs for USR A/B partitions and a verity addon that +//! places the root hash in the kernel command line as `usrhash=`. + +use std::fs; + +// ACL UKI disk layout defines fixed PARTUUIDs for the USR A/B data partitions. +// These are from acl-scripts disk_layout_uki.json. +pub const ACL_USR_A_PARTUUID: &str = "7130c94a-213a-4e5a-8e26-6cce9662f132"; +pub const ACL_USR_B_PARTUUID: &str = "e03dd35c-7c2d-4a47-b3fe-27f15780a57c"; + +/// Reads the active USR verity root hash from `/proc/cmdline`. +/// +/// ACL UKI images include a `usrhash=` parameter in the kernel command +/// line (contributed by the verity addon). Returns `None` if the parameter +/// is not present or `/proc/cmdline` cannot be read. +pub fn read_active_usr_roothash() -> Option { + let cmdline = fs::read_to_string("/proc/cmdline").ok()?; + cmdline + .split_whitespace() + .find_map(|field| field.strip_prefix("usrhash=")) + .map(|hash| hash.to_owned()) +} diff --git a/crates/trident/src/engine/mod.rs b/crates/trident/src/engine/mod.rs index e45142dd2..14ffca1e2 100644 --- a/crates/trident/src/engine/mod.rs +++ b/crates/trident/src/engine/mod.rs @@ -35,6 +35,7 @@ use crate::{ // Engine functionality pub mod ab_update; +pub(crate) mod acl; pub mod bootentries; mod clean_install; mod context; diff --git a/crates/trident/src/engine/newroot.rs b/crates/trident/src/engine/newroot.rs index 5f9a58006..f03ab60d8 100644 --- a/crates/trident/src/engine/newroot.rs +++ b/crates/trident/src/engine/newroot.rs @@ -385,10 +385,8 @@ fn should_be_bind_mounted(fs_type: Option) -> bool { } } -// ACL (Azure Container Linux) UKI disk layout defines fixed PARTUUIDs for -// the USR A/B data partitions. These are from acl-scripts disk_layout_uki.json. -const ACL_USR_A_PARTUUID: &str = "7130c94a-213a-4e5a-8e26-6cce9662f132"; -const ACL_USR_B_PARTUUID: &str = "e03dd35c-7c2d-4a47-b3fe-27f15780a57c"; +// ACL constants and helpers are in the shared acl module. +use super::acl::{self, ACL_USR_A_PARTUUID, ACL_USR_B_PARTUUID}; /// Detects a BTRFS filesystem UUID collision on ACL's USR A/B partitions. /// @@ -444,7 +442,7 @@ fn detect_acl_btrfs_uuid_collision( // partition has the same verity root hash. This provides a cryptographic // guarantee that the filesystems are byte-identical, not just a UUID match. if let Some(staging_hash) = staging_usr_roothash { - match read_active_usr_roothash() { + match acl::read_active_usr_roothash() { Some(active_hash) => { let staging_normalized = staging_hash.trim().to_lowercase(); let active_normalized = active_hash.trim().to_lowercase(); @@ -477,19 +475,6 @@ fn detect_acl_btrfs_uuid_collision( Some(active_uuid) } -/// Reads the active USR verity root hash from `/proc/cmdline`. -/// -/// ACL UKI images include a `usrhash=` parameter in the kernel command -/// line (contributed by the verity addon). Returns `None` if the parameter -/// is not present or `/proc/cmdline` cannot be read. -fn read_active_usr_roothash() -> Option { - let cmdline = fs::read_to_string("/proc/cmdline").ok()?; - cmdline - .split_whitespace() - .find_map(|field| field.strip_prefix("usrhash=")) - .map(|hash| hash.to_owned()) -} - /// Returns an ordered map of mount points to their corresponding FileSystem objects. fn mount_points_map(host_config: &HostConfiguration) -> BTreeMap<&Path, &FileSystem> { host_config diff --git a/crates/trident/src/subsystems/storage/osimage.rs b/crates/trident/src/subsystems/storage/osimage.rs index 4279bdd76..fc74ca010 100644 --- a/crates/trident/src/subsystems/storage/osimage.rs +++ b/crates/trident/src/subsystems/storage/osimage.rs @@ -29,6 +29,8 @@ use crate::{ osimage::{OsImage, OsImageFileSystemType}, }; +use sysdefs::osuuid::OsUuid; + use super::EngineContext; /// Validates that the Host Configuration aligns with the OS image metadata. @@ -208,8 +210,7 @@ fn validate_filesystem_uniqueness( } // For A/B Update, check that no A/B volumes share filesystem UUIDs. - // ACL uses the same FS UUID for both A/B slots — partitions are identified by PARTUUID. - if ctx.servicing_type == ServicingType::AbUpdate && !ctx.image_distro().is_acl() { + if ctx.servicing_type == ServicingType::AbUpdate { if let Some(ab) = &ctx.spec.storage.ab_update { for pair in ab.volume_pairs.iter() { if let Some(mp_info) = ctx.spec.storage.device_id_to_mount_point_info(&pair.id) { @@ -245,12 +246,20 @@ fn validate_filesystem_uniqueness( trace!("Checking A/B volume pair '{}'. Found active volume filesystem UUID '{active_volume_fs_uuid}'\ and inactive volume filesystem UUID '{inactive_volume_fs_uuid}'", pair.id); if active_volume_fs_uuid == inactive_volume_fs_uuid { - return Err(TridentError::new( - InvalidInputError::DuplicateFsUuidAbUpdate { - pair_id: pair.id.to_string(), - uuid: inactive_volume_fs_uuid.to_string(), - }, - )); + if ctx.image_distro().is_acl() { + validate_acl_duplicate_uuid( + os_image, + &mp_info.mount_point.path, + &inactive_volume_fs_uuid, + )?; + } else { + return Err(TridentError::new( + InvalidInputError::DuplicateFsUuidAbUpdate { + pair_id: pair.id.to_string(), + uuid: inactive_volume_fs_uuid.to_string(), + }, + )); + } } } else { warn!("Could not find filesystem UUID for active volume of A/B volume pair '{}'", pair.id); @@ -263,6 +272,124 @@ fn validate_filesystem_uniqueness( Ok(()) } +/// Validates that an ACL A/B update with a duplicate filesystem UUID is safe. +/// +/// ACL BTRFS images may intentionally share filesystem UUIDs between the A and B +/// slots when built from the same source. This is safe only when: +/// +/// 1. The duplicate is on the `/usr` mount point (ACL's verity-protected USR partition) +/// 2. The staging image has a verity root hash in its COSI metadata +/// 3. The active system's `/proc/cmdline` has a matching `usrhash=` parameter +/// 4. The normalized root hashes match — proving byte-identical content via merkle tree +/// +/// If COSI partition metadata is available, the function also validates that the +/// staging USR partition's PARTUUID matches a known ACL USR slot. +fn validate_acl_duplicate_uuid( + os_image: &OsImage, + mount_point: &Path, + fs_uuid: &OsUuid, +) -> Result<(), TridentError> { + use crate::engine::acl::{self, ACL_USR_A_PARTUUID, ACL_USR_B_PARTUUID}; + + let mount_str = mount_point.to_string_lossy(); + let uuid_str = fs_uuid.to_string(); + + // 1. Only /usr is allowed to have duplicate UUIDs on ACL. + if mount_point != Path::new("/usr") { + return Err(TridentError::new( + InvalidInputError::DuplicateFsUuidAclVerificationFailed { + uuid: uuid_str, + mount_point: mount_str.to_string(), + reason: "duplicate FS UUID is only allowed on /usr for ACL".to_string(), + }, + )); + } + + // 2. Staging image must have a verity root hash. + let staging_fs = os_image + .filesystems() + .find(|f| f.mount_point == mount_point); + let staging_roothash = staging_fs + .as_ref() + .and_then(|f| f.verity.as_ref()) + .map(|v| v.roothash.as_str()); + + let staging_hash = match staging_roothash { + Some(h) if !h.is_empty() => h, + _ => { + return Err(TridentError::new( + InvalidInputError::DuplicateFsUuidAclVerificationFailed { + uuid: uuid_str, + mount_point: mount_str.to_string(), + reason: "staging /usr image has no verity root hash in COSI metadata" + .to_string(), + }, + )); + } + }; + + // 3. Active system must have a usrhash= in /proc/cmdline. + let active_hash = match acl::read_active_usr_roothash() { + Some(h) if !h.is_empty() => h, + _ => { + return Err(TridentError::new( + InvalidInputError::DuplicateFsUuidAclVerificationFailed { + uuid: uuid_str, + mount_point: mount_str.to_string(), + reason: "cannot read active USR verity root hash from /proc/cmdline \ + (usrhash= parameter not found)" + .to_string(), + }, + )); + } + }; + + // 4. Hashes must match (case-insensitive, trimmed). + let staging_normalized = staging_hash.trim().to_lowercase(); + let active_normalized = active_hash.trim().to_lowercase(); + if staging_normalized != active_normalized { + return Err(TridentError::new( + InvalidInputError::DuplicateFsUuidAclVerificationFailed { + uuid: uuid_str, + mount_point: mount_str.to_string(), + reason: format!( + "verity root hash mismatch: staging has '{}...', active has '{}...'", + &staging_normalized[..staging_normalized.len().min(16)], + &active_normalized[..active_normalized.len().min(16)] + ), + }, + )); + } + + debug!( + "ACL duplicate FS UUID '{}' on /usr is safe: verity root hashes match ({}...)", + uuid_str, + &staging_normalized[..staging_normalized.len().min(16)] + ); + + // Optional: If COSI partition metadata is available, validate that the staging + // USR partition PARTUUID matches a known ACL USR slot. + if let Some(partitions) = os_image.partitions() { + let known_partuuids: Vec<&str> = vec![ACL_USR_A_PARTUUID, ACL_USR_B_PARTUUID]; + let has_acl_usr_partuuid = partitions.any(|p| { + let part_uuid_str = p.info.part_uuid.to_string().to_lowercase(); + p.info.part_type.is_acl_usr() + && known_partuuids + .iter() + .any(|known| *known == part_uuid_str) + }); + + if !has_acl_usr_partuuid { + warn!( + "ACL COSI partition metadata does not contain a known ACL USR PARTUUID. \ + Update is allowed (verity hashes match) but disk layout may be unexpected." + ); + } + } + + Ok(()) +} + /// Validates that the OS Image and the HC match in terms of verity configuration. /// /// The set of verity filesystems provided by the image must exactly match the set of verity diff --git a/crates/trident_api/src/error.rs b/crates/trident_api/src/error.rs index 57aa13b16..70c88e1a1 100644 --- a/crates/trident_api/src/error.rs +++ b/crates/trident_api/src/error.rs @@ -204,6 +204,16 @@ pub enum InvalidInputError { )] DuplicateFsUuidAbUpdate { pair_id: String, uuid: String }, + #[error( + "ACL A/B update has duplicate FS UUID {uuid} on mount point {mount_point} \ + but cannot verify content identity: {reason}" + )] + DuplicateFsUuidAclVerificationFailed { + uuid: String, + mount_point: String, + reason: String, + }, + #[error("Cannot find history file")] HistoryFileNotFound, From f34c5ee725136049362ad35a4d4b1108758aa365 Mon Sep 17 00:00:00 2001 From: bfjelds Date: Wed, 3 Jun 2026 17:04:49 -0700 Subject: [PATCH 12/22] Fix build: compare PARTUUIDs directly instead of using is_acl_usr() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DiscoverablePartitionType does not have is_acl_usr() — that method lives on the HC PartitionType enum. Since we already check for known ACL USR PARTUUIDs, the part_type check was redundant. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/trident/src/subsystems/storage/osimage.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/crates/trident/src/subsystems/storage/osimage.rs b/crates/trident/src/subsystems/storage/osimage.rs index fc74ca010..6860bd8c2 100644 --- a/crates/trident/src/subsystems/storage/osimage.rs +++ b/crates/trident/src/subsystems/storage/osimage.rs @@ -373,10 +373,9 @@ fn validate_acl_duplicate_uuid( let known_partuuids: Vec<&str> = vec![ACL_USR_A_PARTUUID, ACL_USR_B_PARTUUID]; let has_acl_usr_partuuid = partitions.any(|p| { let part_uuid_str = p.info.part_uuid.to_string().to_lowercase(); - p.info.part_type.is_acl_usr() - && known_partuuids - .iter() - .any(|known| *known == part_uuid_str) + known_partuuids + .iter() + .any(|known| *known == part_uuid_str) }); if !has_acl_usr_partuuid { From 97268753e10112bca3c64c5b54f6e9dd66c2ee42 Mon Sep 17 00:00:00 2001 From: bfjelds Date: Wed, 3 Jun 2026 17:06:39 -0700 Subject: [PATCH 13/22] Fix build: add mut to partitions iterator binding Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/trident/src/subsystems/storage/osimage.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/trident/src/subsystems/storage/osimage.rs b/crates/trident/src/subsystems/storage/osimage.rs index 6860bd8c2..c494dbba5 100644 --- a/crates/trident/src/subsystems/storage/osimage.rs +++ b/crates/trident/src/subsystems/storage/osimage.rs @@ -369,7 +369,7 @@ fn validate_acl_duplicate_uuid( // Optional: If COSI partition metadata is available, validate that the staging // USR partition PARTUUID matches a known ACL USR slot. - if let Some(partitions) = os_image.partitions() { + if let Some(mut partitions) = os_image.partitions() { let known_partuuids: Vec<&str> = vec![ACL_USR_A_PARTUUID, ACL_USR_B_PARTUUID]; let has_acl_usr_partuuid = partitions.any(|p| { let part_uuid_str = p.info.part_uuid.to_string().to_lowercase(); From 952f76374117bae99666edcc4a745d7d10b99f6c Mon Sep 17 00:00:00 2001 From: bfjelds Date: Wed, 3 Jun 2026 18:52:08 -0700 Subject: [PATCH 14/22] Clean up old target-slot UKIs before staging new one The ESP (128 MB) can overflow when multiple UKIs accumulate across A/B updates. Before staging a new UKI, remove old UKIs for the target slot: 1. Trident-managed UKIs matching the target slot (all install indices) 2. Non-trident-managed (original install) UKIs, but only when trident already manages the other slot (proving it owns boot management) The other slot's UKI is always preserved as the active/rollback path. Also extract UKI_SLOT_A/UKI_SLOT_B constants to replace string literals. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/trident/src/engine/boot/uki.rs | 100 ++++++++++++++++++++++++-- crates/trident/src/subsystems/esp.rs | 4 ++ 2 files changed, 100 insertions(+), 4 deletions(-) diff --git a/crates/trident/src/engine/boot/uki.rs b/crates/trident/src/engine/boot/uki.rs index 4b8414b98..cd81d609f 100644 --- a/crates/trident/src/engine/boot/uki.rs +++ b/crates/trident/src/engine/boot/uki.rs @@ -25,11 +25,23 @@ pub const TMP_UKI_NAME: &str = "vmlinuz-0.efi.staged"; pub const UKI_DIRECTORY: &str = formatcp!("{ESP_EFI_DIRECTORY}/Linux"); const TMP_UKI_ADDON_DIR_NAME: &str = formatcp!("{TMP_UKI_NAME}{UKI_ADDON_DIR_SUFFIX}"); +/// Slot identifier embedded in trident-managed UKI filenames. +const UKI_SLOT_A: &str = "azla"; +const UKI_SLOT_B: &str = "azlb"; + /// Returns the UKI file suffix, given the current active volume and install index. fn uki_suffix(ctx: &EngineContext) -> String { match ctx.ab_active_volume { - Some(AbVolumeSelection::VolumeA) => format!("azlb{}.efi", ctx.install_index), - None | Some(AbVolumeSelection::VolumeB) => format!("azla{}.efi", ctx.install_index), + Some(AbVolumeSelection::VolumeA) => format!("{UKI_SLOT_B}{}.efi", ctx.install_index), + None | Some(AbVolumeSelection::VolumeB) => format!("{UKI_SLOT_A}{}.efi", ctx.install_index), + } +} + +/// Returns the slot identifier for the target volume (the slot being updated). +fn target_slot_id(ctx: &EngineContext) -> &'static str { + match ctx.ab_active_volume { + Some(AbVolumeSelection::VolumeA) => UKI_SLOT_B, + None | Some(AbVolumeSelection::VolumeB) => UKI_SLOT_A, } } @@ -160,6 +172,84 @@ pub fn prepare_esp_for_uki(root_mount_point: &Path, esp_mount_path: &Path) -> Re Ok(()) } +/// Removes a UKI file and its associated addon directory if present. +fn remove_uki_and_addons(path: &Path) -> Result<(), Error> { + fs::remove_file(path) + .with_context(|| format!("Failed to remove UKI file '{}'", path.display()))?; + let addon_dir = uki::uki_addon_dir(path); + if addon_dir.exists() && addon_dir.is_dir() { + fs::remove_dir_all(&addon_dir).with_context(|| { + format!( + "Failed to remove UKI addon directory '{}'", + addon_dir.display() + ) + })?; + } + Ok(()) +} + +/// Cleans up old UKIs for the target slot before staging a new one, freeing +/// ESP space. +/// +/// When staging a UKI for slot X, the old slot X UKI is being replaced and +/// can be removed. The other slot's UKI is the active/rollback and must stay. +/// +/// Removes: +/// 1. Trident-managed UKIs matching the target slot (e.g. all `azla` files +/// when staging for slot A, regardless of install index). +/// 2. Non-trident-managed (original install) UKIs, but only when a +/// trident-managed UKI for the *other* slot already exists — meaning +/// trident has taken over boot management and the original install UKI +/// is no longer the sole rollback. +pub fn cleanup_ukis_before_staging( + ctx: &EngineContext, + mount_point: &Path, + esp_mount_path: &Path, +) -> Result<(), Error> { + let esp_uki_directory = join_relative(mount_point, esp_mount_path).join(UKI_DIRECTORY); + if !esp_uki_directory.exists() { + return Ok(()); + } + + let target_slot = target_slot_id(ctx); + let trident_ukis = enumerate_trident_managed_ukis(&esp_uki_directory)?; + + // 1. Remove trident-managed UKIs for the target slot (all install indices). + for (_index, suffix, path) in &trident_ukis { + if suffix.contains(target_slot) { + debug!( + "Pre-staging cleanup: removing old target-slot UKI '{}'", + path.display() + ); + remove_uki_and_addons(path)?; + } + } + + // 2. Remove non-trident-managed UKIs only if trident already manages the + // other slot (proving trident owns boot and the original is superseded). + let other_slot = if target_slot == UKI_SLOT_A { + UKI_SLOT_B + } else { + UKI_SLOT_A + }; + let has_other_slot_uki = trident_ukis + .iter() + .any(|(_index, suffix, _)| suffix.contains(other_slot)); + + if has_other_slot_uki { + let non_trident_ukis = enumerate_non_trident_managed_ukis(&esp_uki_directory)?; + for (_version, path) in non_trident_ukis { + debug!( + "Pre-staging cleanup: removing superseded original UKI '{}'", + path.display() + ); + remove_uki_and_addons(&path)?; + } + } + + Ok(()) +} + /// Enumerates existing UKIs managed by Trident (defined by naming convention: vmlinuz--azl.efi) /// in the given directory, returning their indices, suffixes, and paths. fn enumerate_trident_managed_ukis( @@ -187,7 +277,9 @@ fn enumerate_trident_managed_ukis( .and_then(|filename| filename.strip_prefix("vmlinuz-")) .and_then(|f| f.split_once('-')) .filter(|(_, suffix)| { - suffix.contains("staged") || suffix.contains("azla") || suffix.contains("azlb") + suffix.contains("staged") + || suffix.contains(UKI_SLOT_A) + || suffix.contains(UKI_SLOT_B) }) .and_then(|(index, suffix)| Some((index.parse::().ok()?, suffix.to_string()))) { @@ -319,7 +411,7 @@ fn enumerate_non_trident_managed_ukis( if let Some(version) = filename .to_str() .and_then(|filename| filename.strip_prefix("vmlinuz-")) - .filter(|f| !f.contains("azla") && !f.contains("azlb")) + .filter(|f| !f.contains(UKI_SLOT_A) && !f.contains(UKI_SLOT_B)) .and_then(|filename| filename.strip_suffix(".efi")) { match version.parse() { diff --git a/crates/trident/src/subsystems/esp.rs b/crates/trident/src/subsystems/esp.rs index 36879aaaf..177eb6fec 100644 --- a/crates/trident/src/subsystems/esp.rs +++ b/crates/trident/src/subsystems/esp.rs @@ -288,6 +288,10 @@ fn copy_file_artifacts( // Prepare ESP directory structure for UKI boot uki::prepare_esp_for_uki(mount_point, &ctx.esp_mount_path)?; + // Clean up old UKIs for the target slot to free ESP space before + // staging the new UKI. + uki::cleanup_ukis_before_staging(ctx, mount_point, &ctx.esp_mount_path)?; + // Copy the UKI from the image into the ESP directory uki::stage_uki_on_esp(temp_mount_dir, mount_point, &ctx.esp_mount_path)?; From 5cb26e3edfd749d2fd42fa6b5b6c5c847dedafac Mon Sep 17 00:00:00 2001 From: bfjelds Date: Wed, 3 Jun 2026 19:15:10 -0700 Subject: [PATCH 15/22] Fix rustfmt in osimage.rs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/trident/src/subsystems/storage/osimage.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/trident/src/subsystems/storage/osimage.rs b/crates/trident/src/subsystems/storage/osimage.rs index c494dbba5..00aa40afc 100644 --- a/crates/trident/src/subsystems/storage/osimage.rs +++ b/crates/trident/src/subsystems/storage/osimage.rs @@ -373,9 +373,7 @@ fn validate_acl_duplicate_uuid( let known_partuuids: Vec<&str> = vec![ACL_USR_A_PARTUUID, ACL_USR_B_PARTUUID]; let has_acl_usr_partuuid = partitions.any(|p| { let part_uuid_str = p.info.part_uuid.to_string().to_lowercase(); - known_partuuids - .iter() - .any(|known| *known == part_uuid_str) + known_partuuids.iter().any(|known| *known == part_uuid_str) }); if !has_acl_usr_partuuid { From 5131baec1feda572d6dfddc3afb40963de70424f Mon Sep 17 00:00:00 2001 From: bfjelds Date: Wed, 3 Jun 2026 19:35:42 -0700 Subject: [PATCH 16/22] Fix UKI cleanup to scope by slot+os-index, not just slot In multi-OS configurations, the ESP has UKI pairs per OS instance (azla0/azlb0, azla1/azlb1, etc.). Cleanup must only remove UKIs for the specific slot+os-index being updated, not all UKIs for the same slot letter across different OS instances. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/trident/src/engine/boot/uki.rs | 54 +++++++++++++++------------ 1 file changed, 31 insertions(+), 23 deletions(-) diff --git a/crates/trident/src/engine/boot/uki.rs b/crates/trident/src/engine/boot/uki.rs index cd81d609f..5d722f7ac 100644 --- a/crates/trident/src/engine/boot/uki.rs +++ b/crates/trident/src/engine/boot/uki.rs @@ -37,11 +37,21 @@ fn uki_suffix(ctx: &EngineContext) -> String { } } -/// Returns the slot identifier for the target volume (the slot being updated). -fn target_slot_id(ctx: &EngineContext) -> &'static str { +/// Returns the slot+os-index identifier for the target volume being updated +/// (e.g. "azla0" or "azlb1"). Used to match UKI filenames for cleanup. +fn target_slot_os_id(ctx: &EngineContext) -> String { match ctx.ab_active_volume { - Some(AbVolumeSelection::VolumeA) => UKI_SLOT_B, - None | Some(AbVolumeSelection::VolumeB) => UKI_SLOT_A, + Some(AbVolumeSelection::VolumeA) => format!("{UKI_SLOT_B}{}", ctx.install_index), + None | Some(AbVolumeSelection::VolumeB) => format!("{UKI_SLOT_A}{}", ctx.install_index), + } +} + +/// Returns the slot+os-index identifier for the active/rollback volume +/// (e.g. "azlb0" when active is B). Used to verify trident owns the other slot. +fn active_slot_os_id(ctx: &EngineContext) -> String { + match ctx.ab_active_volume { + Some(AbVolumeSelection::VolumeA) => format!("{UKI_SLOT_A}{}", ctx.install_index), + None | Some(AbVolumeSelection::VolumeB) => format!("{UKI_SLOT_B}{}", ctx.install_index), } } @@ -191,16 +201,16 @@ fn remove_uki_and_addons(path: &Path) -> Result<(), Error> { /// Cleans up old UKIs for the target slot before staging a new one, freeing /// ESP space. /// -/// When staging a UKI for slot X, the old slot X UKI is being replaced and -/// can be removed. The other slot's UKI is the active/rollback and must stay. +/// When staging a UKI for slot X on OS N, the old slot X/OS N UKI is being +/// replaced and can be removed. Other slots and other OS indices are preserved. /// /// Removes: -/// 1. Trident-managed UKIs matching the target slot (e.g. all `azla` files -/// when staging for slot A, regardless of install index). +/// 1. Trident-managed UKIs matching the target slot+os-index (e.g. all `azla0` +/// files when staging for slot A OS 0, regardless of update index). /// 2. Non-trident-managed (original install) UKIs, but only when a -/// trident-managed UKI for the *other* slot already exists — meaning -/// trident has taken over boot management and the original install UKI -/// is no longer the sole rollback. +/// trident-managed UKI for the *active* slot+os-index already exists — +/// meaning trident has taken over boot management and the original install +/// UKI is no longer the sole rollback. pub fn cleanup_ukis_before_staging( ctx: &EngineContext, mount_point: &Path, @@ -211,12 +221,14 @@ pub fn cleanup_ukis_before_staging( return Ok(()); } - let target_slot = target_slot_id(ctx); + let target_slot = target_slot_os_id(ctx); + let active_slot = active_slot_os_id(ctx); let trident_ukis = enumerate_trident_managed_ukis(&esp_uki_directory)?; - // 1. Remove trident-managed UKIs for the target slot (all install indices). + // 1. Remove trident-managed UKIs for the target slot+os-index + // (all update indices, e.g. vmlinuz-100-azla0.efi, vmlinuz-102-azla0.efi). for (_index, suffix, path) in &trident_ukis { - if suffix.contains(target_slot) { + if suffix.contains(&target_slot) { debug!( "Pre-staging cleanup: removing old target-slot UKI '{}'", path.display() @@ -226,17 +238,13 @@ pub fn cleanup_ukis_before_staging( } // 2. Remove non-trident-managed UKIs only if trident already manages the - // other slot (proving trident owns boot and the original is superseded). - let other_slot = if target_slot == UKI_SLOT_A { - UKI_SLOT_B - } else { - UKI_SLOT_A - }; - let has_other_slot_uki = trident_ukis + // active slot+os-index (proving trident owns boot for this OS instance + // and the original is superseded as rollback). + let has_active_slot_uki = trident_ukis .iter() - .any(|(_index, suffix, _)| suffix.contains(other_slot)); + .any(|(_index, suffix, _)| suffix.contains(&active_slot)); - if has_other_slot_uki { + if has_active_slot_uki { let non_trident_ukis = enumerate_non_trident_managed_ukis(&esp_uki_directory)?; for (_version, path) in non_trident_ukis { debug!( From 43b5ad0ba9cfce9189bd7bcac1d7fe1eaa693229 Mon Sep 17 00:00:00 2001 From: bfjelds Date: Thu, 4 Jun 2026 08:03:12 -0700 Subject: [PATCH 17/22] Only clean original UKI for install_index 0 (multiboot safety) In multiboot configurations, the original UKI has OS 0's partition references baked in. OS 1+ instances never depend on it, but only OS 0 should remove it since it's the owner. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/trident/src/engine/boot/uki.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/crates/trident/src/engine/boot/uki.rs b/crates/trident/src/engine/boot/uki.rs index 5d722f7ac..6a2ab61d3 100644 --- a/crates/trident/src/engine/boot/uki.rs +++ b/crates/trident/src/engine/boot/uki.rs @@ -237,14 +237,17 @@ pub fn cleanup_ukis_before_staging( } } - // 2. Remove non-trident-managed UKIs only if trident already manages the - // active slot+os-index (proving trident owns boot for this OS instance - // and the original is superseded as rollback). + // 2. Remove non-trident-managed UKIs only if: + // - This is install_index 0 (the OS that placed the original UKI), AND + // - Trident already manages the active slot (proving the original is + // superseded as rollback). + // In multiboot, install_index > 0 never owns the original UKI — it has + // OS 0's partition refs baked in and can't boot other OS instances. let has_active_slot_uki = trident_ukis .iter() .any(|(_index, suffix, _)| suffix.contains(&active_slot)); - if has_active_slot_uki { + if has_active_slot_uki && ctx.install_index == 0 { let non_trident_ukis = enumerate_non_trident_managed_ukis(&esp_uki_directory)?; for (_version, path) in non_trident_ukis { debug!( From 85025dddb9f095c962e93fb0cae9430e07b3337f Mon Sep 17 00:00:00 2001 From: bfjelds Date: Thu, 4 Jun 2026 11:04:32 -0700 Subject: [PATCH 18/22] refactor: pass active_usr_roothash to validate_acl_duplicate_uuid Move /proc/cmdline read out of validate_acl_duplicate_uuid into its caller (validate_filesystem_uniqueness). The function now accepts active_usr_roothash as Option, making it fully testable in unit tests without filesystem access. Add 7 unit tests covering all validation paths: - matching hash (success) - case-insensitive matching (success) - wrong mount point (reject) - no staging verity hash (reject) - mismatched hashes (reject) - no active hash / None (reject) - empty active hash (reject) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../trident/src/subsystems/storage/osimage.rs | 194 +++++++++++++++++- 1 file changed, 188 insertions(+), 6 deletions(-) diff --git a/crates/trident/src/subsystems/storage/osimage.rs b/crates/trident/src/subsystems/storage/osimage.rs index 00aa40afc..24125e100 100644 --- a/crates/trident/src/subsystems/storage/osimage.rs +++ b/crates/trident/src/subsystems/storage/osimage.rs @@ -247,10 +247,13 @@ fn validate_filesystem_uniqueness( and inactive volume filesystem UUID '{inactive_volume_fs_uuid}'", pair.id); if active_volume_fs_uuid == inactive_volume_fs_uuid { if ctx.image_distro().is_acl() { + let active_usr_roothash = + crate::engine::acl::read_active_usr_roothash(); validate_acl_duplicate_uuid( os_image, &mp_info.mount_point.path, &inactive_volume_fs_uuid, + active_usr_roothash, )?; } else { return Err(TridentError::new( @@ -288,8 +291,9 @@ fn validate_acl_duplicate_uuid( os_image: &OsImage, mount_point: &Path, fs_uuid: &OsUuid, + active_usr_roothash: Option, ) -> Result<(), TridentError> { - use crate::engine::acl::{self, ACL_USR_A_PARTUUID, ACL_USR_B_PARTUUID}; + use crate::engine::acl::{ACL_USR_A_PARTUUID, ACL_USR_B_PARTUUID}; let mount_str = mount_point.to_string_lossy(); let uuid_str = fs_uuid.to_string(); @@ -329,7 +333,7 @@ fn validate_acl_duplicate_uuid( }; // 3. Active system must have a usrhash= in /proc/cmdline. - let active_hash = match acl::read_active_usr_roothash() { + let active_hash = match active_usr_roothash { Some(h) if !h.is_empty() => h, _ => { return Err(TridentError::new( @@ -1429,11 +1433,189 @@ mod tests { }) ); } -} -#[cfg(feature = "functional-test")] -#[cfg_attr(not(test), allow(unused_imports, dead_code))] -mod functional_test { + // --- ACL duplicate UUID validation tests --- + // + // These test `validate_acl_duplicate_uuid` directly, which is the function + // that decides whether a duplicate FS UUID on an ACL image is safe (verity + // hashes match) or not. + + const ACL_TEST_HASH: &str = "abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789"; + + /// Helper: build a MockOsImage with a single /usr image that optionally has verity. + fn acl_mock_image(roothash: Option<&str>) -> MockOsImage { + MockOsImage { + source: url::Url::parse(OSIMAGE_DUMMY_SOURCE).unwrap(), + os_arch: SystemArchitecture::Amd64, + os_release: OsRelease { + id: Some("azurelinux".to_string()), + variant_id: Some("azurecontainerlinux".to_string()), + ..OsRelease::default() + }, + images: vec![MockImage { + mount_point: PathBuf::from("/usr"), + fs_type: OsImageFileSystemType::Ext4, + fs_uuid: OsUuid::Uuid( + Uuid::parse_str("a1a2a3a4b1b2c1c2d1d2d3d4d5d6d7d8").unwrap(), + ), + part_type: DiscoverablePartitionType::LinuxGeneric, + verity: roothash.map(|h| MockVerity { + roothash: h.to_string(), + }), + }], + is_uki: false, + partitioning_info: None, + } + } + + #[test] + fn test_acl_duplicate_uuid_matching_hash_success() { + let mock = acl_mock_image(Some(ACL_TEST_HASH)); + let os_image = OsImage::mock(mock); + let result = validate_acl_duplicate_uuid( + &os_image, + Path::new("/usr"), + &OsUuid::Uuid(Uuid::parse_str("a1a2a3a4b1b2c1c2d1d2d3d4d5d6d7d8").unwrap()), + Some(ACL_TEST_HASH.to_string()), + ); + assert!(result.is_ok(), "expected success, got: {:?}", result); + } + + #[test] + fn test_acl_duplicate_uuid_matching_hash_case_insensitive() { + let mock = acl_mock_image(Some(ACL_TEST_HASH)); + let os_image = OsImage::mock(mock); + let result = validate_acl_duplicate_uuid( + &os_image, + Path::new("/usr"), + &OsUuid::Uuid(Uuid::parse_str("a1a2a3a4b1b2c1c2d1d2d3d4d5d6d7d8").unwrap()), + Some(ACL_TEST_HASH.to_uppercase()), + ); + assert!( + result.is_ok(), + "expected case-insensitive match, got: {:?}", + result + ); + } + + #[test] + fn test_acl_duplicate_uuid_wrong_mount_point() { + let mut mock = acl_mock_image(Some(ACL_TEST_HASH)); + mock.images[0].mount_point = PathBuf::from("/"); + let os_image = OsImage::mock(mock); + let err = validate_acl_duplicate_uuid( + &os_image, + Path::new("/"), + &OsUuid::Uuid(Uuid::parse_str("a1a2a3a4b1b2c1c2d1d2d3d4d5d6d7d8").unwrap()), + Some(ACL_TEST_HASH.to_string()), + ) + .unwrap_err(); + assert!( + matches!( + err.kind(), + ErrorKind::InvalidInput( + InvalidInputError::DuplicateFsUuidAclVerificationFailed { reason, .. } + ) if reason.contains("only allowed on /usr") + ), + "expected wrong mount point error, got: {:?}", + err.kind() + ); + } + + #[test] + fn test_acl_duplicate_uuid_no_staging_verity_hash() { + let mock = acl_mock_image(None); + let os_image = OsImage::mock(mock); + let err = validate_acl_duplicate_uuid( + &os_image, + Path::new("/usr"), + &OsUuid::Uuid(Uuid::parse_str("a1a2a3a4b1b2c1c2d1d2d3d4d5d6d7d8").unwrap()), + Some(ACL_TEST_HASH.to_string()), + ) + .unwrap_err(); + assert!( + matches!( + err.kind(), + ErrorKind::InvalidInput( + InvalidInputError::DuplicateFsUuidAclVerificationFailed { reason, .. } + ) if reason.contains("no verity root hash") + ), + "expected no verity hash error, got: {:?}", + err.kind() + ); + } + + #[test] + fn test_acl_duplicate_uuid_mismatched_hash() { + let mock = acl_mock_image(Some(ACL_TEST_HASH)); + let os_image = OsImage::mock(mock); + let different_hash = + "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"; + let err = validate_acl_duplicate_uuid( + &os_image, + Path::new("/usr"), + &OsUuid::Uuid(Uuid::parse_str("a1a2a3a4b1b2c1c2d1d2d3d4d5d6d7d8").unwrap()), + Some(different_hash.to_string()), + ) + .unwrap_err(); + assert!( + matches!( + err.kind(), + ErrorKind::InvalidInput( + InvalidInputError::DuplicateFsUuidAclVerificationFailed { reason, .. } + ) if reason.contains("mismatch") + ), + "expected hash mismatch error, got: {:?}", + err.kind() + ); + } + + #[test] + fn test_acl_duplicate_uuid_no_active_hash() { + let mock = acl_mock_image(Some(ACL_TEST_HASH)); + let os_image = OsImage::mock(mock); + let err = validate_acl_duplicate_uuid( + &os_image, + Path::new("/usr"), + &OsUuid::Uuid(Uuid::parse_str("a1a2a3a4b1b2c1c2d1d2d3d4d5d6d7d8").unwrap()), + None, + ) + .unwrap_err(); + assert!( + matches!( + err.kind(), + ErrorKind::InvalidInput( + InvalidInputError::DuplicateFsUuidAclVerificationFailed { reason, .. } + ) if reason.contains("usrhash=") + ), + "expected no active hash error, got: {:?}", + err.kind() + ); + } + + #[test] + fn test_acl_duplicate_uuid_empty_active_hash() { + let mock = acl_mock_image(Some(ACL_TEST_HASH)); + let os_image = OsImage::mock(mock); + let err = validate_acl_duplicate_uuid( + &os_image, + Path::new("/usr"), + &OsUuid::Uuid(Uuid::parse_str("a1a2a3a4b1b2c1c2d1d2d3d4d5d6d7d8").unwrap()), + Some(String::new()), + ) + .unwrap_err(); + assert!( + matches!( + err.kind(), + ErrorKind::InvalidInput( + InvalidInputError::DuplicateFsUuidAclVerificationFailed { reason, .. } + ) if reason.contains("usrhash=") + ), + "expected empty active hash error, got: {:?}", + err.kind() + ); + } +} use super::*; use std::{path::PathBuf, str::FromStr}; From 8b44b16af8201069a33ff776901e898ecb8ffe81 Mon Sep 17 00:00:00 2001 From: bfjelds Date: Thu, 4 Jun 2026 11:05:39 -0700 Subject: [PATCH 19/22] fix: restore functional_test module header after edit Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/trident/src/subsystems/storage/osimage.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/trident/src/subsystems/storage/osimage.rs b/crates/trident/src/subsystems/storage/osimage.rs index 24125e100..f12fb420f 100644 --- a/crates/trident/src/subsystems/storage/osimage.rs +++ b/crates/trident/src/subsystems/storage/osimage.rs @@ -1616,6 +1616,10 @@ mod tests { ); } } + +#[cfg(feature = "functional-test")] +#[cfg_attr(not(test), allow(unused_imports, dead_code))] +mod functional_test { use super::*; use std::{path::PathBuf, str::FromStr}; From c4be92013fbdc40c3f03c489f1be3f15659e5734 Mon Sep 17 00:00:00 2001 From: bfjelds Date: Thu, 4 Jun 2026 11:06:51 -0700 Subject: [PATCH 20/22] style: rustfmt fixes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/trident/src/subsystems/storage/osimage.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/crates/trident/src/subsystems/storage/osimage.rs b/crates/trident/src/subsystems/storage/osimage.rs index f12fb420f..01d51000b 100644 --- a/crates/trident/src/subsystems/storage/osimage.rs +++ b/crates/trident/src/subsystems/storage/osimage.rs @@ -1455,9 +1455,7 @@ mod tests { images: vec![MockImage { mount_point: PathBuf::from("/usr"), fs_type: OsImageFileSystemType::Ext4, - fs_uuid: OsUuid::Uuid( - Uuid::parse_str("a1a2a3a4b1b2c1c2d1d2d3d4d5d6d7d8").unwrap(), - ), + fs_uuid: OsUuid::Uuid(Uuid::parse_str("a1a2a3a4b1b2c1c2d1d2d3d4d5d6d7d8").unwrap()), part_type: DiscoverablePartitionType::LinuxGeneric, verity: roothash.map(|h| MockVerity { roothash: h.to_string(), @@ -1549,8 +1547,7 @@ mod tests { fn test_acl_duplicate_uuid_mismatched_hash() { let mock = acl_mock_image(Some(ACL_TEST_HASH)); let os_image = OsImage::mock(mock); - let different_hash = - "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"; + let different_hash = "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"; let err = validate_acl_duplicate_uuid( &os_image, Path::new("/usr"), From 5421ae0d251368ebbc00703860db9ff874276715 Mon Sep 17 00:00:00 2001 From: bfjelds Date: Fri, 5 Jun 2026 11:46:27 -0700 Subject: [PATCH 21/22] fix: address deep review findings DR-001 through DR-007 DR-001 (High): Replace if-let with let-else for missing staging hash in detect_acl_btrfs_uuid_collision - None now logs a warning and refuses the bind-mount instead of silently proceeding unverified. DR-002 (High): Replace suffix.contains() with exact suffix equality in cleanup_ukis_before_staging - prevents azla0 from matching azla01.efi in multiboot with 10+ OS instances. DR-003 (Medium): Extract verity_hashes_match() into engine::acl module, replacing duplicated normalize+compare logic in newroot.rs and osimage.rs. Rejects empty hashes so "" == "" cannot incorrectly pass. DR-004 (Medium): Document pre-staging cleanup ordering rationale in esp.rs - explains the crash-safety trade-off (active slot UKI preserved as A/B fallback). DR-005 (Medium): Make remove_uki_and_addons idempotent by treating NotFound as success - prevents orphaned addon dirs if UKI was already removed by a prior partial cleanup. DR-006 (Medium): Document that cleanup_ukis_before_staging is intentionally universal (not ACL-gated) - ESP space constraints apply to all UKI-based A/B updates. DR-007 (Medium): Replace byte-index hash slicing with char-safe hash_preview() using chars().take(16) - prevents panics on non-ASCII input (defense in depth for hex hashes). Adds unit tests for verity_hashes_match(), hash_preview(), cleanup_ukis_before_staging (exact suffix matching, multi-index cleanup), and remove_uki_and_addons (idempotency, addon directory cleanup). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/trident/src/engine/acl.rs | 83 ++++++++++ crates/trident/src/engine/boot/uki.rs | 150 +++++++++++++++++- crates/trident/src/engine/newroot.rs | 55 ++++--- crates/trident/src/subsystems/esp.rs | 8 +- .../trident/src/subsystems/storage/osimage.rs | 13 +- 5 files changed, 270 insertions(+), 39 deletions(-) diff --git a/crates/trident/src/engine/acl.rs b/crates/trident/src/engine/acl.rs index 62b6db3e8..4517bcf36 100644 --- a/crates/trident/src/engine/acl.rs +++ b/crates/trident/src/engine/acl.rs @@ -22,3 +22,86 @@ pub fn read_active_usr_roothash() -> Option { .find_map(|field| field.strip_prefix("usrhash=")) .map(|hash| hash.to_owned()) } + +/// Compares two verity root hashes for equality after trimming whitespace and +/// lowercasing. Returns `false` if either hash is empty (an empty hash is not +/// a valid identity — `"" == ""` would incorrectly pass). +pub fn verity_hashes_match(a: &str, b: &str) -> bool { + let a = a.trim().to_lowercase(); + let b = b.trim().to_lowercase(); + !a.is_empty() && !b.is_empty() && a == b +} + +/// Returns a char-safe preview of a hash string for log messages. +/// Uses `chars().take(16)` instead of byte-index slicing to avoid panics +/// on non-ASCII input (which shouldn't happen for hex hashes, but defense +/// in depth). +pub fn hash_preview(hash: &str) -> String { + hash.trim().to_lowercase().chars().take(16).collect() +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn verity_hashes_match_identical() { + assert!(verity_hashes_match("abc123", "abc123")); + } + + #[test] + fn verity_hashes_match_case_insensitive() { + assert!(verity_hashes_match("ABC123", "abc123")); + assert!(verity_hashes_match("abc123", "ABC123")); + } + + #[test] + fn verity_hashes_match_trims_whitespace() { + assert!(verity_hashes_match(" abc123 ", "abc123")); + assert!(verity_hashes_match("abc123", " abc123\n")); + } + + #[test] + fn verity_hashes_match_rejects_empty() { + assert!(!verity_hashes_match("", "")); + assert!(!verity_hashes_match("abc123", "")); + assert!(!verity_hashes_match("", "abc123")); + } + + #[test] + fn verity_hashes_match_rejects_whitespace_only() { + assert!(!verity_hashes_match(" ", " ")); + assert!(!verity_hashes_match("abc123", " ")); + } + + #[test] + fn verity_hashes_match_rejects_different() { + assert!(!verity_hashes_match("abc123", "def456")); + } + + #[test] + fn hash_preview_truncates_to_16() { + let long_hash = "0123456789abcdef0123456789abcdef"; + assert_eq!(hash_preview(long_hash), "0123456789abcdef"); + } + + #[test] + fn hash_preview_short_hash_unchanged() { + assert_eq!(hash_preview("abc"), "abc"); + } + + #[test] + fn hash_preview_lowercases() { + assert_eq!(hash_preview("ABCDEF"), "abcdef"); + } + + #[test] + fn hash_preview_trims() { + assert_eq!(hash_preview(" abc "), "abc"); + } + + #[test] + fn hash_preview_empty() { + assert_eq!(hash_preview(""), ""); + } +} diff --git a/crates/trident/src/engine/boot/uki.rs b/crates/trident/src/engine/boot/uki.rs index 6a2ab61d3..f6829e07e 100644 --- a/crates/trident/src/engine/boot/uki.rs +++ b/crates/trident/src/engine/boot/uki.rs @@ -182,10 +182,22 @@ pub fn prepare_esp_for_uki(root_mount_point: &Path, esp_mount_path: &Path) -> Re Ok(()) } -/// Removes a UKI file and its associated addon directory if present. +/// Removes a UKI file and its associated addon directory (if present). +/// Treats `NotFound` as success for idempotency — a partially-failed previous +/// cleanup should not prevent the addon directory from being cleaned. fn remove_uki_and_addons(path: &Path) -> Result<(), Error> { - fs::remove_file(path) - .with_context(|| format!("Failed to remove UKI file '{}'", path.display()))?; + match fs::remove_file(path) { + Ok(()) => {} + Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + trace!("UKI file '{}' already removed, continuing", path.display()); + } + Err(e) => { + return Err( + anyhow::Error::new(e) + .context(format!("Failed to remove UKI file '{}'", path.display())), + ); + } + } let addon_dir = uki::uki_addon_dir(path); if addon_dir.exists() && addon_dir.is_dir() { fs::remove_dir_all(&addon_dir).with_context(|| { @@ -199,7 +211,8 @@ fn remove_uki_and_addons(path: &Path) -> Result<(), Error> { } /// Cleans up old UKIs for the target slot before staging a new one, freeing -/// ESP space. +/// ESP space. This runs for all UKI-based A/B updates (not just ACL) because +/// ESP space constraints are a general concern. /// /// When staging a UKI for slot X on OS N, the old slot X/OS N UKI is being /// replaced and can be removed. Other slots and other OS indices are preserved. @@ -222,13 +235,15 @@ pub fn cleanup_ukis_before_staging( } let target_slot = target_slot_os_id(ctx); + let target_suffix = format!("{target_slot}.efi"); let active_slot = active_slot_os_id(ctx); + let active_suffix = format!("{active_slot}.efi"); let trident_ukis = enumerate_trident_managed_ukis(&esp_uki_directory)?; // 1. Remove trident-managed UKIs for the target slot+os-index // (all update indices, e.g. vmlinuz-100-azla0.efi, vmlinuz-102-azla0.efi). for (_index, suffix, path) in &trident_ukis { - if suffix.contains(&target_slot) { + if *suffix == target_suffix { debug!( "Pre-staging cleanup: removing old target-slot UKI '{}'", path.display() @@ -245,7 +260,7 @@ pub fn cleanup_ukis_before_staging( // OS 0's partition refs baked in and can't boot other OS instances. let has_active_slot_uki = trident_ukis .iter() - .any(|(_index, suffix, _)| suffix.contains(&active_slot)); + .any(|(_index, suffix, _)| *suffix == active_suffix); if has_active_slot_uki && ctx.install_index == 0 { let non_trident_ukis = enumerate_non_trident_managed_ukis(&esp_uki_directory)?; @@ -1226,4 +1241,127 @@ mod tests { b"firstboot-data" ); } + + /// Helper: creates an ESP directory structure with UKI files and returns + /// the mock root mount point (tempdir) and the UKI directory path. + fn setup_esp_for_cleanup(ukis: &[&str]) -> (tempfile::TempDir, PathBuf) { + let mount = tempdir().unwrap(); + let esp_uki_dir = + join_relative(mount.path(), DEFAULT_ESP_MOUNT_POINT_PATH).join(UKI_DIRECTORY); + fs::create_dir_all(&esp_uki_dir).unwrap(); + for name in ukis { + File::create(esp_uki_dir.join(name)).unwrap(); + } + (mount, esp_uki_dir) + } + + /// Validates that cleanup removes only UKIs matching the target slot+os-index + /// (exact suffix match), not UKIs for other slots or OS indices. + #[test] + fn test_cleanup_ukis_removes_target_slot_only() { + // Active = VolumeA, install_index = 0 → target = azlb0, active = azla0 + let ctx = make_ctx(Some(AbVolumeSelection::VolumeA), 0); + let (mount, uki_dir) = setup_esp_for_cleanup(&[ + "vmlinuz-100-azlb0.efi", // target slot → should be removed + "vmlinuz-101-azla0.efi", // active slot → should survive + "vmlinuz-102-azlb1.efi", // different OS index → should survive + "vmlinuz-103-azla1.efi", // different OS index → should survive + ]); + + cleanup_ukis_before_staging( + &ctx, + mount.path(), + Path::new(DEFAULT_ESP_MOUNT_POINT_PATH), + ) + .unwrap(); + + assert!(!uki_dir.join("vmlinuz-100-azlb0.efi").exists()); + assert!(uki_dir.join("vmlinuz-101-azla0.efi").exists()); + assert!(uki_dir.join("vmlinuz-102-azlb1.efi").exists()); + assert!(uki_dir.join("vmlinuz-103-azla1.efi").exists()); + } + + /// Validates that suffix matching is exact — `azla0` must not match + /// `azla01.efi` or `azla00.efi` (multiboot with ≥10 instances). + #[test] + fn test_cleanup_ukis_exact_suffix_no_substring_match() { + // Active = VolumeB, install_index = 0 → target = azla0, active = azlb0 + let ctx = make_ctx(Some(AbVolumeSelection::VolumeB), 0); + let (mount, uki_dir) = setup_esp_for_cleanup(&[ + "vmlinuz-100-azla0.efi", // target → should be removed + "vmlinuz-101-azla01.efi", // OS index 01, NOT 0 → should survive + "vmlinuz-102-azla00.efi", // OS index 00, NOT 0 → should survive + "vmlinuz-103-azla10.efi", // OS index 10 → should survive + ]); + + cleanup_ukis_before_staging( + &ctx, + mount.path(), + Path::new(DEFAULT_ESP_MOUNT_POINT_PATH), + ) + .unwrap(); + + assert!(!uki_dir.join("vmlinuz-100-azla0.efi").exists()); + assert!(uki_dir.join("vmlinuz-101-azla01.efi").exists()); + assert!(uki_dir.join("vmlinuz-102-azla00.efi").exists()); + assert!(uki_dir.join("vmlinuz-103-azla10.efi").exists()); + } + + /// Validates that cleanup removes multiple UKIs with different update + /// indices but the same target slot+os-index. + #[test] + fn test_cleanup_ukis_removes_all_target_update_indices() { + let ctx = make_ctx(Some(AbVolumeSelection::VolumeA), 0); + let (mount, uki_dir) = setup_esp_for_cleanup(&[ + "vmlinuz-100-azlb0.efi", // target → remove + "vmlinuz-102-azlb0.efi", // target → remove + "vmlinuz-104-azlb0.efi", // target → remove + "vmlinuz-101-azla0.efi", // active → keep + ]); + + cleanup_ukis_before_staging( + &ctx, + mount.path(), + Path::new(DEFAULT_ESP_MOUNT_POINT_PATH), + ) + .unwrap(); + + assert!(!uki_dir.join("vmlinuz-100-azlb0.efi").exists()); + assert!(!uki_dir.join("vmlinuz-102-azlb0.efi").exists()); + assert!(!uki_dir.join("vmlinuz-104-azlb0.efi").exists()); + assert!(uki_dir.join("vmlinuz-101-azla0.efi").exists()); + } + + /// Validates that `remove_uki_and_addons` is idempotent — removing a + /// file that no longer exists succeeds rather than returning an error. + #[test] + fn test_remove_uki_and_addons_idempotent() { + let dir = tempdir().unwrap(); + let uki_path = dir.path().join("vmlinuz-100-azla0.efi"); + File::create(&uki_path).unwrap(); + + // First removal should succeed + remove_uki_and_addons(&uki_path).unwrap(); + assert!(!uki_path.exists()); + + // Second removal should also succeed (idempotent) + remove_uki_and_addons(&uki_path).unwrap(); + } + + /// Validates that `remove_uki_and_addons` removes both the UKI and its + /// addon directory, and succeeds when the UKI file is already gone but + /// the addon directory still exists. + #[test] + fn test_remove_uki_and_addons_with_addon_dir() { + let dir = tempdir().unwrap(); + let uki_path = dir.path().join("vmlinuz-100-azla0.efi"); + File::create(&uki_path).unwrap(); + let addon_dir = uki::uki_addon_dir(&uki_path); + fs::create_dir_all(&addon_dir).unwrap(); + fs::write(addon_dir.join("cmdline.addon.efi"), b"addon").unwrap(); + + remove_uki_and_addons(&uki_path).unwrap(); + assert!(!uki_path.exists()); + assert!(!addon_dir.exists()); + } } diff --git a/crates/trident/src/engine/newroot.rs b/crates/trident/src/engine/newroot.rs index f03ab60d8..d0b43b825 100644 --- a/crates/trident/src/engine/newroot.rs +++ b/crates/trident/src/engine/newroot.rs @@ -441,35 +441,42 @@ fn detect_acl_btrfs_uuid_collision( // When a staging root hash is available, verify that the active USR // partition has the same verity root hash. This provides a cryptographic // guarantee that the filesystems are byte-identical, not just a UUID match. - if let Some(staging_hash) = staging_usr_roothash { - match acl::read_active_usr_roothash() { - Some(active_hash) => { - let staging_normalized = staging_hash.trim().to_lowercase(); - let active_normalized = active_hash.trim().to_lowercase(); - if staging_normalized == active_normalized { - debug!( - "Verity root hash verification passed: active and staging USR \ - partitions have matching root hash ({}...)", - &staging_normalized[..staging_normalized.len().min(16)] - ); - } else { - warn!( - "Verity root hash mismatch: active USR has '{}...', staging has '{}...'. \ - Refusing bind-mount despite UUID collision.", - &active_normalized[..active_normalized.len().min(16)], - &staging_normalized[..staging_normalized.len().min(16)] - ); - return None; - } - } - None => { + let Some(staging_hash) = staging_usr_roothash else { + // No staging hash available — cannot verify content identity. + // Refusing the bind-mount is the safe choice: proceeding without + // verification could mount different content at /usr. + warn!( + "No staging USR verity root hash provided. \ + Refusing bind-mount — cannot verify content identity." + ); + return None; + }; + + match acl::read_active_usr_roothash() { + Some(active_hash) => { + if acl::verity_hashes_match(staging_hash, &active_hash) { + debug!( + "Verity root hash verification passed: active and staging USR \ + partitions have matching root hash ({}...)", + acl::hash_preview(staging_hash) + ); + } else { warn!( - "Cannot read active USR verity root hash from /proc/cmdline. \ - Refusing bind-mount despite UUID collision." + "Verity root hash mismatch: active USR has '{}...', staging has '{}...'. \ + Refusing bind-mount despite UUID collision.", + acl::hash_preview(&active_hash), + acl::hash_preview(staging_hash) ); return None; } } + None => { + warn!( + "Cannot read active USR verity root hash from /proc/cmdline. \ + Refusing bind-mount despite UUID collision." + ); + return None; + } } Some(active_uuid) diff --git a/crates/trident/src/subsystems/esp.rs b/crates/trident/src/subsystems/esp.rs index 177eb6fec..2b4ac9324 100644 --- a/crates/trident/src/subsystems/esp.rs +++ b/crates/trident/src/subsystems/esp.rs @@ -288,8 +288,12 @@ fn copy_file_artifacts( // Prepare ESP directory structure for UKI boot uki::prepare_esp_for_uki(mount_point, &ctx.esp_mount_path)?; - // Clean up old UKIs for the target slot to free ESP space before - // staging the new UKI. + // Clean up old UKIs for the target slot before staging the new one. + // Pre-staging cleanup is necessary because the ESP may not have space + // for both old and new UKIs simultaneously (128 MB constraint). The + // trade-off: a crash between cleanup and staging removes the target + // slot's old UKI with no replacement, but the active slot's UKI is + // preserved so the system remains bootable via A/B fallback. uki::cleanup_ukis_before_staging(ctx, mount_point, &ctx.esp_mount_path)?; // Copy the UKI from the image into the ESP directory diff --git a/crates/trident/src/subsystems/storage/osimage.rs b/crates/trident/src/subsystems/storage/osimage.rs index 01d51000b..de9e6ea80 100644 --- a/crates/trident/src/subsystems/storage/osimage.rs +++ b/crates/trident/src/subsystems/storage/osimage.rs @@ -293,7 +293,8 @@ fn validate_acl_duplicate_uuid( fs_uuid: &OsUuid, active_usr_roothash: Option, ) -> Result<(), TridentError> { - use crate::engine::acl::{ACL_USR_A_PARTUUID, ACL_USR_B_PARTUUID}; + use crate::engine::acl; + use acl::{ACL_USR_A_PARTUUID, ACL_USR_B_PARTUUID}; let mount_str = mount_point.to_string_lossy(); let uuid_str = fs_uuid.to_string(); @@ -349,17 +350,15 @@ fn validate_acl_duplicate_uuid( }; // 4. Hashes must match (case-insensitive, trimmed). - let staging_normalized = staging_hash.trim().to_lowercase(); - let active_normalized = active_hash.trim().to_lowercase(); - if staging_normalized != active_normalized { + if !acl::verity_hashes_match(staging_hash, &active_hash) { return Err(TridentError::new( InvalidInputError::DuplicateFsUuidAclVerificationFailed { uuid: uuid_str, mount_point: mount_str.to_string(), reason: format!( "verity root hash mismatch: staging has '{}...', active has '{}...'", - &staging_normalized[..staging_normalized.len().min(16)], - &active_normalized[..active_normalized.len().min(16)] + acl::hash_preview(staging_hash), + acl::hash_preview(&active_hash) ), }, )); @@ -368,7 +367,7 @@ fn validate_acl_duplicate_uuid( debug!( "ACL duplicate FS UUID '{}' on /usr is safe: verity root hashes match ({}...)", uuid_str, - &staging_normalized[..staging_normalized.len().min(16)] + acl::hash_preview(staging_hash) ); // Optional: If COSI partition metadata is available, validate that the staging From 195956fcbd99f3c68440974381b7cd72ba26b466 Mon Sep 17 00:00:00 2001 From: bfjelds Date: Fri, 5 Jun 2026 11:54:32 -0700 Subject: [PATCH 22/22] style: rustfmt fixes in uki.rs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/trident/src/engine/boot/uki.rs | 30 +++++++-------------------- 1 file changed, 8 insertions(+), 22 deletions(-) diff --git a/crates/trident/src/engine/boot/uki.rs b/crates/trident/src/engine/boot/uki.rs index f6829e07e..394000e24 100644 --- a/crates/trident/src/engine/boot/uki.rs +++ b/crates/trident/src/engine/boot/uki.rs @@ -192,10 +192,8 @@ fn remove_uki_and_addons(path: &Path) -> Result<(), Error> { trace!("UKI file '{}' already removed, continuing", path.display()); } Err(e) => { - return Err( - anyhow::Error::new(e) - .context(format!("Failed to remove UKI file '{}'", path.display())), - ); + return Err(anyhow::Error::new(e) + .context(format!("Failed to remove UKI file '{}'", path.display()))); } } let addon_dir = uki::uki_addon_dir(path); @@ -1268,12 +1266,8 @@ mod tests { "vmlinuz-103-azla1.efi", // different OS index → should survive ]); - cleanup_ukis_before_staging( - &ctx, - mount.path(), - Path::new(DEFAULT_ESP_MOUNT_POINT_PATH), - ) - .unwrap(); + cleanup_ukis_before_staging(&ctx, mount.path(), Path::new(DEFAULT_ESP_MOUNT_POINT_PATH)) + .unwrap(); assert!(!uki_dir.join("vmlinuz-100-azlb0.efi").exists()); assert!(uki_dir.join("vmlinuz-101-azla0.efi").exists()); @@ -1294,12 +1288,8 @@ mod tests { "vmlinuz-103-azla10.efi", // OS index 10 → should survive ]); - cleanup_ukis_before_staging( - &ctx, - mount.path(), - Path::new(DEFAULT_ESP_MOUNT_POINT_PATH), - ) - .unwrap(); + cleanup_ukis_before_staging(&ctx, mount.path(), Path::new(DEFAULT_ESP_MOUNT_POINT_PATH)) + .unwrap(); assert!(!uki_dir.join("vmlinuz-100-azla0.efi").exists()); assert!(uki_dir.join("vmlinuz-101-azla01.efi").exists()); @@ -1319,12 +1309,8 @@ mod tests { "vmlinuz-101-azla0.efi", // active → keep ]); - cleanup_ukis_before_staging( - &ctx, - mount.path(), - Path::new(DEFAULT_ESP_MOUNT_POINT_PATH), - ) - .unwrap(); + cleanup_ukis_before_staging(&ctx, mount.path(), Path::new(DEFAULT_ESP_MOUNT_POINT_PATH)) + .unwrap(); assert!(!uki_dir.join("vmlinuz-100-azlb0.efi").exists()); assert!(!uki_dir.join("vmlinuz-102-azlb0.efi").exists());