Add support for UEFI signing, SBAT signature revocations, and NX mitigations.#34
Conversation
|
Whoa, thanks for this draft contribution :) It shall help with #25 when merged. |
|
Thanks Peter, this is greatly appreciated 😍 |
martinwhitaker
left a comment
martinwhitaker
left a comment
There was a problem hiding this comment.
Adding boot/peimage.h will also require updating the top level LICENSE file. Currently all files in the project are GPL 2 only.
|
We don't need to load at 0x200000, but we do need to load at an address < 3GB (preferably < 2GB). I don't know any way to achieve that other than specifying a fixed address. To keep the program memory footprint low, we set up the paging tables to only use the first 4GB of virtual memory space. We use the space between 2GB and 3GB when testing memory, and we use the space between 3GB and 3.5GB to provide permanent access to memory mapped hardware and the frame buffer. So if we allow the EFI loader to load the program anywhere in physical memory, we would have to be prepared to to relocate it. |
I'm pretty sure I can find a GPLv2 version of it, let me look around a bit. |
vathpela
force-pushed
the
uefi-signing-and-sbat
branch
from
10e06f8 to
8e07a47
Compare
Is relocating it an option you'd be okay with? I am able to contribute relocation code... That said, I may be able to just drop that patch; it's mostly there right now to make comparing two VMs with different firmware easier. |
|
Relocating is okay, and is already supported - the program regularly relocates itself whilst the tests are running to allow the memory it occupies to be tested. So it should just be a matter of scanning the E820 memory map to find a suitable location, copying the program code and data sections, and contriving to return from I just wasn't going to bother doing that unless it was found to be needed. |
vathpela
force-pushed
the
uefi-signing-and-sbat
branch
from
8e07a47 to
f8d88aa
Compare
|
@vathpela Is there something we can do to help? |
|
Hi there. Just checking what is the plan here? The PR is currently marked as draft, how much work is left here? Do we expect this can be merged early during 6.10 development? |
This patch makes it so we use "gcc -x assembler-with-cpp" to build our .S files, instead of translating them to .s files and assembling directly. This allows us to use header files and simple symbolic arithmetic more conveniently in .S files, and at the same time reduces the number of temporary files created when building. Signed-off-by: Peter Jones <[email protected]>
This adds a header file to describe the PE binary we're building. This has constants defined for all the values we use in the PE headers, as well as the structures for reference (guarded by #ifdef __ASSEMBLY__). This particular peimage.h is originally from binutils-2.10.0.18, which is GPLv2 licensed, and is copyright the Free Software Foundation. I've added the few additional fields we need. Signed-off-by: Peter Jones <[email protected]>
This changes header.S to use the constants defined in peimage.h to for the values in its structure, making it a lot easier to debug. Signed-off-by: Peter Jones <[email protected]>
Currently, the PE headers we create in boot/header.S do not allocate space for any Data Directory entries, as they haven't been needed. In order to support signatures and compatibility with some loaders, we need the Data Directory to be populated at least enough to set DataDirectory.Certs and DataDirectory.BaseReloc. This patch extends that space enough to include those entries. Signed-off-by: Peter Jones <[email protected]>
SizeOfImage is defined as: The size (in bytes) of the image, including all headers, as the image is loaded in memory. It must be a multiple of SectionAlignment. SizeOfHeaders likewise is defined as: The combined size of an MS-DOS stub, PE header, and section headers rounded up to a multiple of FileAlignment. Currently SizeOfImage represents .bss and .text, but it doesn't include .header or .setup, nor any sections we'll add later, and there's nothing enforcing that it matches SectionAlignment. Additionally, since .bss is being set up in our running code and /not/ by the loader, the current value is dangerously high, as in the event there is an error in the section table, it could potentially lead the loader to mark memory allocated at runtime holding user-supplied data by any EFI binary loaded before us as executable. This patch adds a new symbol, _img_end, which is after .text and is rounded up to 4kB (which is also what SectionAlignment is set to). It also adds a local label, anchored with ".org 512", and uses that to set SizeOfHeaders - this will ensure the build fails without outputting and invalid binary if the headers take too much space. Signed-off-by: Peter Jones <[email protected]>
In the past, we've seen some problems with some EFI loaders refusing to load a binary that has both a .text section with the VMA set and no relocations, when the VMA set to load is already allocated for some other purpose. This patch adds a dummy absolute relocation from 0 to 0, so the loader can always feel like it has done something useful. Signed-off-by: Peter Jones <[email protected]>
vathpela
force-pushed
the
uefi-signing-and-sbat
branch
from
f8d88aa to
6dfd70e
Compare
|
I haven't had the time to debug whatever issue I'm hitting with this on QEMU, but it works on all of my real hardware, so I think that's probably not a good reason to hold this back, especially since there's little point in actually running memtest in QEMU. So I've removed the .gitignore patch that I was just carrying for my own creature comforts, as well as the patch to set the NX flag (because I was wrong and there's work to be done there), and marked it as ready for review. |
This patch adds a new section, ".sbat", which allows for the revocation of signed binaries given a numeric value representing the set of bugs which allow for arbitrary code execution, and therefore a Secure Boot breakout, in a given family of binaries. In this case, the class is defined as "memtest86+", and the current set of bugs is 1. This doesn't imply that we're aware of bugs currently, merely that when we change it to 2, any bugs that /have/ been discovered have been fixed. Documentation for how SBAT works can be found at the following URLs: https://github.com/rhboot/shim/blob/main/SBAT.md https://github.com/rhboot/shim/blob/main/SBAT.example.md Signed-off-by: Peter Jones <[email protected]>
vathpela
force-pushed
the
uefi-signing-and-sbat
branch
from
6dfd70e to
e35dae2
Compare
|
Awesome! We will carefully review this PR then I'll put this in betatesters' hands as soon as possible. |
|
This looks OK to me. I did use QEMU extensively for developing and debugging the USB drivers, but I'm not seeing any issues with it. |
|
Betatesters found an issue with this PR on Asus 700-series motherboard. We're investigating the issue with their BIOS team. It seems linked to a specific ME. |
|
I do note that now _bss_size is not included in SizeOfImage, I can't see that there is any guarantee that the bss section does not collide with memory reserved by the EFI BIOS (which is why I included it). |
|
Info point: We found an issue (indirectly related) on ASUS motherboard BIOS based on 700-series Intel chipset that prevent testing of secureboot related code right now. The bug is serious as it lead to a firmware corruption with boot failure. It's only present on 0602 BIOS update. A beta fix has been sent by ASUS and successfully tested. Betatesters are now waiting for the final new BIOS to resume validation on SB. |
Am I right in understanding that we believe this is a bios bug? |
|
Yes, this has been confirmed and solved by Asus. Unfortunately, the betatester team found in the meanwhile another (unrelated) critical bug on Asus motherboards that broke the validation process on the production line. They're now waiting for a fix to resume testing this PR. |
|
Got the the first preliminary report from betatesters. This PR has been tested on 50+ platforms with Secure Boot Enabled with great success! Thanks @vathpela! The only fails they got were on Intel NUCs (waiting for more details). I tested it myself on some motherboards and laptops without issues so far. It's time to merge! |
When the reloc and sbat sections were added by PR #34, three bugs were introduced: 1. The virtual address and size fields in the PE headers were set to the same values as the raw address and size fields. This is incorrect, because the sections in the image file are aligned on 512 byte boundaries, but when loaded into memory they need to be aligned on 4096 byte boundaries. 2. The value programmed into the SizeOfImage field was too large, as it double-counted the region before the start of the .text section. 3. The value programmed into the SizeOfImage field no longer included the bss size. That potentially allowed the EFI loader to load the image immediately before a reserved region of memory without leaving enough space for the bss section. This commit fixes those bugs by calculating both file and virtual memory offsets & sizes in the ld script. Note that we can't add a bss section to the EFI image because many EFI loaders fail to load images that have uninitialised data sections. Instead the text region size in virtual memory is increased to include the bss size. This fixes issue #243. It also eliminates the gaps between sections observed in issue #202.
When the reloc and sbat sections were added by PR #34, three bugs were introduced: 1. The virtual address and size fields in the PE headers were set to the same values as the raw address and size fields. This is incorrect, because the sections in the image file are aligned on 512 byte boundaries, but when loaded into memory they need to be aligned on 4096 byte boundaries. 2. The value programmed into the SizeOfImage field was too large, as it double-counted the region before the start of the .text section. 3. The value programmed into the SizeOfImage field no longer included the bss size. That potentially allowed the EFI loader to load the image immediately before a reserved region of memory without leaving enough space for the bss section. This commit fixes those bugs by calculating both file and virtual memory offsets & sizes in the ld script. Note that we can't add a bss section to the EFI image because many EFI loaders fail to load images that have uninitialised data sections. Instead the text region size in virtual memory is increased to include the bss size. This fixes issue #243. It also eliminates the gaps between sections observed in issue #202.
|
@vathpela , could you please have a look at #516 ? |
6 participants
(This is not yet ready)
In order to facilitate adoption by major linux distributions, memtest86+ EFI builds are going to need to support Authenticode signatures and the SBAT revocation method, and will eventually need to support being loaded with NX (W^X) vulnerability mitigations enabled.
This patch series aims to add all of those, though right now it doesn't work correctly on some UEFI firmware builds, and I'm still debugging, so there are some extra patches here that won't be necessary once I'm ready to propose it upstream.