Messages in this thread Patch in this message | | | Date | Tue, 22 Nov 2022 15:49:29 -0600 | Subject | Re: [PATCH v3 00/17] x86: head_64.S spring cleaning | From | Tom Lendacky <> |
| |
On 11/22/22 15:37, Ard Biesheuvel wrote: > On Tue, 22 Nov 2022 at 21:48, Tom Lendacky <thomas.lendacky@amd.com> wrote: >> >> On 11/22/22 10:10, Ard Biesheuvel wrote: >>> After doing some cleanup work on the EFI code in head_64.S, the mixed >>> mode code in particular, I noticed that the memory encryption pieces >>> could use some attention as well, so I cleaned that up too. >>> >>> Changes since v2: >>> - add some clarifying comments to the EFI mixed mode changes >>> - include patch to make the EFI handover protocol optional that was sent >>> out separately before >>> - rebase onto tip/master >>> >>> Changes since v1: >>> - at Boris's request, split the patches into smaller ones that are >>> easier to review >>> >>> Cc: Thomas Gleixner <tglx@linutronix.de> >>> Cc: Ingo Molnar <mingo@redhat.com> >>> Cc: Borislav Petkov <bp@alien8.de> >>> Cc: Dave Hansen <dave.hansen@linux.intel.com> >>> Cc: Michael Roth <michael.roth@amd.com> >> >> This causes an SEV guest to blow up on boot in the early boot code. It >> looks like the stack pointer is not valid and it triple faults on a pushq >> instruction (pushq $__KERNEL_CS in arch/x86/boot/compressed/head_64.S of >> startup_64). >> > > Thanks for the report. > > So the mystery here (at least to me) is that all the changes are to > the 32-bit code, and startup_64 reloads the stack pointer from the > symbol
I bisected it to:
99b7f4b23d9f ("x86/boot/compressed, efi: Merge multiple definitions of image_offset into one")
And doing the following fixed it:
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c index cb5f0befee57..a0bfd31358ba 100644 --- a/drivers/firmware/efi/libstub/x86-stub.c +++ b/drivers/firmware/efi/libstub/x86-stub.c @@ -23,7 +23,7 @@ const efi_system_table_t *efi_system_table; const efi_dxe_services_table_t *efi_dxe_table; -u32 image_offset; +u32 image_offset __section(".data"); static efi_loaded_image_t *image = NULL; static efi_status_t I assume it has to do with being in .data vs .bss and not being explicitly cleared with the encryption bit set. With the change to put image_offset in the .data section, it is read as zero, where as when it was in the .bss section it was reading "ciphertext".
Thanks, Tom
> > Does your config have CONFIG_EFI_MIXED enabled? > > Can I reproduce this fully emulated with QEMU? Or do I need a SEV host? > >> Here is the Qemu register dump: >> RAX=00000000029cc260 RBX=ffffffffdd98c000 RCX=0000000000000010 RDX=0000000000000002 >> RSI=000000003dec1000 RDI=0000000000000000 RBP=ffffffffdb000000 RSP=ffffffffde36e000 >> R8 =000000003dec1410 R9 =000000003dec13fc R10=000000000000006c R11=0000000000000000 >> R12=0000000000000000 R13=0000000000000001 R14=0000000000000004 R15=000000003eacdf44 >> RIP=0000000002000263 RFL=00200002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0 >> ES =0000 0000000000000000 00000000 00000000 >> CS =0038 0000000000000000 ffffffff 00a09b00 DPL=0 CS64 [-RA] >> SS =0000 0000000000000000 00000000 00000000 >> DS =0000 0000000000000000 00000000 00000000 >> FS =0000 0000000000000000 00000000 00000000 >> GS =0000 0000000000000000 00000000 00000000 >> LDT=0000 0000000000000000 0000ffff 00008200 DPL=0 LDT >> TR =0000 0000000000000000 0000ffff 00008b00 DPL=0 TSS64-busy >> GDT= 00000000029cc270 0000002f >> IDT= 000000003f55e018 00000fff >> CR0=80010033 CR2=ffffffffde36dff8 CR3=000000003fc01000 CR4=00000668 >> DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000 >> DR6=00000000ffff0ff0 DR7=0000000000000400 >> EFER=0000000000000d00 >> >> Thanks, >> Tom >> >>> >>> Ard Biesheuvel (17): >>> x86/compressed: efi-mixed: rename efi_thunk_64.S to efi-mixed.S >>> x86/compressed: efi-mixed: move 32-bit entrypoint code into .text >>> section >>> x86/compressed: efi-mixed: move bootargs parsing out of 32-bit startup >>> code >>> x86/compressed: efi-mixed: move efi32_pe_entry into .text section >>> x86/compressed: efi-mixed: move efi32_entry out of head_64.S >>> x86/compressed: efi-mixed: move efi32_pe_entry() out of head_64.S >>> x86/compressed: efi: merge multiple definitions of image_offset into >>> one >>> x86/compressed: efi-mixed: simplify IDT/GDT preserve/restore >>> x86/compressed: avoid touching ECX in startup32_set_idt_entry() >>> x86/compressed: pull global variable ref up into startup32_load_idt() >>> x86/compressed: move startup32_load_idt() into .text section >>> x86/compressed: move startup32_load_idt() out of head_64.S >>> x86/compressed: move startup32_check_sev_cbit() into .text >>> x86/compressed: move startup32_check_sev_cbit() out of head_64.S >>> x86/compressed: adhere to calling convention in >>> get_sev_encryption_bit() >>> x86/compressed: only build mem_encrypt.S if AMD_MEM_ENCRYPT=y >>> efi: x86: Make the deprecated EFI handover protocol optional >>> >>> arch/x86/Kconfig | 17 + >>> arch/x86/boot/compressed/Makefile | 8 +- >>> arch/x86/boot/compressed/efi_mixed.S | 351 ++++++++++++++++++++ >>> arch/x86/boot/compressed/efi_thunk_64.S | 195 ----------- >>> arch/x86/boot/compressed/head_32.S | 4 - >>> arch/x86/boot/compressed/head_64.S | 303 +---------------- >>> arch/x86/boot/compressed/mem_encrypt.S | 152 ++++++++- >>> arch/x86/boot/header.S | 2 +- >>> arch/x86/boot/tools/build.c | 2 + >>> drivers/firmware/efi/libstub/x86-stub.c | 2 +- >>> 10 files changed, 533 insertions(+), 503 deletions(-) >>> create mode 100644 arch/x86/boot/compressed/efi_mixed.S >>> delete mode 100644 arch/x86/boot/compressed/efi_thunk_64.S >>>
| |