diff mbox series

x86/efi: Make efi_set_virtual_address_map IBT safe

Message ID 87jzvm12q0.ffs@tglx
State New
Headers
Series x86/efi: Make efi_set_virtual_address_map IBT safe |

Commit Message

Thomas Gleixner June 29, 2023, 7:35 p.m. UTC 
  Niklāvs reported a boot regression on an Alderlake machine and bisected it
to commit 9df9d2f0471b ("init: Invoke arch_cpu_finalize_init() earlier").

By moving the invocation of arch_cpu_finalize_init() further down he
identified that efi_enter_virtual_mode() is the function which causes
the boot hang.

The main difference of the earlier invocation is that the boot CPU is
already fully initialized and mitigations and alternatives are applied.

But the only really interesting change turned out to be IBT, which is
now enabled before efi_enter_virtual_mode(). "ibt=off" on the kernel
command line cured the problem.

Inspection of the involved calls in efi_enter_virtual_mode() unearthed that
efi_set_virtual_address_map() is the only place in the kernel which invokes
an EFI call without the IBT safe wrapper. This went obviously unnoticed so
far as IBT was enabled later.

Use arch_efi_call_virt() instead of efi_call() to cure that.

Fixes: fe379fa4d199 ("x86/ibt: Disable IBT around firmware")
Fixes: 9df9d2f0471b ("init: Invoke arch_cpu_finalize_init() earlier")
Reported-by: Niklāvs Koļesņikovs <pinkflames.linux@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=217602
---
I put two fixes tags in as the IBT one missed that and the earlier
invocation unearthed it and made it observable.
---
 arch/x86/platform/efi/efi_64.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Ard Biesheuvel June 29, 2023, 9:42 p.m. UTC | #1 
On Thu, 29 Jun 2023 at 21:35, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Niklāvs reported a boot regression on an Alderlake machine and bisected it
> to commit 9df9d2f0471b ("init: Invoke arch_cpu_finalize_init() earlier").
>
> By moving the invocation of arch_cpu_finalize_init() further down he
> identified that efi_enter_virtual_mode() is the function which causes
> the boot hang.
>
> The main difference of the earlier invocation is that the boot CPU is
> already fully initialized and mitigations and alternatives are applied.
>
> But the only really interesting change turned out to be IBT, which is
> now enabled before efi_enter_virtual_mode(). "ibt=off" on the kernel
> command line cured the problem.
>
> Inspection of the involved calls in efi_enter_virtual_mode() unearthed that
> efi_set_virtual_address_map() is the only place in the kernel which invokes
> an EFI call without the IBT safe wrapper. This went obviously unnoticed so
> far as IBT was enabled later.
>
> Use arch_efi_call_virt() instead of efi_call() to cure that.
>
> Fixes: fe379fa4d199 ("x86/ibt: Disable IBT around firmware")
> Fixes: 9df9d2f0471b ("init: Invoke arch_cpu_finalize_init() earlier")
> Reported-by: Niklāvs Koļesņikovs <pinkflames.linux@gmail.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217602

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

I take it you'll send this straight to Linus?


> ---
> I put two fixes tags in as the IBT one missed that and the earlier
> invocation unearthed it and made it observable.
> ---
>  arch/x86/platform/efi/efi_64.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -853,9 +853,9 @@ efi_set_virtual_address_map(unsigned lon
>
>         /* Disable interrupts around EFI calls: */
>         local_irq_save(flags);
> -       status = efi_call(efi.runtime->set_virtual_address_map,
> -                         memory_map_size, descriptor_size,
> -                         descriptor_version, virtual_map);
> +       status = arch_efi_call_virt(efi.runtime, set_virtual_address_map,
> +                                   memory_map_size, descriptor_size,
> +                                   descriptor_version, virtual_map);
>         local_irq_restore(flags);
>
>         efi_fpu_end();
Thomas Gleixner June 29, 2023, 9:45 p.m. UTC | #2 
On Thu, Jun 29 2023 at 23:42, Ard Biesheuvel wrote:
> On Thu, 29 Jun 2023 at 21:35, Thomas Gleixner <tglx@linutronix.de> wrote:
>> Niklāvs reported a boot regression on an Alderlake machine and bisected it
>> to commit 9df9d2f0471b ("init: Invoke arch_cpu_finalize_init() earlier").
>>
>> By moving the invocation of arch_cpu_finalize_init() further down he
>> identified that efi_enter_virtual_mode() is the function which causes
>> the boot hang.
>>
>> The main difference of the earlier invocation is that the boot CPU is
>> already fully initialized and mitigations and alternatives are applied.
>>
>> But the only really interesting change turned out to be IBT, which is
>> now enabled before efi_enter_virtual_mode(). "ibt=off" on the kernel
>> command line cured the problem.
>>
>> Inspection of the involved calls in efi_enter_virtual_mode() unearthed that
>> efi_set_virtual_address_map() is the only place in the kernel which invokes
>> an EFI call without the IBT safe wrapper. This went obviously unnoticed so
>> far as IBT was enabled later.
>>
>> Use arch_efi_call_virt() instead of efi_call() to cure that.
>>
>> Fixes: fe379fa4d199 ("x86/ibt: Disable IBT around firmware")
>> Fixes: 9df9d2f0471b ("init: Invoke arch_cpu_finalize_init() earlier")
>> Reported-by: Niklāvs Koļesņikovs <pinkflames.linux@gmail.com>
>> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217602
>
> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
>
> I take it you'll send this straight to Linus?

I can do that.

Thanks,

        tglx
diff mbox series

Patch

--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -853,9 +853,9 @@  efi_set_virtual_address_map(unsigned lon
 
 	/* Disable interrupts around EFI calls: */
 	local_irq_save(flags);
-	status = efi_call(efi.runtime->set_virtual_address_map,
-			  memory_map_size, descriptor_size,
-			  descriptor_version, virtual_map);
+	status = arch_efi_call_virt(efi.runtime, set_virtual_address_map,
+				    memory_map_size, descriptor_size,
+				    descriptor_version, virtual_map);
 	local_irq_restore(flags);
 
 	efi_fpu_end();