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 + * 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
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, %rbxsidt (%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), %raxlidt (%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, %ebxlgdtl (%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, %rspAny 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
From: Joerg Roedel
Sent: 20 August 2021 08:34
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.
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.
So I suspect you just can't make EFI calls after installing the Linux IDT.
Looks like UEFI is no better than the traditional BIOS. Great fun trying to reliably switch from 32bit paged to 16bit segmented and back (especially on VIA C3) and discovering that that bios code enables interrupts - so all hell happens in the ISR entry path.
It may be that the only safe way to make UEFI calls (after the very initial entry code) is using an emulator.
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
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
From: 'Joerg Roedel'
Sent: 20 August 2021 09:52
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.
So allocate and initialise the Linux IDT - so entries can be added. But don't execute 'lidt' until later on.
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
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.
Regards,
Joerg
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
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, Aug 20, 2021 at 11:26:00AM +0200, Borislav Petkov wrote:
Lemme go test it on whatever I can - if others wanna run this, it would be very much appreciated!
FWIW, it boots fine here on my machines - not that it means a whole lot.
linux-stable-mirror@lists.linaro.org
-
'Joerg Roedel' -
Ard Biesheuvel -
Borislav Petkov -
David Laight -
Joerg Roedel
-
Joerg Roedel