| Message ID | 20210820073429.19457-1-joro@8bytes.org |
|---|---|
| State | New |
| Headers | show |
| Series | x86/efi: Restore Firmware IDT in before ExitBootServices() | expand |
On Fri, 20 Aug 2021 at 09:34, Joerg Roedel <joro@8bytes.org> wrote: > > From: Joerg Roedel <jroedel@suse.de> > > Commit 79419e13e808 ("x86/boot/compressed/64: Setup IDT in startup_32 > boot path") introduced an IDT into the 32 bit boot path of the > decompressor stub. But the IDT is set up before ExitBootServices() is > called and some UEFI firmwares rely on their own IDT. > > Save the firmware IDT on boot and restore it before calling into EFI > functions to fix boot failures introduced by above commit. > > Reported-by: Fabio Aiuto <fabioaiuto83@gmail.com> > Fixes: 79419e13e808 ("x86/boot/compressed/64: Setup IDT in startup_32 boot path") > Cc: stable@vger.kernel.org # 5.13+ > Signed-off-by: Joerg Roedel <jroedel@suse.de> Acked by: Ard Biesheuvel <ardb@kernel.org> One nit below > --- > arch/x86/boot/compressed/efi_thunk_64.S | 23 ++++++++++++++++++----- > arch/x86/boot/compressed/head_64.S | 3 +++ > 2 files changed, 21 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/boot/compressed/efi_thunk_64.S b/arch/x86/boot/compressed/efi_thunk_64.S > index 95a223b3e56a..99cfd5dea23c 100644 > --- a/arch/x86/boot/compressed/efi_thunk_64.S > +++ b/arch/x86/boot/compressed/efi_thunk_64.S > @@ -39,7 +39,7 @@ SYM_FUNC_START(__efi64_thunk) > /* > * Convert x86-64 ABI params to i386 ABI > */ > - subq $32, %rsp > + subq $64, %rsp Any reason in particular for the increase by 32? > movl %esi, 0x0(%rsp) > movl %edx, 0x4(%rsp) > movl %ecx, 0x8(%rsp) > @@ -49,14 +49,19 @@ SYM_FUNC_START(__efi64_thunk) > leaq 0x14(%rsp), %rbx > sgdt (%rbx) > > + addq $16, %rbx > + sidt (%rbx) > + > /* > - * Switch to gdt with 32-bit segments. This is the firmware GDT > - * that was installed when the kernel started executing. This > - * pointer was saved at the EFI stub entry point in head_64.S. > + * Switch to idt and gdt with 32-bit segments. This is the firmware GDT > + * and IDT that was installed when the kernel started executing. The > + * pointers were saved at the EFI stub entry point in head_64.S. > * > * Pass the saved DS selector to the 32-bit code, and use far return to > * restore the saved CS selector. > */ > + leaq efi32_boot_idt(%rip), %rax > + lidt (%rax) > leaq efi32_boot_gdt(%rip), %rax > lgdt (%rax) > > @@ -67,7 +72,7 @@ SYM_FUNC_START(__efi64_thunk) > pushq %rax > lretq > > -1: addq $32, %rsp > +1: addq $64, %rsp > movq %rdi, %rax > > pop %rbx > @@ -132,6 +137,9 @@ SYM_FUNC_START_LOCAL(efi_enter32) > */ > cli > > + lidtl (%ebx) > + subl $16, %ebx > + > lgdtl (%ebx) > > movl %cr4, %eax > @@ -166,6 +174,11 @@ SYM_DATA_START(efi32_boot_gdt) > .quad 0 > SYM_DATA_END(efi32_boot_gdt) > > +SYM_DATA_START(efi32_boot_idt) > + .word 0 > + .quad 0 > +SYM_DATA_END(efi32_boot_idt) > + > SYM_DATA_START(efi32_boot_cs) > .word 0 > SYM_DATA_END(efi32_boot_cs) > diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S > index a2347ded77ea..572c535cf45b 100644 > --- a/arch/x86/boot/compressed/head_64.S > +++ b/arch/x86/boot/compressed/head_64.S > @@ -319,6 +319,9 @@ SYM_INNER_LABEL(efi32_pe_stub_entry, SYM_L_LOCAL) > movw %cs, rva(efi32_boot_cs)(%ebp) > movw %ds, rva(efi32_boot_ds)(%ebp) > > + /* Store firmware IDT descriptor */ > + sidtl rva(efi32_boot_idt)(%ebp) > + > /* Disable paging */ > movl %cr0, %eax > btrl $X86_CR0_PG_BIT, %eax > -- > 2.32.0 >
On Fri, Aug 20, 2021 at 09:41:12AM +0200, Ard Biesheuvel wrote: > On Fri, 20 Aug 2021 at 09:34, Joerg Roedel <joro@8bytes.org> wrote: > > Acked by: Ard Biesheuvel <ardb@kernel.org> Thanks. > > > - subq $32, %rsp > > + subq $64, %rsp > > Any reason in particular for the increase by 32? Just wanted it to be a power of two, like before. Before it was 32 bytes, of which 30 were used. Now its 64, of which 40 are used. Regards, Joerg
On Fri, Aug 20, 2021 at 08:43:47AM +0000, David Laight wrote: > Hmmm... > If Linux needs its own IDT then temporarily substituting the old IDT > prior to a UEFI call will cause 'grief' if a 'Linux' interrupt > happens during the UEFI call. This is neede only during very early boot before Linux called ExitBootServices(). Nothing that causes IRQs is set up by Linux yet. Of course the Firmware could have set something up, but Linux runs with IRQs disabled when on its own IDT at that stage. Regards, Joerg
On Fri, Aug 20, 2021 at 09:34:29AM +0200, Joerg Roedel wrote: > From: Joerg Roedel <jroedel@suse.de> > > Commit 79419e13e808 ("x86/boot/compressed/64: Setup IDT in startup_32 > boot path") introduced an IDT into the 32 bit boot path of the > decompressor stub. But the IDT is set up before ExitBootServices() is > called and some UEFI firmwares rely on their own IDT. > > Save the firmware IDT on boot and restore it before calling into EFI > functions to fix boot failures introduced by above commit. > > Reported-by: Fabio Aiuto <fabioaiuto83@gmail.com> > Fixes: 79419e13e808 ("x86/boot/compressed/64: Setup IDT in startup_32 boot path") > Cc: stable@vger.kernel.org # 5.13+ > Signed-off-by: Joerg Roedel <jroedel@suse.de> > --- > arch/x86/boot/compressed/efi_thunk_64.S | 23 ++++++++++++++++++----- > arch/x86/boot/compressed/head_64.S | 3 +++ > 2 files changed, 21 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/boot/compressed/efi_thunk_64.S b/arch/x86/boot/compressed/efi_thunk_64.S > index 95a223b3e56a..99cfd5dea23c 100644 > --- a/arch/x86/boot/compressed/efi_thunk_64.S > +++ b/arch/x86/boot/compressed/efi_thunk_64.S > @@ -39,7 +39,7 @@ SYM_FUNC_START(__efi64_thunk) > /* > * Convert x86-64 ABI params to i386 ABI > */ > - subq $32, %rsp > + subq $64, %rsp > movl %esi, 0x0(%rsp) > movl %edx, 0x4(%rsp) > movl %ecx, 0x8(%rsp) > @@ -49,14 +49,19 @@ SYM_FUNC_START(__efi64_thunk) > leaq 0x14(%rsp), %rbx > sgdt (%rbx) > > + addq $16, %rbx > + sidt (%rbx) > + > /* > - * Switch to gdt with 32-bit segments. This is the firmware GDT > - * that was installed when the kernel started executing. This > - * pointer was saved at the EFI stub entry point in head_64.S. > + * Switch to idt and gdt with 32-bit segments. This is the firmware GDT IDT and GDT, both capitalized pls. Also, the comment at the top of the file needs adjusting. > + * and IDT that was installed when the kernel started executing. The > + * pointers were saved at the EFI stub entry point in head_64.S. > * > * Pass the saved DS selector to the 32-bit code, and use far return to > * restore the saved CS selector. > */ > + leaq efi32_boot_idt(%rip), %rax > + lidt (%rax) > leaq efi32_boot_gdt(%rip), %rax > lgdt (%rax) > > @@ -67,7 +72,7 @@ SYM_FUNC_START(__efi64_thunk) > pushq %rax > lretq > > -1: addq $32, %rsp > +1: addq $64, %rsp > movq %rdi, %rax > > pop %rbx > @@ -132,6 +137,9 @@ SYM_FUNC_START_LOCAL(efi_enter32) > */ Can you pls also extend this comment here to say "IDT" for faster pinpointing when someone like me is looking for the place where the kernel IDT/GDT get restored again. In any case, those are all minor nitpicks, other than that LGTM. Lemme go test it on whatever I can - if others wanna run this, it would be very much appreciated! Thx.
On Fri, 20 Aug 2021 at 12:19, Joerg Roedel <joro@8bytes.org> wrote: > > On Fri, Aug 20, 2021 at 09:02:46AM +0000, David Laight wrote: > > So allocate and initialise the Linux IDT - so entries can be added. > > But don't execute 'lidt' until later on. > > The IDT is needed in this path to handle #VC exceptions caused by CPUID > instructions. So loading the IDT later is not an option. > That does raise a question, though. Does changing the IDT interfere with the ability of the UEFI boot services to receive and handle the timer interrupt? Because before ExitBootServices(), that is owned by the firmware, and UEFI heavily relies on it for everything (event handling, polling mode block/network drivers, etc) If restoring the IDT temporarily just papers over this by creating tiny windows where the timer interrupt starts working again, this is bad, and we need to figure out another way to address the original problem.
On Fri, Aug 20, 2021 at 01:31:36PM +0200, Ard Biesheuvel wrote: > That does raise a question, though. Does changing the IDT interfere > with the ability of the UEFI boot services to receive and handle the > timer interrupt? Because before ExitBootServices(), that is owned by > the firmware, and UEFI heavily relies on it for everything (event > handling, polling mode block/network drivers, etc) Yes it would interfer, if the boot code would run with IRQs enabled, which it does not. But switching the GDT also interfers with that, and we are doing the same switching with the GDT already. > If restoring the IDT temporarily just papers over this by creating > tiny windows where the timer interrupt starts working again, this is > bad, and we need to figure out another way to address the original > problem. As I can see it, there is no time window where an interrupt could happen (NMIs aside). When returning from EFI IRQs are disabled again (in case EFI let them enabled) while still on the EFI GDT and IDT. After IRQs are disabled the kernel restores its own GDT and IDT. Regards, Joerg
From: Ard Biesheuvel > Sent: 20 August 2021 12:32 > > On Fri, 20 Aug 2021 at 12:19, Joerg Roedel <joro@8bytes.org> wrote: > > > > On Fri, Aug 20, 2021 at 09:02:46AM +0000, David Laight wrote: > > > So allocate and initialise the Linux IDT - so entries can be added. > > > But don't execute 'lidt' until later on. > > > > The IDT is needed in this path to handle #VC exceptions caused by CPUID > > instructions. So loading the IDT later is not an option. > > > > That does raise a question, though. Does changing the IDT interfere > with the ability of the UEFI boot services to receive and handle the > timer interrupt? Because before ExitBootServices(), that is owned by > the firmware, and UEFI heavily relies on it for everything (event > handling, polling mode block/network drivers, etc) > > If restoring the IDT temporarily just papers over this by creating > tiny windows where the timer interrupt starts working again, this is > bad, and we need to figure out another way to address the original > problem. Could the whole thing be flipped? So load a temporary IDT so that you can detect invalid instructions and restore the UEFI IDT immediately afterwards? I'm guessing the GDT is changed in order to access all of physical memory (well enough to load the kernel). Could that be done using the LDT? It is unlikely that the UEFI cares about that? Is this 32bit non-paged code? Running that with a physical memory offset made my head hurt. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Fri, Aug 20, 2021 at 03:46:11PM +0000, David Laight wrote: > So load a temporary IDT so that you can detect invalid instructions > and restore the UEFI IDT immediately afterwards? Going forward with SEV-SNP, the IDT is not only needed for special instructions, but also to detect when the hypervisor is doing fishy things with the guests memory, which could happen at _any_ instruction boundary. > I'm guessing the GDT is changed in order to access all of physical > memory (well enough to load the kernel). The kernels GDT is needed to switch from 32-bit protected mode to long mode, where it calls ExitBootServices(). I think the reason is to avoid compiling a 64-bit and a 32-bit EFI library into the decompressor stub. With a 32-bit library the kernel could call ExitBootServices() right away before it jumps to startup_32. But it only has the 64-bit library, so it has to switch to long-mode first before it make subsequent EFI calls. > Could that be done using the LDT? > It is unlikely that the UEFI cares about that? Well, I guess it could work via the LDT too, but the current GDT switching code if proven to work on exiting BIOSes and I'd rather not change it to something less proven when there is no serious problem with it. > Is this 32bit non-paged code? > Running that with a physical memory offset made my head hurt. Yes, 32-bit EFI launches the kernel in 32-bit protected mode, paging disabled. I think that it also has to use a flat segmentation model without offsets. But someone who knows the EFI spec better than me can correct me here :) Regards, Joerg
diff --git a/arch/x86/boot/compressed/efi_thunk_64.S b/arch/x86/boot/compressed/efi_thunk_64.S index 95a223b3e56a..99cfd5dea23c 100644 --- a/arch/x86/boot/compressed/efi_thunk_64.S +++ b/arch/x86/boot/compressed/efi_thunk_64.S @@ -39,7 +39,7 @@ SYM_FUNC_START(__efi64_thunk) /* * Convert x86-64 ABI params to i386 ABI */ - subq $32, %rsp + subq $64, %rsp movl %esi, 0x0(%rsp) movl %edx, 0x4(%rsp) movl %ecx, 0x8(%rsp) @@ -49,14 +49,19 @@ SYM_FUNC_START(__efi64_thunk) leaq 0x14(%rsp), %rbx sgdt (%rbx) + addq $16, %rbx + sidt (%rbx) + /* - * Switch to gdt with 32-bit segments. This is the firmware GDT - * that was installed when the kernel started executing. This - * pointer was saved at the EFI stub entry point in head_64.S. + * Switch to idt and gdt with 32-bit segments. This is the firmware GDT + * and IDT that was installed when the kernel started executing. The + * pointers were saved at the EFI stub entry point in head_64.S. * * Pass the saved DS selector to the 32-bit code, and use far return to * restore the saved CS selector. */ + leaq efi32_boot_idt(%rip), %rax + lidt (%rax) leaq efi32_boot_gdt(%rip), %rax lgdt (%rax) @@ -67,7 +72,7 @@ SYM_FUNC_START(__efi64_thunk) pushq %rax lretq -1: addq $32, %rsp +1: addq $64, %rsp movq %rdi, %rax pop %rbx @@ -132,6 +137,9 @@ SYM_FUNC_START_LOCAL(efi_enter32) */ cli + lidtl (%ebx) + subl $16, %ebx + lgdtl (%ebx) movl %cr4, %eax @@ -166,6 +174,11 @@ SYM_DATA_START(efi32_boot_gdt) .quad 0 SYM_DATA_END(efi32_boot_gdt) +SYM_DATA_START(efi32_boot_idt) + .word 0 + .quad 0 +SYM_DATA_END(efi32_boot_idt) + SYM_DATA_START(efi32_boot_cs) .word 0 SYM_DATA_END(efi32_boot_cs) diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S index a2347ded77ea..572c535cf45b 100644 --- a/arch/x86/boot/compressed/head_64.S +++ b/arch/x86/boot/compressed/head_64.S @@ -319,6 +319,9 @@ SYM_INNER_LABEL(efi32_pe_stub_entry, SYM_L_LOCAL) movw %cs, rva(efi32_boot_cs)(%ebp) movw %ds, rva(efi32_boot_ds)(%ebp) + /* Store firmware IDT descriptor */ + sidtl rva(efi32_boot_idt)(%ebp) + /* Disable paging */ movl %cr0, %eax btrl $X86_CR0_PG_BIT, %eax