[PATCH] x86/efi: Restore Firmware IDT in before ExitBootServices()

[PATCH] x86/efi: Restore Firmware IDT in before ExitBootServices()

[Joerg Roedel]

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
-- 
2.32.0

Re: [PATCH] x86/efi: Restore Firmware IDT in before ExitBootServices()

[Ard Biesheuvel]

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
>

Re: [PATCH] x86/efi: Restore Firmware IDT in before ExitBootServices()

[Joerg Roedel]

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

RE: [PATCH] x86/efi: Restore Firmware IDT in before ExitBootServices()

[David Laight]

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)

Re: [PATCH] x86/efi: Restore Firmware IDT in before ExitBootServices()

['Joerg Roedel']

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

RE: [PATCH] x86/efi: Restore Firmware IDT in before ExitBootServices()

[David Laight]

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)

Re: [PATCH] x86/efi: Restore Firmware IDT in before ExitBootServices()

['Joerg Roedel']

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

Re: [PATCH] x86/efi: Restore Firmware IDT in before ExitBootServices()

[Ard Biesheuvel]

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.

Re: [PATCH] x86/efi: Restore Firmware IDT in before ExitBootServices()

[Joerg Roedel]

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

RE: [PATCH] x86/efi: Restore Firmware IDT in before ExitBootServices()

[David Laight]

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)

Re: [PATCH] x86/efi: Restore Firmware IDT in before ExitBootServices()

[Joerg Roedel]

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

Re: [PATCH] x86/efi: Restore Firmware IDT in before ExitBootServices()

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/efi: Restore Firmware IDT in before ExitBootServices()

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette