Repository navigation
Remove kernel cmdline hardcoding and enable PCR calculations for UKI - #745
piyush-jena wants to merge 4 commits into
Conversation
74fa987 to
0753837
Compare
0753837 to
d9dacab
Compare
d9dacab to
270041f
Compare
de92863 to
d8a62bd
Compare
540c49c to
27d7dc2
Compare
jmt-lab
left a comment
There was a problem hiding this comment.
Can we do a pass for your AI to remove narrative comments, its not that bad right now but i see a few places where it is more narrative then helpful
| merged.entry(name).or_default().extend( | ||
| value | ||
| .split(',') | ||
| .map(str::trim) | ||
| .filter(|value| !value.is_empty()) | ||
| .map(str::to_string), | ||
| ); | ||
| } | ||
|
|
||
| // The merged value is quoted because an unquoted comma-separated value is a | ||
| // bootconfig array, which the kernel expands back into one command-line | ||
| // assignment per value. | ||
| Ok(merged | ||
| .into_iter() | ||
| .map(|(key, values)| format!("{key} = \"{}\"\n", Vec::from_iter(values).join(","))) |
There was a problem hiding this comment.
[P1] Don't split, sort and dedupe the inside of bootconfig values when merging drop-ins
merge_parameters splits every value on ,, trims each piece, puts the pieces in a BTreeSet (sorted, deduplicated) and joins them back into one quoted value. That changes what the kernel sees for any value where commas or order matter. rpm2img now runs ukisys merge-bootconfig for every image, GRUB images included, so this affects both boot paths:
kernel.console = ttyS0,115200n8becomeskernel.console = "115200n8,ttyS0", so the kernel getsconsole=115200n8,ttyS0and the serial console breaks.kernel.console = tty0, ttyS0(an array, which the rpm2img grep check allows) used to expand toconsole=tty0 console=ttyS0. It now becomes the single argumentconsole=tty0,ttyS0, which means devicetty0with optionsttyS0. Thean_array_becomes_one_valuetest locks this in, while the ukisys testcmdline_joins_bootconfig_with_image_builder_arguments(no merge step) expects the expanded form, so the two code paths disagree.- Values where order matters, such as
lsm=, get alphabetised. Values with empty fields, likea,,bor dm-style tables, lose those fields.
Suggested fix: merge only identical repeated assignments, or keep each contribution's raw value as an array element in first-seen order (emit key = v1, v2, so the kernel gets one argument per contribution). Don't re-split inside a quoted value. If comma-joining is really intended for list-valued keys like module_blacklist, limit it to an explicit allow-list of those keys and keep the original order.
There was a problem hiding this comment.
I don't have scope to know if this is a real finding or not, let me know if not
| # (for reading grub.cfg/vmlinuz), but UKI images use a FAT BOOT-A partition, | ||
| # so PCR prediction would fail here. | ||
| if [[ "${UEFI_SECURE_BOOT}" == "yes" && "${UKI_IMAGE}" == "no" ]]; then | ||
| if [[ "${UEFI_SECURE_BOOT}" == "yes" ]]; then |
There was a problem hiding this comment.
[P1] Gate pcrsys on platform for UKI images, or non-AWS secure-boot UKI builds now fail
This hunk drops the UKI_IMAGE == no guard, so pcrsys disk now runs for every secure-boot image, UKI images included. The step can't fail quietly: stdout is redirected into pcr-predictions.json, and rpm2img runs with errexit. On the UKI path, predict_pcrs in tools/pcrsys/src/main.rs bails with ensure_whatever!(platform == Platform::Aws, "direct UKI prediction supports AWS only"). Neither rpm2img nor img2img limits --with-uki-image=yes to AWS. So a metal or vmware variant built with UKI_IMAGE=yes and UEFI_SECURE_BOOT=yes, which used to build (prediction was skipped), now fails at the end of the build.
The same change is in twoliter/embedded/img2img (~line 679).
Suggested fix: keep skipping prediction for non-AWS UKI images, e.g. if [[ "${UEFI_SECURE_BOOT}" == "yes" && ( "${UKI_IMAGE}" == "no" || "${VARIANT_PLATFORM,,}" == "aws" ) ]]. Alternatively, reject UKI_IMAGE=yes for non-AWS platforms up front with a clear error.
| //! ukisys: constructs kernel commandline for UKIs, derives the unsigned | ||
| //! systemd-stub PE a Bottlerocket UKI was built from, by stripping its | ||
| //! Authenticode signature and truncating the trailing payload sections. | ||
| //! Those sections are `.osrel`, `.cmdline`, `.uname`, and `.linux`. |
There was a problem hiding this comment.
[P3] Remove the duplicated crate doc paragraph
The new module doc text (lines 5-8) was added next to the old paragraph (lines 1-4) instead of replacing it, so rustdoc now shows "ukisys: ..." twice with nearly the same text. Delete lines 1-4 and keep the new wording that mentions command-line construction.
27d7dc2 to
bb75694
Compare
Add a bootconfig crate for the image tools to share: - Decode binary bootconfig.data and parse bootconfig text. - Merge the module_blacklist and initcall_blacklist values that packages contribute in separate drop-ins into one value each. - Compose the command line embedded in a UKI, rejecting lines that collapsing whitespace would change, unpaired quotes, and lines too long for the kernel. - Parse grub.cfg, and predict the command line GRUB starts the kernel with and the one the kernel then reports. The parsers start from pcrsys's bootconfig and grub.cfg grammars. Signed-off-by: Piyush Jena <jepiyush@amazon.com>
UKI images carried hard-coded systemd and FIPS tokens in place of bootconfig. Build their command line from the image's own bootconfig instead: - `ukisys merge-bootconfig` combines the drop-ins' module_blacklist and initcall_blacklist values, so `bootconfig -a` accepts them. - `ukisys cmdline` composes the command line from bootconfig.data and the image's base kernel and init arguments. - rpm2img assembles bootconfig before the UKI, passes the composed command line to ukify, and shares the base arguments with grub.cfg. Drop uki_bootconfig_cmdline from imghelper, along with its test. Signed-off-by: Piyush Jena <jepiyush@amazon.com>
Predict PCRs for UKI images that the firmware boots directly, on AWS: - PCR 4 measures the UKI's Authenticode digest. - PCR 5 has one state, since no BOOT partition attributes change. - PCRs 7 and 14 carry no shim measurements. - PCR 9 measures the LoadOptions and the os-release initrd that systemd-stub generates. - PCR 11 measures the UKI's sections and the boot phases. Also: - Read the partition table and the boot files once, into a model of the disk image. - Parse grub.cfg and bootconfig with the bootconfig crate. - On aarch64, GRUB starts the kernel through its PE entry point, whose EFI stub measures GRUB's LoadOptions into PCR 9 before rottweiler extends /proc/cmdline. Predict that event too. - Reject GPT headers that claim more than 128 partition entries, before gptman reserves memory for every entry. Signed-off-by: Piyush Jena <jepiyush@amazon.com>
rpm2img and img2img skipped PCR prediction for UKI images. pcrsys now predicts direct UKI boot on AWS, so run it for AWS UKI images too. UKI images for other platforms are still skipped. Signed-off-by: Piyush Jena <jepiyush@amazon.com>
bb75694 to
4d0e93d
Compare
Description of changes:
bootconfiglibrary which provides support functions for the followingbootconfig.dataand construct kernel command line for UKI images.cmdlinesection in the PE image of UKIgrub.cfgto read grub configpcrsysukisysmerge-bootconfigcmdlinerpm2img: the build path
rpm2img sets up
BOOTCONFIG_DIR, installs RPMs intoROOT_MOUNT/BOOT_MOUNT, builds the root filesystem, then callsgenerate_verity_rootfrom imghelper.Next comes the block the middle commit moved
/boot/boot-config.ddrop-ins shipped by packages are validated (single-line key = value only), concatenated, sorted, passed throughukisys merge-bootconfig. This callsbootconfig::merge_parameterswhich collapses duplicate keys into one sorted quoted value.is collapsed to
This is compiled to binary
bootconfig.databy the kernel'sbootconfig -a. In case of UKI images,ukisys cmdlinereads that binary data and kernel and init args and writes a file;ukify build --cmdline "@${file}". This data is embedded into the UKI in.cmdlinesection.img2img: the repack path
img2img decompresses an existing image, re-extracts the partitions, optionally replaces root contents, and then mirrors rpm2img's ordering: it reads EFI-A's
PARTUUIDcalls the samegenerate_verity_rootto get a freshDM_VERITY_ROOT. It never regenerates bootconfig. For GRUB images Section 6 just seds the new verity table intogrub.cfgand re-signs it. For UKI images it callsrepack_uki:ukify inspectextracts.linux, .cmdline, .osrel, .uname, .sbat;ukisys derive-stubstrips the Authenticode signature and the trailing payload sections to recover a stub ukify build will accept; the old.cmdline` is string-surgeried so only the dm-mod.create="..." value changes (the literal quotes are matched deliberately, since the table contains spaces); the UKI is rebuilt, re-signed with sign_uki, verified section-by-section.pcrsys:
pcrsys disk --image "${OS_IMAGE}" --efi-vars ... --platform ...
emitspcr-predictions.json`. It reads the real disk rather than trusting build variables and generates PCR predictions.Some refactoring was done to move certain functions to more logical locations. They are detailed below:
tools/bootconfig/src/parsers/bootconfig.pest
tools/bootconfig/src/parsers/grub.pest
tools/bootconfig/src/parsers/grub.rs
tools/bootconfig/src/parsers/bootconfig.rs
tools/bootconfig/src/grub_cmdline.rs
tools/bootconfig/src/params.rs
Testing done:
Tested alongside bottlerocket-os/bottlerocket-core-kit#1076
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.