-
Notifications
You must be signed in to change notification settings - Fork 594
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
boot,bootloader: add support for shim fallback and setting EFI boot variables on install #13205
boot,bootloader: add support for shim fallback and setting EFI boot variables on install #13205
Conversation
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## master #13205 +/- ##
==========================================
- Coverage 78.81% 78.80% -0.01%
==========================================
Files 1020 1021 +1
Lines 127158 127308 +150
==========================================
+ Hits 100214 100321 +107
- Misses 20670 20701 +31
- Partials 6274 6286 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes 📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
fb65a92
to
3f258f3
Compare
The fallback grub EFI asset is already in EFI/ubuntu/, but the non-fallback grub asset should be in EFI/BOOT/ as well. The sources for these grub assets are identical. This corresponds to work in snapd PR #13205. See the following for more details: - canonical/snapd#13205 Signed-off-by: Oliver Calder <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a question about the connection between the shim fallback path, the recovery boot chain, and trusted assets in general. Perhaps @alfonsosanchezbeato or @valentindavid could help provide some insight? Thanks!
I still need to add unit tests and spread tests, but the logic is here and I would appreciate getting the review process started. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nice. I have some some comments. I am also wondering how this would work in different scenarios:
- In most cases, a UC image is flashed to a system to the expected disk target. In that case, with the proposed changes to the gadget,
fbx64.efi
would automatically create the needed boot entry in EFI NVRAM, so the code for handling the variables would not be used, at least in normal cases. - The second scenario is for hybrid images. There, either subiquity or snapd (via installer API) will need to set appropriately the EFI boot entry. Iirc it was decided that snaps would do this, maybe @pedronis could confirm. But I'm not sure if the glue code for this is already here.
Other things that come to my mind:
- Should we be follow the same algorithm as fallback.efi from shim (https://github.com/rhboot/shim/blob/main/fallback.c) when creating the variables, for consistency?
- In the boot partition we have EFI/boot/grubx64.efi, maybe we should move it to a different path now.
1ed9dc6
to
e96d6f3
Compare
case "${ARCH}" in | ||
x64 ) ;; | ||
aa64 ) ;; | ||
* ) ERROR "Invalid architecture '${ARCH}': must be 'x64' or 'aa64'" ;; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we test for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just want to make sure the ARCH
variable was set correctly, otherwise later parts of the test fail in not fun to debug ways. But it's true, maybe a test -n $ARCH
would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, if there's a better way to compute the ARCH string used in EFI assets (i.e. the x64
in shimx64.efi
), that would be even better. In addition to being set, I also wanted to make sure that the logic which sets the ARCH
variable was successful.
I've added a simpler spread test that invokes directly the code which sets EFI boot variables. This test passes locally, so it seems the remaining problems with the original nested test relate to if/when/how the code is invoked during the installation process. I am essentially out of ideas as to why the variables are not being set in install mode. Any guidance from @alfonsosanchezbeato @valentindavid or @pedronis would be greatly appreciated, so we can try to get this landed before the sprint. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a remark. Also, a TODO about changing EFI variables when a gadget update happens should be added, maybe in update.go.
I believe the latest changes should finally fix the problems of the EFI variable-setting code not running. The MacOS quick checks still need to be fixed, since In the future, an additional PR should be opened to set EFI boot variables in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments, but overall it is in the good direction I'd say
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial pass of PR, overall looks super nice!
boot/makebootable.go
Outdated
opts = &bootloader.Options{ | ||
Role: bootloader.RoleRecovery, | ||
} | ||
// Set EFI boot variables according to bootloader on ubuntu-seed | ||
seedBl, err := bootloader.Find(InitramfsUbuntuSeedDir, opts) | ||
if err != nil { | ||
logger.Debugf("WARNING: cannot find bootloader in seed directory, skipping setting EFI variables") | ||
return nil | ||
} | ||
if ubl, ok := seedBl.(bootloader.UefiBootloader); ok { | ||
description, assetPath, optionalData, err := ubl.EfiLoadOptionParameters() | ||
if err != nil { | ||
logger.Debugf("WARNING: cannot get EFI load option parameter: %v", err) | ||
return nil | ||
} | ||
err = SetEfiBootVariables(description, assetPath, optionalData) | ||
if err != nil { | ||
logger.Debugf("WARNING: failed to set EFI boot variables: %v", err) | ||
return nil | ||
} | ||
} else { | ||
logger.Debugf("seed bootloader does not support setting EFI boot variables; skipping") | ||
} | ||
|
||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: could this be moved to a seperate function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I like this idea. I've moved it into a new function SetUbuntuSeedEfiBootVariables
(not thrilled about the name) in setefibootvars.go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes! I have a some additional comments now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for all the changes! I have a minor comment if you want to address.
boot/setefibootvars.go
Outdated
|
||
// SetUbuntuSeedEfiBootVariables sets EFI variables according to the bootloader | ||
// found on ubuntu seed if it is a UefiBootloader. | ||
func SetUbuntuSeedEfiBootVariables() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function does not need to be exported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks! I much prefer this to be unexported, with just the more general SetEfiBootVariables
exported.
I still need to fix MacOS quick checks before this can be merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes, lgtm.
When gadget uses shim fallback mode, the trusted assets chain is different. Add support to detect that. LP: #1962182 Signed-off-by: Dimitri John Ledkov <[email protected]> Signed-off-by: Oliver Calder <[email protected]>
8ea377d
to
9d7985d
Compare
Signed-off-by: Oliver Calder <[email protected]> boot: renamed trustedShimFallbackBinary to seedShimPath Signed-off-by: Oliver Calder <[email protected]> boot: refactored setting EFI boot variables at install Signed-off-by: Oliver Calder <[email protected]> boot: adjusted variable names and fixed variable initialization Signed-off-by: Oliver Calder <[email protected]> boot: improve setting Boot#### EFI variable Notably, splits off the process of reading a Boot#### variable and extracting its DevicePath into its own function `readBootVariable` which can be mocked and otherwise simplifies the `setBootNumberVariable` function. Also, fixes behavior around the final BootFFFF variable. Previously, it was not possible to select the BootFFFF variable if it was unused, due to overflow concerns on uint16. Now, the behavior around BootFFFF is identical to that of any other boot variable, by using an int internally instead of uint16, which also allows a more robust check for whether there were no matching variables. Signed-off-by: Oliver Calder <[email protected]> boot: added unit tests for setting EFI Boot#### variable Signed-off-by: Oliver Calder <[email protected]> boot: refactored setting EFI boot variables Rewrote EFI boot variable functions to more closely match the behavior of shim fallback: https://github.com/rhboot/shim/blob/main/fallback.c In particular, the following have changed: 1. Existing Boot#### variables must fully match the new load option to be considered a match. In particular, the load option attributes, label, and device path must all be byte-for-byte identical. Previously, only the device paths were compared. 2. Matching Boot#### variables are no longer overwritten. Since the variable data must now byte-for-byte match the new load option, there is no need to overwrite the existing variable. 3. Since existing Boot#### variables are no longer overwritten, the variable attributes are no longer checked for those variables. Instead, it is assumed that the Boot#### variable attributes are viable for it to be used as a boot option. This matches the behavior of `rhboot/shim/fallback.c`, for better or for worse. 4. When modifying the BootOrder variable, boot option numbers are no longer pruned if there is no matching Boot#### variable. Signed-off-by: Oliver Calder <[email protected]> boot,bootloader: introduce UefiBootloader to build EFI load options Previously, the path of the shim binary relative to the EFI partition was passed into `SetEfiBootVariables`. However, different bootloaders may wish to set up `OptionalData` in the load option. Additionally, not all `TrustedAssetBootloaders` will attempt to set EFI boot variables, and not all bootloaders which should set EFI boot variables necessarily support secure boot. Thus, these should be decoupled. This commit adds a new `UefiBootloader` interface with the `ConstructShimEfiLoadOption` method, which builds an EFI load option from the shim path for the given bootloader. Signed-off-by: Oliver Calder <[email protected]> boot,bootloader: fixed linting errors and improved EFI boot variable test clarity Signed-off-by: Oliver Calder <[email protected]> bootloader: improved unit test for grub EFI load option creation Signed-off-by: Oliver Calder <[email protected]> boot: set EFI boot variables in `MakeRunnableSystem` Previously, attempted to set boot variables in `MakeRecoverySystemBootable`, which is called by `MakeBootableImage`, which is called when building the image file, rather than during install mode. `MakeRunnableSystem` is called on first boot during install mode, and thus should be responsible for setting EFI boot variables. Signed-off-by: Oliver Calder <[email protected]> boot: use seed bootloader when setting EFI variables In install mode, the bootloader located in ubuntu-seed should be used when setting the EFI boot variables. Previously, the bootloader in ubuntu-boot was accidentally re-used. Signed-off-by: Oliver Calder <[email protected]> tests: added simple test to execute setefibootvar.go code Signed-off-by: Oliver Calder <[email protected]> tests: fixed standalone set EFI vars code test to work with different layouts Signed-off-by: Oliver Calder <[email protected]> tests: moved simple setefibootvar.go check to nested test Signed-off-by: Oliver Calder <[email protected]> tests: added check for idempotence when setting EFI boot variables Signed-off-by: Oliver Calder <[email protected]> bootloader: adjust comments, organization, and add TODO Signed-off-by: Oliver Calder <[email protected]> boot,bootloader: fix setting EFI boot variables Make function to search for EFI asset device path and construct load option common so each UefiBootloader does not have to re-implement it. Instead, the bootloader returns the description, asset file path, and optional data, which can then be used to create the EFI load option. Also, in `makeRunnableSystem`, the bootloader in ubuntu-seed must have `NoSlashBoot` in order to correctly find the grub.cfg file and thus the grub bootloader. This commit fixes this bug, and refactors a bit to account for the changes in responsibilities between the bootloader and the setefibootvars.go code. Signed-off-by: Oliver Calder <[email protected]> bootloader: fixed grub EFI load option test with tmp rootdir Signed-off-by: Oliver Calder <[email protected]> go.mod: move golang.org/x/text import next to other golang.org/x/ imports Signed-off-by: Oliver Calder <[email protected]> boot: adjust opts to look for recovery bootloader when setting EFI variables Signed-off-by: Oliver Calder <[email protected]> boot: do not overwrite BootOrder if unchanged, and unexport EFI variable helper functions Signed-off-by: Oliver Calder <[email protected]> boot: unexport `setEfiBootOrderVariable` Signed-off-by: Oliver Calder <[email protected]> boot: move code to detect bootloader and set EFI variables accordingly into dedicated function Signed-off-by: Oliver Calder <[email protected]> boot: unexport `setUbuntuSeedEfiBootVariables` and accompanying error Signed-off-by: Oliver Calder <[email protected]> boot,bootloader: ensure nil optionalData for EFI variable is equivalent to 0-length slice Signed-off-by: Oliver Calder <[email protected]> boot: handle empty boot order and other boot var improvements Signed-off-by: Oliver Calder <[email protected]> boot: make setefibootvars functions linux-only Signed-off-by: Oliver Calder <[email protected]>
The test checks that EFI boot variables exist for the following: 1. A Boot#### variable pointing to the shim file path. 2. A BootOrder variable with the #### from the above Boot#### as first. Since the layout of EFI assets is dependent on the gadget snap, the test downloads and unpacks the gadget, then modifies the contents so that one variant has the shim and grub binaries in `EFI/boot/` and another variant has the shim and grub binaries in `EFI/ubuntu/` and the fallback binary in `EFI/boot/`. After building a core image around that modified gadget, the VM is booted and the test checks that the EFI variables are set correctly. Then, the test modifies the gadget to match the other variant's initial layout, and then installs the newly modified gadget. This should trigger re-setting EFI boot variables as well. Signed-off-by: Oliver Calder <[email protected]> tests: fix problems in spread test for setting EFI boot variables Signed-off-by: Oliver Calder <[email protected]> tests: disabled TPM on EFI boot vars test and separated gadget script Signed-off-by: Oliver Calder <[email protected]> tests: fixed EFI vars test to use correct toolbox and include all EFI assets Signed-off-by: Oliver Calder <[email protected]> tests: modify-gadget.sh re-use existing gadget so edition is incremented Signed-off-by: Oliver Calder <[email protected]> tests: fix mangled EFI var search string and other improvements Signed-off-by: Oliver Calder <[email protected]> tests: polish tests for setting EFI boot variables Notably, allow tests/nested/core/core20-set-efi-boot-variables to run on arm64 as well as amd64, simplify setefivars.go to search for multiple assets on multiple architectures, and allow tests/nested/manual/core20-set-efi-boot-vars to run on any ubuntu-2*. Signed-off-by: Oliver Calder <[email protected]>
Signed-off-by: Oliver Calder <[email protected]>
9d7985d
to
ba950c1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some remarks and questions
@@ -532,7 +541,10 @@ func (g *grub) getGrubRecoveryModeTrustedAssets() ([]string, error) { | |||
if err != nil { | |||
return nil, err | |||
} | |||
return []string{assets.shimBinary, assets.grubBinary}, nil | |||
if osutil.FileExists(filepath.Join(g.rootdir, assets.fallbackBinary)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens during installation or refresh with this condition? does it change from false to true at a reasonable point? should it based on what is in the rootdir or what is in the gadget? should that vary if the bootloader was produced via Find vs ForGadget?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In doInstall
the assets are updated before the resealing happens:
https://github.com/snapcore/snapd/blob/a90520700167916c204872c93ec74a14c517af86/overlord/snapstate/snapstate.go#L548-L563
("update-gadget-assets" before "update-gadget-cmdline") so in principle the file should be there already before resealing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the confusing thing is that it is already broken. Because we call NewBootFile("", ta, RoleRecovery)
and NewBootFile("", ta, RoleRunMode)
, we probably do not do a proper first reseal (the one with all bootchains).
We do pass the kernel path to BootChain
and RecoveryBootChain
. But not the gadget. So we are missing some information here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, what is exactly already broken? if the first sealing was broken we would not be able to boot the first time? at installation time the gadget assets reach the seal code via the TrustedAssetsInstallObserver that is then passed to MakeRunnableSystem but you are probably referring to something different but I'm not sure what
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
during a refresh/ assets update the initial reseal is done via TrustedAssetsUpdateObserver and then a final seal is done after reboot I think, via observeSuccessfulBootAssets in the boot package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why you can't push, at the bottom it says it's allowed. Anyway should I look at reviewing #13398? that one seems already very big though, so I wouldn't push more there but start something on top if neeeded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is commit cb88e79
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have rebased that commit to master in #13412. I will make sure the tests are passing. And I will ping you then.
But there will be some conflict resolution needed in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logger.Debugf("%v", err) | ||
} else if err != nil { | ||
logger.Debugf("WARNING: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the decision to only log errors here?
@@ -558,6 +558,14 @@ func makeRunnableSystem(model *asserts.Model, bootWith *BootableSet, sealer *Tru | |||
if err := MarkRecoveryCapableSystem(recoverySystemLabel); err != nil { | |||
return fmt.Errorf("cannot record %q as a recovery capable system: %v", recoverySystemLabel, err) | |||
} | |||
|
|||
err = setUbuntuSeedEfiBootVariables() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be closer to where we call bootloader.InstallBootConfig
This PR is superseded by #13511. Closing. @olivercalder @valentindavid |
We want to boot Ubuntu Core from
EFI/ubuntu/shim*.efi
, setup fallback boot properly, and manually set EFI boot variables on install so that the correct efi asset is used without going through a long multi-fallback boot process.This relates to the following changes in the pc-gadget snap:
This is meant to be a revival/replacement of the following existing snapd PRs: