| Message ID | 20220224154330.26564-1-baskov@ispras.ru |
|---|---|
| Headers | show |
| Series | Handle UEFI NX-restricted page tables | expand |
(cc Matt and Peter) On Thu, 24 Feb 2022 at 16:45, Baskov Evgeniy <baskov@ispras.ru> wrote: > > This is another implementation of this patch. It uses DXE services > to change memory protection flags as you suggested earlier. > > As I mentioned, you can reproduce this issue with any firmware, > including OVMF by setting the PcdDxeNxMemoryProtectionPolicy policy: > gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0x7FD4 > Restricting the user from creating executable pages is not an goal, > but restricting one from creating both executable and writable pages > is a goal, which is enforced in the firmware we use. We cannot allow > allocating pages of any type that have RWX permissions. > gDS->SetMemorySpaceAttributes() is technically part of the UEFI PI > specification, so it is not too bad to rely on it. However: > - DXE services is not something designed to be used by an UEFI application. > - From what we know, no other operating system uses this interface, > which means that it can easily break in production firmware on the > boards we do not control before anyone could even notice. > We do not strictly mind experimenting with this route, but it would be > preferable for this interface to become more standard in this case: > move it to UEFI Boot Services or a separate protocol and include it in > UEFI conformance suite. It will also help if we enable this feature in > Linux by default. > Thanks for exploring my suggestion to use the DXE services for this. Given that this is a workaround for a very specific issue arising on PI based implementations of UEFI, I consider this a quirk, and so I think this approach is reasonable. I'd still like to gate it on some kind of identification, though - perhaps something related to DMI like the x86 core kernel does as well. I've cc'ed Peter and Matt, who have much more experience dealing with these kinds of things on x86 - my experience is mostly based on ARM, which tends to be less quirky when it comes to UEFI support, given that vendors that implement EFI actually care about being compliant (instead of only about getting a windows sticker) Matt, Peter, any thoughts? > Baskov Evgeniy (2): > efi: declare DXE services table > libstub: ensure allocated memory to be executable > > arch/x86/include/asm/efi.h | 5 ++ > drivers/firmware/efi/libstub/efistub.h | 53 ++++++++++++++++++++ > drivers/firmware/efi/libstub/x86-stub.c | 73 ++++++++++++++++++++++++++-- > include/linux/efi.h | 2 + > 4 files changed, 128 insertions(+), 5 deletions(-)
On Mon, Feb 28, 2022 at 05:45:53PM +0100, Ard Biesheuvel wrote: > Given that this is a workaround for a very specific issue arising on > PI based implementations of UEFI, I consider this a quirk, and so I > think this approach is reasonable. I'd still like to gate it on some > kind of identification, though - perhaps something related to DMI like > the x86 core kernel does as well. When the V1 patches were reviewed, you suggested allocating EFI_LOADER_CODE rather than EFI_LOADER_DATA. The example given for a failure case is when NxMemoryProtectionPolicy is set to 0x7fd4, in which case EFI_LOADER_CODE, EFI_BOOT_SERVICES_CODE and EFI_RUNTIEM_SERVICES_CODE should not have the nx policy applied. So it seems like your initial suggestion (s/LOADER_DATA/LOADER_CODE/) should have worked, even if there was disagreement about whether the spec required it to. Is this firmware applying a stricter policy?
On 2022-02-28 21:30, Matthew Garrett wrote: > On Mon, Feb 28, 2022 at 05:45:53PM +0100, Ard Biesheuvel wrote: > >> Given that this is a workaround for a very specific issue arising on >> PI based implementations of UEFI, I consider this a quirk, and so I >> think this approach is reasonable. I'd still like to gate it on some >> kind of identification, though - perhaps something related to DMI like >> the x86 core kernel does as well. > > When the V1 patches were reviewed, you suggested allocating > EFI_LOADER_CODE rather than EFI_LOADER_DATA. The example given for a > failure case is when NxMemoryProtectionPolicy is set to 0x7fd4, in > which > case EFI_LOADER_CODE, EFI_BOOT_SERVICES_CODE and > EFI_RUNTIEM_SERVICES_CODE should not have the nx policy applied. So it > seems like your initial suggestion (s/LOADER_DATA/LOADER_CODE/) should > have worked, even if there was disagreement about whether the spec > required it to. Is this firmware applying a stricter policy? Yes, this firmware is being modified to enforce stricter policy. Also, the kernel still uses memory, that is not allocated via EFI boot services, for trampoline placement in the first 640K of address space, for which NX bit is also set. The exact address for trampoline code is chosen only after the EfiExitBootServices() call. And, I think, changing the code in such way that trampoline is allocated beforehand will touch more code paths. Thanks, Baskov Evgeniy
On 2022-02-28 19:45, Ard Biesheuvel wrote: > (cc Matt and Peter) > > > Thanks for exploring my suggestion to use the DXE services for this. > > Given that this is a workaround for a very specific issue arising on > PI based implementations of UEFI, I consider this a quirk, and so I > think this approach is reasonable. I'd still like to gate it on some > kind of identification, though - perhaps something related to DMI like > the x86 core kernel does as well. > > I've cc'ed Peter and Matt, who have much more experience dealing with > these kinds of things on x86 - my experience is mostly based on ARM, > which tends to be less quirky when it comes to UEFI support, given > that vendors that implement EFI actually care about being compliant > (instead of only about getting a windows sticker) > > Matt, Peter, any thoughts? > > >> Baskov Evgeniy (2): >> efi: declare DXE services table >> libstub: ensure allocated memory to be executable >> >> arch/x86/include/asm/efi.h | 5 ++ >> drivers/firmware/efi/libstub/efistub.h | 53 ++++++++++++++++++++ >> drivers/firmware/efi/libstub/x86-stub.c | 73 >> ++++++++++++++++++++++++++-- >> include/linux/efi.h | 2 + >> 4 files changed, 128 insertions(+), 5 deletions(-) We now have tested the patch on major platforms, and it works without any issues. But in case of firmware bugs I have changed the code to only modify attributes if either EFI_MEMORY_RO or EFI_MEMORY_WP is set and the memory has type EfiGcdMemoryTypeSystemMemory. I also added option CONFIG_EFI_DXE_MEM_ATTRIBUTES (enabled by default), to allow this code to be disabled at compile time. These changes will be sent in version 3 of the patch. Thanks, Baskov Evgeniy
On Thu, Mar 03, 2022 at 04:42:07PM +0300, baskov@ispras.ru wrote: > On 2022-02-28 21:30, Matthew Garrett wrote: > > On Mon, Feb 28, 2022 at 05:45:53PM +0100, Ard Biesheuvel wrote: > > > > > Given that this is a workaround for a very specific issue arising on > > > PI based implementations of UEFI, I consider this a quirk, and so I > > > think this approach is reasonable. I'd still like to gate it on some > > > kind of identification, though - perhaps something related to DMI like > > > the x86 core kernel does as well. > > > > When the V1 patches were reviewed, you suggested allocating > > EFI_LOADER_CODE rather than EFI_LOADER_DATA. The example given for a > > failure case is when NxMemoryProtectionPolicy is set to 0x7fd4, in which > > case EFI_LOADER_CODE, EFI_BOOT_SERVICES_CODE and > > EFI_RUNTIEM_SERVICES_CODE should not have the nx policy applied. So it > > seems like your initial suggestion (s/LOADER_DATA/LOADER_CODE/) should > > have worked, even if there was disagreement about whether the spec > > required it to. Is this firmware applying a stricter policy? > > Yes, this firmware is being modified to enforce stricter policy. Ok. I think this should really go through the UEFI spec process - I agree that from a strict interpretation of the spec, what this firmware is doing is legitimate, but I don't like having a situation where we have to depend on the DXE spec. How does Windows handle this? Just update the page tables itself for any regions it needs during boot?
On 2022-03-03 23:47, Matthew Garrett wrote: > > Ok. I think this should really go through the UEFI spec process - I > agree that from a strict interpretation of the spec, what this firmware > is doing is legitimate, but I don't like having a situation where we > have to depend on the DXE spec. > > How does Windows handle this? Just update the page tables itself for > any > regions it needs during boot? Sorry for delay. Windows is closed source, so we cannot give guarantees on its behavior, but this is our belief regarding its behavior. Added Bret Barkelew (bret.barkelew@microsoft.com) to the CC-list in case he can add something. Regarding the spec changes, we agree it is reasonable, but whether the spec changes or not it will take some time to update the edk2. Our first solution was safer in regards to the use of the services, yet as Ard suggested, using DXE services is much cleaner as long as it works. We can post it to edk2-devel, but our opinion is that these issues are independent. Thanks, Baskov Evgeniy
On Thu, Mar 03, 2022 at 08:47:59PM +0000, Matthew Garrett wrote: > On Thu, Mar 03, 2022 at 04:42:07PM +0300, baskov@ispras.ru wrote: > > On 2022-02-28 21:30, Matthew Garrett wrote: > > > On Mon, Feb 28, 2022 at 05:45:53PM +0100, Ard Biesheuvel wrote: > > > > > > > Given that this is a workaround for a very specific issue arising on > > > > PI based implementations of UEFI, I consider this a quirk, and so I > > > > think this approach is reasonable. I'd still like to gate it on some > > > > kind of identification, though - perhaps something related to DMI like > > > > the x86 core kernel does as well. > > > > > > When the V1 patches were reviewed, you suggested allocating > > > EFI_LOADER_CODE rather than EFI_LOADER_DATA. The example given for a > > > failure case is when NxMemoryProtectionPolicy is set to 0x7fd4, in which > > > case EFI_LOADER_CODE, EFI_BOOT_SERVICES_CODE and > > > EFI_RUNTIEM_SERVICES_CODE should not have the nx policy applied. So it > > > seems like your initial suggestion (s/LOADER_DATA/LOADER_CODE/) should > > > have worked, even if there was disagreement about whether the spec > > > required it to. Is this firmware applying a stricter policy? > > > > Yes, this firmware is being modified to enforce stricter policy. > > Ok. I think this should really go through the UEFI spec process - I > agree that from a strict interpretation of the spec, what this firmware > is doing is legitimate, but I don't like having a situation where we > have to depend on the DXE spec. It's in the process of getting into the UEFI spec now as https://bugzilla.tianocore.org/show_bug.cgi?id=3519 . > How does Windows handle this? Just update the page tables itself for any > regions it needs during boot? Microsoft's bootloader sets up its own pagetables, though I believe they're switching it to use the (soon to be) standardized API.
On 2022-03-18 19:37, Peter Jones wrote: > On Thu, Mar 03, 2022 at 08:47:59PM +0000, Matthew Garrett wrote: >> On Thu, Mar 03, 2022 at 04:42:07PM +0300, baskov@ispras.ru wrote: >> > On 2022-02-28 21:30, Matthew Garrett wrote: >> > > On Mon, Feb 28, 2022 at 05:45:53PM +0100, Ard Biesheuvel wrote: >> > > >> > > > Given that this is a workaround for a very specific issue arising on >> > > > PI based implementations of UEFI, I consider this a quirk, and so I >> > > > think this approach is reasonable. I'd still like to gate it on some >> > > > kind of identification, though - perhaps something related to DMI like >> > > > the x86 core kernel does as well. >> > > >> > > When the V1 patches were reviewed, you suggested allocating >> > > EFI_LOADER_CODE rather than EFI_LOADER_DATA. The example given for a >> > > failure case is when NxMemoryProtectionPolicy is set to 0x7fd4, in which >> > > case EFI_LOADER_CODE, EFI_BOOT_SERVICES_CODE and >> > > EFI_RUNTIEM_SERVICES_CODE should not have the nx policy applied. So it >> > > seems like your initial suggestion (s/LOADER_DATA/LOADER_CODE/) should >> > > have worked, even if there was disagreement about whether the spec >> > > required it to. Is this firmware applying a stricter policy? >> > >> > Yes, this firmware is being modified to enforce stricter policy. >> >> Ok. I think this should really go through the UEFI spec process - I >> agree that from a strict interpretation of the spec, what this >> firmware >> is doing is legitimate, but I don't like having a situation where we >> have to depend on the DXE spec. > > It's in the process of getting into the UEFI spec now as > https://bugzilla.tianocore.org/show_bug.cgi?id=3519 . > >> How does Windows handle this? Just update the page tables itself for >> any >> regions it needs during boot? > > Microsoft's bootloader sets up its own pagetables, though I believe > they're switching it to use the (soon to be) standardized API. The third version of the patch is the most close in structure to the proposed protocol. And until the protocol is standardized and implemented on problematic firmware, I think, it remains the better solution in terms of simplicity and further porting to the new protocol. It is desirable to get the issue resolved, and make the kernel stricter comply to the spec, without waiting for the new API implementation. And later, switch the kernel to be using the protocol with subsequent patches as soon as it gets usable. So, is there a chance for these patches to be accepted in current form, or with some modifications? Thanks, Baskov Evgeniy
On Thu, 24 Mar 2022 at 17:39, <baskov@ispras.ru> wrote: > > On 2022-03-18 19:37, Peter Jones wrote: > > On Thu, Mar 03, 2022 at 08:47:59PM +0000, Matthew Garrett wrote: > >> On Thu, Mar 03, 2022 at 04:42:07PM +0300, baskov@ispras.ru wrote: > >> > On 2022-02-28 21:30, Matthew Garrett wrote: > >> > > On Mon, Feb 28, 2022 at 05:45:53PM +0100, Ard Biesheuvel wrote: > >> > > > >> > > > Given that this is a workaround for a very specific issue arising on > >> > > > PI based implementations of UEFI, I consider this a quirk, and so I > >> > > > think this approach is reasonable. I'd still like to gate it on some > >> > > > kind of identification, though - perhaps something related to DMI like > >> > > > the x86 core kernel does as well. > >> > > > >> > > When the V1 patches were reviewed, you suggested allocating > >> > > EFI_LOADER_CODE rather than EFI_LOADER_DATA. The example given for a > >> > > failure case is when NxMemoryProtectionPolicy is set to 0x7fd4, in which > >> > > case EFI_LOADER_CODE, EFI_BOOT_SERVICES_CODE and > >> > > EFI_RUNTIEM_SERVICES_CODE should not have the nx policy applied. So it > >> > > seems like your initial suggestion (s/LOADER_DATA/LOADER_CODE/) should > >> > > have worked, even if there was disagreement about whether the spec > >> > > required it to. Is this firmware applying a stricter policy? > >> > > >> > Yes, this firmware is being modified to enforce stricter policy. > >> > >> Ok. I think this should really go through the UEFI spec process - I > >> agree that from a strict interpretation of the spec, what this > >> firmware > >> is doing is legitimate, but I don't like having a situation where we > >> have to depend on the DXE spec. > > > > It's in the process of getting into the UEFI spec now as > > https://bugzilla.tianocore.org/show_bug.cgi?id=3519 . > > > >> How does Windows handle this? Just update the page tables itself for > >> any > >> regions it needs during boot? > > > > Microsoft's bootloader sets up its own pagetables, though I believe > > they're switching it to use the (soon to be) standardized API. > > The third version of the patch is the most close in structure > to the proposed protocol. And until the protocol is standardized and > implemented on problematic firmware, I think, it remains the better > solution in terms of simplicity and further porting to the new > protocol. > > It is desirable to get the issue resolved, and make the kernel stricter > comply to the spec, without waiting for the new API implementation. > And later, switch the kernel to be using the protocol with > subsequent patches as soon as it gets usable. > > So, is there a chance for these patches to be accepted in current > form, or with some modifications? > I am fine with taking the v3, as it is the most likely to only affect the systems that actually need this change in behavior. So unless there are any objections, I will queue these up after the merge window.
On Thu, Mar 24, 2022 at 07:39:47PM +0300, baskov@ispras.ru wrote: > On 2022-03-18 19:37, Peter Jones wrote: > > On Thu, Mar 03, 2022 at 08:47:59PM +0000, Matthew Garrett wrote: > > > On Thu, Mar 03, 2022 at 04:42:07PM +0300, baskov@ispras.ru wrote: > > > > On 2022-02-28 21:30, Matthew Garrett wrote: > > > > > On Mon, Feb 28, 2022 at 05:45:53PM +0100, Ard Biesheuvel wrote: > > > > > > > > > > > Given that this is a workaround for a very specific issue arising on > > > > > > PI based implementations of UEFI, I consider this a quirk, and so I > > > > > > think this approach is reasonable. I'd still like to gate it on some > > > > > > kind of identification, though - perhaps something related to DMI like > > > > > > the x86 core kernel does as well. > > > > > > > > > > When the V1 patches were reviewed, you suggested allocating > > > > > EFI_LOADER_CODE rather than EFI_LOADER_DATA. The example given for a > > > > > failure case is when NxMemoryProtectionPolicy is set to 0x7fd4, in which > > > > > case EFI_LOADER_CODE, EFI_BOOT_SERVICES_CODE and > > > > > EFI_RUNTIEM_SERVICES_CODE should not have the nx policy applied. So it > > > > > seems like your initial suggestion (s/LOADER_DATA/LOADER_CODE/) should > > > > > have worked, even if there was disagreement about whether the spec > > > > > required it to. Is this firmware applying a stricter policy? > > > > > > > > Yes, this firmware is being modified to enforce stricter policy. > > > > > > Ok. I think this should really go through the UEFI spec process - I > > > agree that from a strict interpretation of the spec, what this > > > firmware > > > is doing is legitimate, but I don't like having a situation where we > > > have to depend on the DXE spec. > > > > It's in the process of getting into the UEFI spec now as > > https://bugzilla.tianocore.org/show_bug.cgi?id=3519 . > > > > > How does Windows handle this? Just update the page tables itself for > > > any > > > regions it needs during boot? > > > > Microsoft's bootloader sets up its own pagetables, though I believe > > they're switching it to use the (soon to be) standardized API. > > The third version of the patch is the most close in structure > to the proposed protocol. And until the protocol is standardized and > implemented on problematic firmware, I think, it remains the better > solution in terms of simplicity and further porting to the new > protocol. The ECR was approved at last week's meeting, it'll be in the next UEFI spec. Details of what spec version that'll be and when it will officially be released are still under discussion, but it's been approved in its current form. Microsoft has been kind enough to provide us code for test firmware, though the build process is a little rough. I've done some builds of it here: https://copr.fedorainfracloud.org/coprs/pjones/mu-qemuq35/builds/ . The src rpm there is a bit absurdly large, because I've done the very quick-and-dirty hack of just shoving a pile of git repos into it instead of trying to make release tarballs of everything, and it needs the network enabled to rebuild it for fairly dumb reasons. But the result is a firmware that works in QEMU. > It is desirable to get the issue resolved, and make the kernel stricter > comply to the spec, without waiting for the new API implementation. > And later, switch the kernel to be using the protocol with > subsequent patches as soon as it gets usable. It works for the bootloaders in my development trees; I've booted a kernel with your patches. From the bootloader POV we do need one more simple patch to enable the compatibility flag in the headers, I'll send it as a follow-up to this mail.
On Tue, 29 Mar 2022 at 20:47, Peter Jones <pjones@redhat.com> wrote: > > Following Baskov Evgeniy's "Handle UEFI NX-restricted page tables" > patches, it's safe to set this compatibility flag to let loaders know > they don't need to make special accommodations for kernel to load if > pre-boot NX is enabled. > > Signed-off-by: Peter Jones <pjones@redhat.com> Thanks, I've queued this up. > --- > arch/x86/boot/header.S | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S > index 6dbd7e9f74c..0352e4589ef 100644 > --- a/arch/x86/boot/header.S > +++ b/arch/x86/boot/header.S > @@ -163,7 +163,11 @@ extra_header_fields: > .long 0x200 # SizeOfHeaders > .long 0 # CheckSum > .word IMAGE_SUBSYSTEM_EFI_APPLICATION # Subsystem (EFI application) > +#ifdef CONFIG_DXE_MEM_ATTRIBUTES > + .word IMAGE_DLL_CHARACTERISTICS_NX_COMPAT # DllCharacteristics > +#else > .word 0 # DllCharacteristics > +#endif > #ifdef CONFIG_X86_32 > .long 0 # SizeOfStackReserve > .long 0 # SizeOfStackCommit > -- > 2.35.1 >
On Fri, 25 Mar 2022 at 09:06, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Thu, 24 Mar 2022 at 17:39, <baskov@ispras.ru> wrote: > > > > On 2022-03-18 19:37, Peter Jones wrote: > > > On Thu, Mar 03, 2022 at 08:47:59PM +0000, Matthew Garrett wrote: > > >> On Thu, Mar 03, 2022 at 04:42:07PM +0300, baskov@ispras.ru wrote: > > >> > On 2022-02-28 21:30, Matthew Garrett wrote: > > >> > > On Mon, Feb 28, 2022 at 05:45:53PM +0100, Ard Biesheuvel wrote: > > >> > > > > >> > > > Given that this is a workaround for a very specific issue arising on > > >> > > > PI based implementations of UEFI, I consider this a quirk, and so I > > >> > > > think this approach is reasonable. I'd still like to gate it on some > > >> > > > kind of identification, though - perhaps something related to DMI like > > >> > > > the x86 core kernel does as well. > > >> > > > > >> > > When the V1 patches were reviewed, you suggested allocating > > >> > > EFI_LOADER_CODE rather than EFI_LOADER_DATA. The example given for a > > >> > > failure case is when NxMemoryProtectionPolicy is set to 0x7fd4, in which > > >> > > case EFI_LOADER_CODE, EFI_BOOT_SERVICES_CODE and > > >> > > EFI_RUNTIEM_SERVICES_CODE should not have the nx policy applied. So it > > >> > > seems like your initial suggestion (s/LOADER_DATA/LOADER_CODE/) should > > >> > > have worked, even if there was disagreement about whether the spec > > >> > > required it to. Is this firmware applying a stricter policy? > > >> > > > >> > Yes, this firmware is being modified to enforce stricter policy. > > >> > > >> Ok. I think this should really go through the UEFI spec process - I > > >> agree that from a strict interpretation of the spec, what this > > >> firmware > > >> is doing is legitimate, but I don't like having a situation where we > > >> have to depend on the DXE spec. > > > > > > It's in the process of getting into the UEFI spec now as > > > https://bugzilla.tianocore.org/show_bug.cgi?id=3519 . > > > > > >> How does Windows handle this? Just update the page tables itself for > > >> any > > >> regions it needs during boot? > > > > > > Microsoft's bootloader sets up its own pagetables, though I believe > > > they're switching it to use the (soon to be) standardized API. > > > > The third version of the patch is the most close in structure > > to the proposed protocol. And until the protocol is standardized and > > implemented on problematic firmware, I think, it remains the better > > solution in terms of simplicity and further porting to the new > > protocol. > > > > It is desirable to get the issue resolved, and make the kernel stricter > > comply to the spec, without waiting for the new API implementation. > > And later, switch the kernel to be using the protocol with > > subsequent patches as soon as it gets usable. > > > > So, is there a chance for these patches to be accepted in current > > form, or with some modifications? > > > > I am fine with taking the v3, as it is the most likely to only affect > the systems that actually need this change in behavior. > > So unless there are any objections, I will queue these up after the > merge window. I have queued these up now.
This is another implementation of this patch. It uses DXE services to change memory protection flags as you suggested earlier. As I mentioned, you can reproduce this issue with any firmware, including OVMF by setting the PcdDxeNxMemoryProtectionPolicy policy: gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0x7FD4 Restricting the user from creating executable pages is not an goal, but restricting one from creating both executable and writable pages is a goal, which is enforced in the firmware we use. We cannot allow allocating pages of any type that have RWX permissions. gDS->SetMemorySpaceAttributes() is technically part of the UEFI PI specification, so it is not too bad to rely on it. However: - DXE services is not something designed to be used by an UEFI application. - From what we know, no other operating system uses this interface, which means that it can easily break in production firmware on the boards we do not control before anyone could even notice. We do not strictly mind experimenting with this route, but it would be preferable for this interface to become more standard in this case: move it to UEFI Boot Services or a separate protocol and include it in UEFI conformance suite. It will also help if we enable this feature in Linux by default. Baskov Evgeniy (2): efi: declare DXE services table libstub: ensure allocated memory to be executable arch/x86/include/asm/efi.h | 5 ++ drivers/firmware/efi/libstub/efistub.h | 53 ++++++++++++++++++++ drivers/firmware/efi/libstub/x86-stub.c | 73 ++++++++++++++++++++++++++-- include/linux/efi.h | 2 + 4 files changed, 128 insertions(+), 5 deletions(-)