Rebase on 15.7 #20
Closed
Rebase on 15.7 #20
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
The EFI 1.10 spec (and presumably earlier revisions as well) didn't have RT->QueryVariableInfo(), and on Chris Murphy's MacBookPro8,2 , that memory appears to be initialized randomly. This patch changes it to not call RT->QueryVariableInfo() if the EFI_RUNTIME_SERVICES table's major revision is less than two, and assumes our maximum variable size is 1024 in that case. Signed-off-by: Peter Jones <pjones@redhat.com>
On some versions of binutils[0], including binutils-2.23.52.0.1-55.el7,
do not correctly initialize the data when computing the PE optional
header checksum. Unfortunately, this means that any time you get a
build that reproduces correctly using the version of objcopy from those
versions, it's just a matter of luck.
This patch introduces a new utility program, post-process-pe, which does
some basic validation of the resulting binaries, and if necessary,
performs some minor repairs:
- sets the timestamp to 0
- this was previously done with dd using constant offsets that aren't
really safe.
- re-computes the checksum.
[0] I suspect, but have not yet fully verified, that this is
accidentally fixed by the following upstream binutils commit:
commit cf7a3c01d82abdf110ef85ab770e5997d8ac28ac
Author: Alan Modra <amodra@gmail.com>
Date: Tue Dec 15 22:09:30 2020 +1030
Lose some COFF/PE static vars, and peicode.h constify
This patch tidies some COFF and PE code that unnecessarily used static
variables to communicate between functions.
v2 - MAP_PRIVATE was totally wrong...
Signed-off-by: Peter Jones <pjones@redhat.com>
An openSUSE user reported(*) that shim 15.4 failed to boot the system with the following message: "Could not create MokListXRT: Out of Resources" In the beginning, I thought it's caused by the growing size of vendor-dbx. However, we found the following messages after set SHIM_VERBOSE: max_var_sz:8000 remaining_sz:85EC max_storage_sz:9000 SetVariable(“MokListXRT”, ... varsz=0x1404) = Out of Resources Even though the firmware claimed the remaining storage size is 0x85EC and the maximum variable size is 0x8000, it still rejected MokListXRT with size 0x1404. It seems that the return values from QueryVariableInfo() are not reliable. Since this firmware didn't really support Secure Boot, the variable mirroring is not so critical, so we can just accept the failure of import_mok_state() and continue boot. (*) https://bugzilla.suse.com/show_bug.cgi?id=1185261 Signed-off-by: Gary Lin <glin@suse.com>
1. Use : instead of , to separate a list. 2. Fix spelling of therefore. 3. Pull unrelated clause out of parenthesized clause. Signed-off-by: Serge Hallyn <serge@hallyn.com>
Use the stronger "will" rather than "will should". I'm not sure based on what's there, but suspect "must" would be appropriate instead? Signed-off-by: Serge Hallyn <serge@hallyn.com>
The load options handling is quite complicated and tries to accomodate several scenarios, but there are currently multiple issues: - If the supplied LoadOptions is an EFI_LOAD_OPTION structure, second_stage gets initialized to the entire contents of the OptionalData field and load_options is initialized to NULL, which means it isn't possible to pass additional options to the second stage loader (and it looks like the intention is for this to be supported). - If the supplied LoadOptions contains 2 or more strings, the code seems to assume that shim was executed from the UEFI shell and that the first argument is the path of the shim executable, so it's ignored. But this breaks the ability to pass additional options to the second stage loader from BDS on firmware implementations that initialize LoadOptions to just the OptionalData field of the EFI_LOAD_OPTION, which is what EDK2 seems to do. This is moot anyway because this case (strings == 2) doesn't actually seem to work, as nothing sets loader_len and therefore second_stage is not set to the custom loader path. - If the supplied LoadOptions contains a single string that isn't shim's path, nothing sets loader_len and therefore second_stage isn't set at the end of set_second_stage. - set_second_stage replaces L' ' characters with L'\0' - whilst this is useful to NULL terminate the path for the second stage, it doesn't seem quite right to do this for the remaining LoadOptions data. Grub's chainloader command supplies additional arguments as a NULL-terminated space-delimited string via LoadOptions. Making it NULL-delimited seems to be incompatible with the kernel's commandline handling, which wouldn't work for scenarios where you might want to direct-boot a kernel image (wrapped in systemd's EFI stub) from shim. - handle_image passes the original LoadOptions to the second stage if load_options is NULL, which means that the second stage currently always gets shim's load options. I've made an attempt to try to fix things. After the initial checks in set_second_stage, it now does this: - Tries to parse LoadOptions as an EFI_LOAD_OPTION in order to extract the OptionalData if it is. - If it's not an EFI_LOAD_OPTION, check if the first string is the current shim path and ignore it if it is (the UEFI shell case). - Split LoadOptions in to a single NULL terminated string (used to initialize second_stage) and the unmodified remaining data (used to initialize load_options and load_options_size). I've also modified handle_image to always set LoadOptions and LoadOptionsSize. If shim is executed with no options, or is only executed with a single option to override the second stage loader path, the second stage is executed with LoadOptions = NULL and LoadOptionsSize = 0 now. I've tested this on EDK2 and I can load a custom loader with extra options from both BDS and the UEFI shell: FS0:\> shimx64.efi test.efi LoadOptionsSize: 0 LoadOptions: (null) FS0:\> shimx64.efi test.efi LoadOptionsSize: 0 LoadOptions: (null) FS0:\> shimx64.efi test.efi foo bar LoadOptionsSize: 16 LoadOptions: foo bar
Signed-off-by: Peter Jones <pjones@redhat.com>
pause() is a posix function, and having it named the same as this makes it hard to include the asm.h header in some test cases. Signed-off-by: Peter Jones <pjones@redhat.com>
…pes. Signed-off-by: Peter Jones <pjones@redhat.com>
In some test cases, it's useful to be able to call some of the very common stuff in gnu-efi's efilib.h (i.e. CompareGuid()), but including that header itself is too big for me to tackle right now. This patch adds a few more decls to test.h. Signed-off-by: Peter Jones <pjones@redhat.com>
test.c duplicates a couple of objects (StrnCmp, StrCmp) that are in libefi.a, as well as SHIM_LOCK_GUID from lib/guid.o. While it's nice to have these at some places, we need to disable them if we're actually linking a test case against either of those. This patch adds HAVE_foo guards around those objects. Signed-off-by: Peter Jones <pjones@redhat.com>
This allows us to use library functions from libefi.a in our test programs. Signed-off-by: Peter Jones <pjones@redhat.com>
In some test cases, it may be useful to call libefi.a functions, such as the device path parsing functions, which allocate pages via BS->AllocatePool() or BS->AllocatePages. This patch ads a simple mock implementation of those functions, as well as the EFI_SYSTEM_TABLE, EFI_BOOT_SERVICES, and EFI_RUNTIME_SERVICES variables *ST, *BS, and *RT (respectively), and initializes them before the test cases run. Signed-off-by: Peter Jones <pjones@redhat.com>
This test helper was conspicuously missing, so this patch just adds it at the obvious place. Signed-off-by: Peter Jones <pjones@redhat.com>
Some tests have some complex flows, and it's useful to be able to see the call path when there's a failure. This patch adds a very simple traceback printer, along with changing the test build arguments to include more debug information. The result you get from this traceback printer just gives you a function name and the index into its .txt content, so to use it for more than "which function calls which", you'll need to use eu-addr2line with the output. Signed-off-by: Peter Jones <pjones@redhat.com>
This moves set_second_stage() and some of the helper functions it uses out of shim.c, so that it's easier to write test cases for. Signed-off-by: Peter Jones <pjones@redhat.com>
This adds tests for all the cases we've documented in the set_second_stage() comments. Each test checks that all of second_stage, loader_str, and loader_str_size are set correctly. Note that this adds a dependency on libefivar to build device paths to test against. Signed-off-by: Peter Jones <pjones@redhat.com>
Some firmware feeds the LoadOptions with an odd length when booting from an USB device(*). We should only skip this kind of LoadOptions, not fail it, or the user won't be able to boot the system from USB or CD-ROM. (*) https://bugzilla.suse.com/show_bug.cgi?id=1185232#c62 Signed-off-by: Gary Lin <glin@suse.com>
The previous commit(*) merged .rel* and .dyn* into .rodata, and this made ld to generate the wrong size for .rela* sections that covered other unrelated sections. When the EFI image was loaded, _relocate() went through the unexpected data and may cause unexpected crash. This commit moves .rel* and .dyn* out of .rodata in the ld script but also moves the related variables, such as _evrodata, _rodata_size, and _rodata_vsize, to the end of the new .dyn section, so that the crafted pe-coff section header for .rodata still covers our new .rela and .dyn sections. (*) 212ba30 ("arm/aa64 targets: put .rel* and .dyn* in .rodata") Fix issue: rhboot#371 Signed-off-by: Gary Lin <glin@suse.com>
Fix the case where data_size is 0, so config_template is not implicitly copied like the size calculation above. upstream-status: rhboot#249 Signed-off-by: Jonathan Yong <jonathan.yong@intel.com>
Some UEFI environment such as u-boot doesn't implement QueryVariableInfo(), so we couldn't rely on the function to estimate the available space for RT variables. All we can do is to call SetVariable() directly and check the return value of SetVariable(). Signed-off-by: Gary Lin <glin@suse.com>
When EBS protection is disabled the code which hooks into EBS is complied out, but on unhook it's the code which restores Exit() that is disabled. This appears to be a mistake, and it can result in writing NULL to EBS in the boot services table. Fix this by moving the ifdefs to compile out the code to unhook EBS instead of the code to unhook Exit(). Also ifdef the definition of system_exit_boot_services to safeguard against its accidental use. Fixes: 4b0a61d ("shim: compile time option to bypass the ExitBootServices() check") Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
…ntry in optnum The CopyMem() calls in add_to_boot_list() expect that find_boot_option() returned an index to the matching entry in the BootOrder array. The previous code returned the numerical portion of the boot entry label, which in some cases resulted in -1 * sizeof(CHAR16) being passed to CopyMem() which would in turn corrupt the running firmware resulting in an exception and a failure to boot or reset.
Some servers (HAProxy) yield Content-Length in lowercase, and shim would fail to find it. Signed-off-by: Robbie Harwood <rharwood@redhat.com>
After calling AllocateZeroPool() we must check the returned pointer. Fixes: 3ce517f ("Add a fallback loader for when shim is invoked as BOOTX64.EFI") Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Heinrich correctly noticed that read_file()'s memory allocation failure error path tested the wrong variable, and thus would never catch the error condition. The *next* error, file->Read() failing, has the same incorrect variable usage in its FreePool() invocation. This fixes it to free the right variable as well. Signed-off-by: Peter Jones <pjones@redhat.com>
An error message complaining about missing file BOOTx64.EFI makes no sense on other architectures. Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
There is no need to check the parameters of strntoken() twice. Fixes: c7bb10c ("Tidy up our string primitives...") Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
When listing all variables checking for duplicates, print which error was returned if not EFI_NOT_FOUND (which simply means reached the end of the list). Signed-off-by: João Paulo Rechi Vita <jprvita@endlessos.org>
Now that we are iterating over all EFI variables to look for duplicate boot entries, we should use a dynamic buffer, so if the firmware tells us the buffer is too small for the next variable name we can re-allocate it to a big enough size. Signed-off-by: João Paulo Rechi Vita <jprvita@endlessos.org>
Signed-off-by: Peter Jones <pjones@redhat.com>
Signed-off-by: Peter Jones <pjones@redhat.com>
Signed-off-by: Peter Jones <pjones@redhat.com>
Given a set of EFI variables and boot assets, it should be possible to compute what the value of PCR 7 will be on the next boot. As shim manages the contents of the SbatLevel variable and this is measured to PCR 7, export the payloads that shim contains in a new COFF section (.sbatlevel) so that it can be introspected by code outside of shim. The new section works a bit like .vendor_cert - it contains a header and then the payload. In this case, the header contains no size fields because the strings are NULL terminated. Shim uses this new section internally in set_sbat_uefi_variable. The .sbatlevel section starts with a 4 byte version field which is not used by shim but may be useful for external auditors if the format of the section contents change in the future. Signed-off-by: Chris Coulson <chris.coulson@canonical.com>
When calling back into shim from grub, the MokListRT may contain additional entries not available in the original MokList, an example being the certs included via user_cert. Use the MokListRT instead when calling check_db_cert and check_db_hash. Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
It's been suggested that we should link to the test plan in the readme. This seems pretty reasonable to me, so here it is. Signed-off-by: Peter Jones <pjones@redhat.com>
Intel Trust Domain Extensions (Intel TDX) extends Virtual Machine Extensions (VMX) and Multi-Key Total Memory Encryption (MK-TME) with a new kind of virtual machine guest called a Trust Domain(TD)[1]. A TD runs in a CPU mode that is designed to protect the confidentiality of its memory contents and its CPU state from any other software, including the hosting Virtual Machine Monitor (VMM). Trust Domain Virtual Firmware (TDVF) is required to provide Intel TDX implementation and service for EFI_CC_MEASUREMENT_PROTOCOL[2]. The bugzilla for TDVF is at https://bugzilla.tianocore.org/show_bug.cgi?id=3625. To support CC measurement/attestation with Intel TDX technology, these 4 RTMR registers will be extended by TDX service like TPM/TPM2 PCR: - RTMR[0] for TDVF configuration - RTMR[1] for the TD OS loader and kernel - RTMR[2] for the OS application - RTMR[3] reserved for special usage only Add a TDX Implementation for CC Measurement protocol along with TPM/TPM2 protocol. References: [1] https://software.intel.com/content/dam/develop/external/us/en/documents/tdx-whitepaper-v4.pdf [2] https://software.intel.com/content/dam/develop/external/us/en/documents/tdx-virtual-firmware-design-guide-rev-1.pdf [3] https://software.intel.com/content/dam/develop/external/us/en/documents/intel-tdx-guest-hypervisor-communication-interface-1.0-344426-002.pdf Signed-off-by: Lu Ken <ken.lu@intel.com> [rharwood: style pass on code and commit message] Signed-off-by: Robbie Harwood <rharwood@redhat.com>
In 6c8d08c ("shim: Ignore UEFI LoadOptions that are just NUL characters."), a check was added to discard load options that are entirely NUL. We now see some firmwares that start LoadOptions with a NUL, and then follow it with garbage (path to directory containing loaders). Widen the check to just discard anything that starts with a NUL. Resolves: rhboot#490 Related: rhboot#95 See-also: https://bugzilla.redhat.com/show_bug.cgi?id=2113005 Signed-off-by: Robbie Harwood <rharwood@redhat.com>
We've seen crashes in early GRUB code on an ARM Cortex-A72-based platform that point at seemingly harmless instructions. Flushing the i-cache of those instructions prior to executing has been shown to avoid the problem, which has parallels with this story: https://www.mail-archive.com/osv-dev@googlegroups.com/msg06203.html Add a cache flushing utility function and provide an implementation using a GCC intrinsic. This will need to be extended to support other compilers. Note that this intrinsic is a no-op for x86 platforms. This fixes issue rhboot#498. Signed-off-by: dann frazier <dann.frazier@canonical.com>
0214cd9 fixes a NULL pointer dereference problem, it introduces two new problems. First it incorrectly assumes li.FilePath is a string. Second, it puts EFI_LOADED_IMAGE li on the stack. It has been found that not all archectures can handle this being on the stack. The shim_li variable will be setup properly from the read_image call. Use the global shim_li variable instead when calling verify_image. Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
Use the EFI RT memory function CopyMem instead of memcpy in load_cert_file. Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
This changes the alignment of UINT64 data to 8 bytes on IA32, which matches EDK2's understanding of alignment. In particular this change affects the offset where shim writes `EFI_LOADED_IMAGE.ImageSize`. Fixes rhboot#515 Signed-off-by: Nicholas Bishop <nicholasbishop@google.com>
Seen mokmanager image load failure '2 sections contain entry point' for shim built on Oracle Linux 9 aarch64. found_entry_point counter in handle_image() uses SizeOfRawData to calculate section boundary. PE spec defines VirtualSize for the total size of the section when loaded into memory. SizeOfRawData is the size of the section (for object files) or the size of the initialized data on disk. Fix this issue by updating section in-memory size limit to VirtualSize. Resolves: rhboot#517 Signed-off-by: Ilya Okomin <ilya.okomin@oracle.com>
Remove timestamps, user names, etc. from the tarball so that it can be built reproducibly by multiple people, on different machines. The outer bzip2 layer might still be different, no reproducible bzip2 known. Signed-off-by: Julian Andres Klode <julian.klode@canonical.com>
MokListTrusted was added by mistake to PCR 7 in 4e51340. The value of MokListTrusted does not alter the behavior of secure boot so, as per https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClient_PFP_r1p05_v23_pub.pdf#page=36 (section 3.3.4 PCR usage) so it should not be factored in the value of PCR 7. See: rhboot#423 rhboot@4e51340 Fixes rhboot#484 Fixes rhboot#492 Signed-off-by: Arthur Gautier <arthur.gautier@arista.com>
AuthenticodeVerify() calls OpenSSLs d2i_PKCS7() API to parse asn encoded signed authenticode pkcs#7 data. when this successfully returns, a type check is done by calling PKCS7_type_is_signed() and then Pkcs7->d.sign->contents->type is used. It is possible to construct an asn1 blob that successfully decodes and have d2i_PKCS7() return a valid pointer and have PKCS7_type_is_signed() also return success but have Pkcs7->d.sign be a NULL pointer. Looking at how PKCS7_verify() [inside of OpenSSL] implements checking for pkcs7 structs it does the following: - call PKCS7_type_is_signed() - call PKCS7_get_detached() Looking into how PKCS7_get_detatched() is implemented, it checks to see if p7->d.sign is NULL or if p7->d.sign->contents->d.ptr is NULL. As such, the fix is to do the same as OpenSSL after calling d2i_PKCS7(). - Add call to PKS7_get_detached() to existing error handling Cc: Chao Zhang <chao.b.zhang@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Signed-off-by: Jian J Wang <jian.j.wang@intel.com> Cherry-picked-from: tianocore/edk2@26442d1
This adds a few more UEFI functions to our coverity model, so we see a few less false positives during scanning. It also fixes an error in our model for OpenSSL's OBJ_dup(). Signed-off-by: Peter Jones <pjones@redhat.com>
Though we don't need to bump SBAT_LEVEL for this, we've decided to change the level to 3 here in case 53509ea turns out to be worse than we think it is, so we can fix that easily later. Signed-off-by: Peter Jones <pjones@redhat.com>
Due to the issues addressed in the 2022-11-15 batch of grub CVEs[0], we need to bump the sbat version from grub. This patch changes it from 2 to 3. [0] https://lists.gnu.org/archive/html/grub-devel/2022-11/msg00059.html Signed-off-by: Peter Jones <pjones@redhat.com>
Signed-off-by: Peter Jones <pjones@redhat.com>
…AMI BIOS" This reverts commit 0cc030c. If we consider two entries with the same label as duplicates (next commit) we don't need to worry about AMI's masking of boot entries. Signed-off-by: João Paulo Rechi Vita <jprvita@endlessos.org>
Consider all existing entries with the same label as the one we are trying to create as duplicates. When a duplicate is detected, add it to a remove list and later go over the remove list removing each boot entry and its occurrence in BootOrder, and finally let the rest of the logic work as if there was no boot entry. This change is needed for distributions that ship raw disk images instead of having an installer program partition the disk (ex., Endless OS) and generate a unique UUID for all partitions on first boot (otherwise every installation of the same release would have the same partition UUIDs). This setup relies on fallback to create a new boot entry pointing to the new ESP UUID on the second boot. Without this change, fallback will create a new boot entry with the same label pointing a the file path in the new ESP UUID while keeping the old entry that points at the non-existent UUID as a duplicate. While some firmware implemetations will clean-up these invalid boot entries on the next boot, other will just leave it around forever. However, this change breaks systems where it is desirable to have more than one boot entry with the same label pointing at different file paths for some reason (ex., two installations of the same OS on different ESPs). I'm not sure if this is a practical real-world use-case. https://phabricator.endlessm.com/T14430 https://phabricator.endlessm.com/T16731 Signed-off-by: João Paulo Rechi Vita <jprvita@endlessos.org>
In rhboot#533 , iokomin noticed that gas in binutils before 2.36 appears to be incorrectly concatenating string literals in '.asciz' directives, including an extra NUL character in between the strings, and this will cause us to incorrectly parse the .sbatlevel section in shim binaries. This patch adds test cases that will cause the build to fail if this has happened, as well as changing sbat_var.S to to use '.ascii' and '.byte' to construct the data, rather than using '.asciz'. Signed-off-by: Peter Jones <pjones@redhat.com> (cherry picked from commit 657b248)
Currently by default, when we build shim we do not set the PE NX-compatibility DLL Characteristic flag. This signifies to the firmware that shim (including the components it loads) is not prepared for several related firmware changes: - non-executable stack - non-executable pages from AllocatePages()/AllocatePool()/etc. - non-writable 0 page (not strictly related but some firmware will be transitioning at the same time) - the need to use the UEFI 2.10 Memory Attribute Protocol to set page permissions. This patch changes that default to be enabled by default. Distributors of shim will need to ensure that either their builds disable this bit (using "post-process-pe -N"), or that the bootloaders and kernels you support loading are all compliant with this change. A new make variable, POST_PROCESS_PE_FLAGS, has been added to simplify doing so. Signed-off-by: Peter Jones <pjones@redhat.com> (cherry picked from commit 7c76425)
|
The debian branch is in the T34488-rebase-15.7-debian, but I can't actually make a PR out of it due to the way I made the branch. Rather than committing the debian package as a single commit, I rebased on the debian/15.7-1_deb11u1 tag in the salsa repo. IMO that makes it easier to handle the rebase, but I can do it the other way if you prefer. As far as those commits are concerned:
Downstream changes kept.
Dropped as discussed in https://phabricator.endlessm.com/T31604.
Squashed into On top of that I added the following commits:
|
|
The debian commits relative to 15.7-1~deb11u1 can be seen here. |
|
This has been superseded by #22. |
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.
Rebase on 15.7 to get the fixes for
LoadOptionshandling (needed for loading fwupd). Below is a review of our outstanding changes, but the actual kept commits come from the fallback-cleanup-duplicate-boot-entries branch.Upstream backports.
Kept using the versions in
fallback-cleanup-duplicate-boot-entries.Dropped as discussed in https://phabricator.endlessm.com/T31604.
The changes relative to 15.7 can be seen here. This includes 2 more patches backported from upstream:
https://phabricator.endlessm.com/T34488