boot/grub.go: return all possible boot chains#13398
Closed
valentindavid wants to merge 6 commits intocanonical:masterfrom
Closed
boot/grub.go: return all possible boot chains#13398valentindavid wants to merge 6 commits intocanonical:masterfrom
valentindavid wants to merge 6 commits intocanonical:masterfrom
Conversation
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 <dimitri.ledkov@canonical.com> Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
Signed-off-by: Oliver Calder <oliver.calder@canonical.com> boot: renamed trustedShimFallbackBinary to seedShimPath Signed-off-by: Oliver Calder <oliver.calder@canonical.com> boot: refactored setting EFI boot variables at install Signed-off-by: Oliver Calder <oliver.calder@canonical.com> boot: adjusted variable names and fixed variable initialization Signed-off-by: Oliver Calder <oliver.calder@canonical.com> 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 <oliver.calder@canonical.com> boot: added unit tests for setting EFI Boot#### variable Signed-off-by: Oliver Calder <oliver.calder@canonical.com> 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 <oliver.calder@canonical.com> 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 <oliver.calder@canonical.com> boot,bootloader: fixed linting errors and improved EFI boot variable test clarity Signed-off-by: Oliver Calder <oliver.calder@canonical.com> bootloader: improved unit test for grub EFI load option creation Signed-off-by: Oliver Calder <oliver.calder@canonical.com> 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 <oliver.calder@canonical.com> 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 <oliver.calder@canonical.com> tests: added simple test to execute setefibootvar.go code Signed-off-by: Oliver Calder <oliver.calder@canonical.com> tests: fixed standalone set EFI vars code test to work with different layouts Signed-off-by: Oliver Calder <oliver.calder@canonical.com> tests: moved simple setefibootvar.go check to nested test Signed-off-by: Oliver Calder <oliver.calder@canonical.com> tests: added check for idempotence when setting EFI boot variables Signed-off-by: Oliver Calder <oliver.calder@canonical.com> bootloader: adjust comments, organization, and add TODO Signed-off-by: Oliver Calder <oliver.calder@canonical.com> 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 <oliver.calder@canonical.com> bootloader: fixed grub EFI load option test with tmp rootdir Signed-off-by: Oliver Calder <oliver.calder@canonical.com> go.mod: move golang.org/x/text import next to other golang.org/x/ imports Signed-off-by: Oliver Calder <oliver.calder@canonical.com> boot: adjust opts to look for recovery bootloader when setting EFI variables Signed-off-by: Oliver Calder <oliver.calder@canonical.com> boot: do not overwrite BootOrder if unchanged, and unexport EFI variable helper functions Signed-off-by: Oliver Calder <oliver.calder@canonical.com> boot: unexport `setEfiBootOrderVariable` Signed-off-by: Oliver Calder <oliver.calder@canonical.com> boot: move code to detect bootloader and set EFI variables accordingly into dedicated function Signed-off-by: Oliver Calder <oliver.calder@canonical.com> boot: unexport `setUbuntuSeedEfiBootVariables` and accompanying error Signed-off-by: Oliver Calder <oliver.calder@canonical.com> boot,bootloader: ensure nil optionalData for EFI variable is equivalent to 0-length slice Signed-off-by: Oliver Calder <oliver.calder@canonical.com> boot: handle empty boot order and other boot var improvements Signed-off-by: Oliver Calder <oliver.calder@canonical.com> boot: make setefibootvars functions linux-only Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
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 <oliver.calder@canonical.com> tests: fix problems in spread test for setting EFI boot variables Signed-off-by: Oliver Calder <oliver.calder@canonical.com> tests: disabled TPM on EFI boot vars test and separated gadget script Signed-off-by: Oliver Calder <oliver.calder@canonical.com> tests: fixed EFI vars test to use correct toolbox and include all EFI assets Signed-off-by: Oliver Calder <oliver.calder@canonical.com> tests: modify-gadget.sh re-use existing gadget so edition is incremented Signed-off-by: Oliver Calder <oliver.calder@canonical.com> tests: fix mangled EFI var search string and other improvements Signed-off-by: Oliver Calder <oliver.calder@canonical.com> 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 <oliver.calder@canonical.com>
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
github-actions
Bot
added
the
Run Nested -auto-
valentindavid
force-pushed
the
valentindavid/shim-fallback-efi-vars-all-boot-chains
branch
5 times, most recently
from
3e90399 to
793464d
Compare
We need to resolve the boot chains another place based on the trusted assets we encountered to be installed. At this point it could be any chain. We will need to discover later what the correct chain is. Also make TrustedAssets return an unsorted data structure to make sure we do not use the order like the comments claimed.
valentindavid
force-pushed
the
valentindavid/shim-fallback-efi-vars-all-boot-chains
branch
from
13c8362 to
cb88e79
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #13398 +/- ##
==========================================
+ Coverage 78.81% 78.83% +0.02%
==========================================
Files 1020 1023 +3
Lines 127158 127958 +800
==========================================
+ Hits 100214 100872 +658
- Misses 20670 20771 +101
- Partials 6274 6315 +41
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is draft because this is a staging branch for #13205 to sort out the issue with multiple boot chains during resealing.