x86/efi: Restore Firmware IDT in before ExitBootServices()
Subsystems (2)
PATCH 1/1 x86/efi: Restore Firmware IDT in before ExitBootServices()
From: Joerg Roedel <[email protected]>
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.
--- arch/x86/boot/compressed/efi_thunk_64.S | 23 ++++++++++++++++++----- arch/x86/boot/compressed/head_64.S | 3 +++ 2 files changed, 21 insertions(+), 5 deletions(-)
arch/x86/boot/compressed/efi_thunk_64.S +18 -5
@@ -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)
arch/x86/boot/compressed/head_64.S +3 -0
@@ -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
Signature · 1 line
2.32.0
Quoted 8 lines · Joerg Roedel
From: Joerg Roedel <[email protected]>
Commit
79419e13e808("x86/boot/compressed/64: Setup IDT instartup_32boot path") introduced an IDT into the 32 bit boot path of the decompressor stub. But the IDT is set up beforeExitBootServices()is called and some UEFI firmwares rely on their own IDT.
Quoted 7 lines
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 <[email protected]>Fixes 79419e13e808 ("x86/boot/compressed/64: Setup IDT in startup_32 boot path")Cc [email protected] # 5.13+Signed-off-by Joerg Roedel <[email protected]>
Acked by: Ard Biesheuvel <[email protected]>
One nit below
Quoted 10 lines
--- arch/x86/boot/compressed/efi_thunk_64.S | 23 ++++++++++++++++++----- arch/x86/boot/compressed/head_64.S | 3 +++ 2 files changed, 21 insertions(+), 5 deletions(-)arch/x86/boot/compressed/efi_thunk_64.S +0
-0
arch/x86/boot/compressed/efi_thunk_64.S SYM_FUNC_START(__efi64_thunk) · line 42
Quoted 5 lines
arch/x86/boot/compressed/efi_thunk_64.S +1
-1@@ -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?
Quoted 74 lines
arch/x86/boot/compressed/efi_thunk_64.S +17
-4movl %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)arch/x86/boot/compressed/head_64.S +3
-1@@ -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
Quoted 3 lines · Ard Biesheuvel
On Fri, 20 Aug 2021 at 09:34, Joerg Roedel <[email protected]> wrote:
Acked by: Ard Biesheuvel <[email protected]>
Thanks.
Quoted 5 lines
arch/x86/boot/compressed/efi_thunk_64.S SYM_FUNC_START(__efi64_thunk) · line 42
Quoted 3 lines
arch/x86/boot/compressed/efi_thunk_64.S +1
-1- 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
Quoted 11 lines
Sent: 20 August 2021 08:34
From: Joerg Roedel <[email protected]>
Commit
79419e13e808("x86/boot/compressed/64: Setup IDT instartup_32boot path") introduced an IDT into the 32 bit boot path of the decompressor stub. But the IDT is set up beforeExitBootServices()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)
Quoted 4 lines · David Laight
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'
Quoted 8 lines
Sent: 20 August 2021 09:52
Quoted 4 lines · David Laight
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.
Quoted 4 lines
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)
Quoted 2 lines · David Laight
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
Quoted 8 lines · Joerg Roedel
Quoted 2 lines · David Laight
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.
Quoted 5 lines · Ard Biesheuvel
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.
Quoted 4 lines
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
Quoted 18 lines
Sent: 20 August 2021 12:32
Quoted 8 lines · Joerg Roedel
Quoted 2 lines · David Laight
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)
Quoted 4 lines
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)
Quoted 2 lines · David Laight
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.
Quoted 2 lines
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.
Quoted 2 lines
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.
Quoted 2 lines
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
Borislav Petkov
maintainerQuoted 34 lines · Joerg Roedel
From: Joerg Roedel <[email protected]>
Commit
79419e13e808("x86/boot/compressed/64: Setup IDT instartup_32boot path") introduced an IDT into the 32 bit boot path of the decompressor stub. But the IDT is set up beforeExitBootServices()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 <[email protected]>Fixes 79419e13e808 ("x86/boot/compressed/64: Setup IDT in startup_32 boot path")Cc [email protected] # 5.13+Signed-off-by Joerg Roedel <[email protected]>--- arch/x86/boot/compressed/efi_thunk_64.S | 23 ++++++++++++++++++----- arch/x86/boot/compressed/head_64.S | 3 +++ 2 files changed, 21 insertions(+), 5 deletions(-)arch/x86/boot/compressed/efi_thunk_64.S +1
-1@@ -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
arch/x86/boot/compressed/efi_thunk_64.S SYM_FUNC_START(__efi64_thunk) · line 56
Quoted 10 lines
arch/x86/boot/compressed/efi_thunk_64.S +4
-3sgdt (%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.
Quoted 21 lines
arch/x86/boot/compressed/efi_thunk_64.S +5
-1+ * 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
arch/x86/boot/compressed/efi_thunk_64.S SYM_FUNC_START_LOCAL(efi_enter32) · line 137
Quoted 1 line
arch/x86/boot/compressed/efi_thunk_64.S +0
-0@@ -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.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteBorislav Petkov
maintainerQuoted 2 lines · Borislav Petkov
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.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette