netboot cleanup for additional files #686
Draft
Conversation
This file contains 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
Reading files during a netboot comes with the caveat that fetching files from a network does not support anything like listing a directory. In the past this has meant that we do not try to open optional files during a netboot. However at least the revocation.efi file is now tested during a netboot, which will print an error when it is not found. Since that error is spurious we should allow for those errors to be suppressed. This is also desirable since we will likely go looking for additional files in the near future.
While a revocations.efi binary can contain either SBAT revocations,
SkuSi revocations, or both, it is desirable to package them separately
so that higher level tools such as fwupd can decide which ones to put
in place at a given moment. This changes revocations.efi to
revocations_sbat.efi and revocations_sku.efi
XXX: Retain support a common revocations.efi file?
Constrain the sections to match the file?
Bugfix: In the netboot case revocations.efi files were read, but processed as shim_certificate.efi files which is simply wrong.
Since we can't read the directory, we can try to load
shim_certificate_[0..9].efi explicitly and give up after
the first one that fails to load.
XXX: should we just bring in snprintf()?
support more than 10?
nameing scheme
hughsie
reviewed
Aug 20, 2024
| @@ -1464,7 +1464,10 @@ load_revocations_file(EFI_HANDLE image_handle, CHAR16 *PathName) | |||
| uint8_t *ssps_latest = NULL; | |||
| uint8_t *sspv_latest = NULL; | |||
|
|
|||
| efi_status = read_image(image_handle, L"revocations.efi", &PathName, | |||
| efi_status = read_image(image_handle, L"revocations_sbat.efi", &PathName, | |||
| &data, &datasize, | |||
There was a problem hiding this comment.
are you missing a check for efi_status here?
hughsie
reviewed
Aug 20, 2024
| while (load_cert_file(image_handle, FileName, PathName, | ||
| SUPPRESS_NETBOOT_OPEN_FAILURE_NOISE) | ||
| == EFI_SUCCESS && i++ < 10) { | ||
| FileName[17]++; |
There was a problem hiding this comment.
I think it's going to be confusing having to rename shim_certificate_sbat.efi to shim_certificate_0.efi, shim_certificate_ski.efi to shim_certificate_1.efi etc. Can't we just use those names?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
These proposed changes are ready for comments, especially around the naming. Some XXX's in the commit comments.
These are all pretty simple changes, I've not included the SbatLevel index in dbx which isn't pretty (yet).