[0/6] efi/x86: Avoid legacy decompressor during EFI boot

  This series is conceptually a combination of Evgeny's series [0] and
mine [1], both of which attempt to make the early decompressor code more
amenable to executing in the EFI environment with stricter handling of
memory permissions.

My series [1] implemented zboot for x86, by getting rid of the entire
x86 decompressor, and replacing it with existing EFI code that does the
same but in a generic way. The downside of this is that only EFI boot is
supported, making it unviable for distros, which need to support BIOS
boot and hybrid EFI boot modes that omit the EFI stub.

Evgeny's series [0] adapted the entire decompressor code flow to allow
it to execute in the EFI context as well as the bare metal context, and
this involves changes to the 1:1 mapping code and the page fault
handlers etc, none of which are really needed when doing EFI boot in the
first place.

So this series attempts to occupy the middle ground here: it makes
minimal changes to the existing decompressor so some of it can be called
from the EFI stub. Then, it reimplements the EFI boot flow to decompress
the kernel and boot it directly, without relying on the trampoline code,
page table code or page fault handling code. This allows us to get rid
of quite a bit of unsavory EFI stub code, and replace it with two clear
invocations of the EFI firmware APIs to clear NX restrictions from
allocations that have been populated with executable code. 

The only code that is being reused is the decompression library itself,
along with the minimal ELF parsing that is required to copy the ELF
segments in place, and the relocation processing that fixes up absolute
symbol references to refer to the correct virtual addresses.

Note that some of Evgeny's changes to clean up the PE/COFF header
generation will still be needed, but I've omitted those here for
brevity.

Cc: Evgeniy Baskov <baskov@ispras.ru>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Alexey Khoroshilov <khoroshilov@ispras.ru>
Cc: Peter Jones <pjones@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Dave Young <dyoung@redhat.com>
Cc: Mario Limonciello <mario.limonciello@amd.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>

[0] https://lore.kernel.org/all/cover.1678785672.git.baskov@ispras.ru/
[1] https://lore.kernel.org/all/20230416120729.2470762-1-ardb@kernel.org/

Ard Biesheuvel (6):
  x86: decompressor: Move global symbol references to C code
  x86: decompressor: Factor out kernel decompression and relocation
  x86: efistub: Obtain ACPI RSDP address while running in the stub
  x86: efistub: Perform 4/5 level paging switch from the stub
  x86: efistub: Prefer EFI memory attributes protocol over DXE services
  x86: efistub: Avoid legacy decompressor when doing EFI boot

 arch/x86/boot/compressed/efi_mixed.S           |  55 ---
 arch/x86/boot/compressed/head_32.S             |  24 --
 arch/x86/boot/compressed/head_64.S             |  39 +--
 arch/x86/boot/compressed/misc.c                |  44 ++-
 arch/x86/include/asm/efi.h                     |   2 +
 drivers/firmware/efi/libstub/efi-stub-helper.c |   4 +
 drivers/firmware/efi/libstub/x86-stub.c        | 360 +++++++++++++-------
 7 files changed, 279 insertions(+), 249 deletions(-)
  

On Mon, Apr 24, 2023 at 06:57:20PM +0200, Ard Biesheuvel wrote:
>  arch/x86/boot/compressed/efi_mixed.S           |  55 ---
>  arch/x86/boot/compressed/head_32.S             |  24 --
>  arch/x86/boot/compressed/head_64.S             |  39 +--
>  arch/x86/boot/compressed/misc.c                |  44 ++-
>  arch/x86/include/asm/efi.h                     |   2 +
>  drivers/firmware/efi/libstub/efi-stub-helper.c |   4 +
>  drivers/firmware/efi/libstub/x86-stub.c        | 360 +++++++++++++-------
>  7 files changed, 279 insertions(+), 249 deletions(-)

Upon a quick scan, I can't argue with that diffstat and would prefer
a lot more if we did this instead of Evgeny's pile which touches a lot
of nasty and hard to debug code which gets executed on *everything*.

So if people agree with that approach, I'd gladly give it a more
detailed look.

Thx.
  

On Wed, 26 Apr 2023 at 11:18, Borislav Petkov <bp@alien8.de> wrote:
>
> On Mon, Apr 24, 2023 at 06:57:20PM +0200, Ard Biesheuvel wrote:
> >  arch/x86/boot/compressed/efi_mixed.S           |  55 ---
> >  arch/x86/boot/compressed/head_32.S             |  24 --
> >  arch/x86/boot/compressed/head_64.S             |  39 +--
> >  arch/x86/boot/compressed/misc.c                |  44 ++-
> >  arch/x86/include/asm/efi.h                     |   2 +
> >  drivers/firmware/efi/libstub/efi-stub-helper.c |   4 +
> >  drivers/firmware/efi/libstub/x86-stub.c        | 360 +++++++++++++-------
> >  7 files changed, 279 insertions(+), 249 deletions(-)
>
> Upon a quick scan, I can't argue with that diffstat and would prefer
> a lot more if we did this instead of Evgeny's pile which touches a lot
> of nasty and hard to debug code which gets executed on *everything*.
>
> So if people agree with that approach, I'd gladly give it a more
> detailed look.
>

I think the general approach is better, but to be honest, I may have
missed a thing or two, so it would be good if people more familiar
with the history could chime in.
  

On 2023-04-24 19:57, Ard Biesheuvel wrote:
> This series is conceptually a combination of Evgeny's series [0] and
> mine [1], both of which attempt to make the early decompressor code 
> more
> amenable to executing in the EFI environment with stricter handling of
> memory permissions.
> 
> My series [1] implemented zboot for x86, by getting rid of the entire
> x86 decompressor, and replacing it with existing EFI code that does the
> same but in a generic way. The downside of this is that only EFI boot 
> is
> supported, making it unviable for distros, which need to support BIOS
> boot and hybrid EFI boot modes that omit the EFI stub.
> 
> Evgeny's series [0] adapted the entire decompressor code flow to allow
> it to execute in the EFI context as well as the bare metal context, and
> this involves changes to the 1:1 mapping code and the page fault
> handlers etc, none of which are really needed when doing EFI boot in 
> the
> first place.
> 
> So this series attempts to occupy the middle ground here: it makes
> minimal changes to the existing decompressor so some of it can be 
> called
> from the EFI stub. Then, it reimplements the EFI boot flow to 
> decompress
> the kernel and boot it directly, without relying on the trampoline 
> code,
> page table code or page fault handling code. This allows us to get rid
> of quite a bit of unsavory EFI stub code, and replace it with two clear
> invocations of the EFI firmware APIs to clear NX restrictions from
> allocations that have been populated with executable code.
> 
> The only code that is being reused is the decompression library itself,
> along with the minimal ELF parsing that is required to copy the ELF
> segments in place, and the relocation processing that fixes up absolute
> symbol references to refer to the correct virtual addresses.
> 
> Note that some of Evgeny's changes to clean up the PE/COFF header
> generation will still be needed, but I've omitted those here for
> brevity.

My series also implements W^X for both UEFI and non-UEFI boot paths, but 
I
agree that we can just consider non-UEFI code legacy and it would be 
better
to avoid touching it and encourage everyone to use UEFI code path on x86
instead. If PE format will also get fixed with either my patches or some
others, I do like your approach more than mine, as it removes a lot of 
old
cruft but does not break things (as far as I see). Seems like a perfect
compromise between [1] and my approach.

I've briefly tested the patches and looked through them and they look 
good
to me. Two things I've noticed:
  * there's one TSC-related TODO;
  * probably we want to clear .bss in efi32_stub_entry and 
efi64_stub_entry
    for UEFI handover protocol, since it's unfortunately still present 
and
    .bss will contain garbage.
I'll probably do some more testing on the weekend and let you know if I
find something.

Please tell me if/when you are going to merge these or similar, and I 
will
clean up and rebase PE-related patches on top of these.

I'd also like to send W^X patches for EFISTUB (omitting the non-UEFI 
boot
path) as a follow up after the PE file header will get fixed. They will 
be
considerably smaller with this approach and will not touch legacy code.

Thanks,
Evgeniy Baskov

> 
> Cc: Evgeniy Baskov <baskov@ispras.ru>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Alexey Khoroshilov <khoroshilov@ispras.ru>
> Cc: Peter Jones <pjones@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Dave Young <dyoung@redhat.com>
> Cc: Mario Limonciello <mario.limonciello@amd.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> 
> [0] https://lore.kernel.org/all/cover.1678785672.git.baskov@ispras.ru/
> [1] 
> https://lore.kernel.org/all/20230416120729.2470762-1-ardb@kernel.org/
> 
> Ard Biesheuvel (6):
>   x86: decompressor: Move global symbol references to C code
>   x86: decompressor: Factor out kernel decompression and relocation
>   x86: efistub: Obtain ACPI RSDP address while running in the stub
>   x86: efistub: Perform 4/5 level paging switch from the stub
>   x86: efistub: Prefer EFI memory attributes protocol over DXE services
>   x86: efistub: Avoid legacy decompressor when doing EFI boot
> 
>  arch/x86/boot/compressed/efi_mixed.S           |  55 ---
>  arch/x86/boot/compressed/head_32.S             |  24 --
>  arch/x86/boot/compressed/head_64.S             |  39 +--
>  arch/x86/boot/compressed/misc.c                |  44 ++-
>  arch/x86/include/asm/efi.h                     |   2 +
>  drivers/firmware/efi/libstub/efi-stub-helper.c |   4 +
>  drivers/firmware/efi/libstub/x86-stub.c        | 360 
> +++++++++++++-------
>  7 files changed, 279 insertions(+), 249 deletions(-)
  

On Fri, 28 Apr 2023 at 14:23, Evgeniy Baskov <baskov@ispras.ru> wrote:
>
> On 2023-04-24 19:57, Ard Biesheuvel wrote:
> > This series is conceptually a combination of Evgeny's series [0] and
> > mine [1], both of which attempt to make the early decompressor code
> > more
> > amenable to executing in the EFI environment with stricter handling of
> > memory permissions.
> >
> > My series [1] implemented zboot for x86, by getting rid of the entire
> > x86 decompressor, and replacing it with existing EFI code that does the
> > same but in a generic way. The downside of this is that only EFI boot
> > is
> > supported, making it unviable for distros, which need to support BIOS
> > boot and hybrid EFI boot modes that omit the EFI stub.
> >
> > Evgeny's series [0] adapted the entire decompressor code flow to allow
> > it to execute in the EFI context as well as the bare metal context, and
> > this involves changes to the 1:1 mapping code and the page fault
> > handlers etc, none of which are really needed when doing EFI boot in
> > the
> > first place.
> >
> > So this series attempts to occupy the middle ground here: it makes
> > minimal changes to the existing decompressor so some of it can be
> > called
> > from the EFI stub. Then, it reimplements the EFI boot flow to
> > decompress
> > the kernel and boot it directly, without relying on the trampoline
> > code,
> > page table code or page fault handling code. This allows us to get rid
> > of quite a bit of unsavory EFI stub code, and replace it with two clear
> > invocations of the EFI firmware APIs to clear NX restrictions from
> > allocations that have been populated with executable code.
> >
> > The only code that is being reused is the decompression library itself,
> > along with the minimal ELF parsing that is required to copy the ELF
> > segments in place, and the relocation processing that fixes up absolute
> > symbol references to refer to the correct virtual addresses.
> >
> > Note that some of Evgeny's changes to clean up the PE/COFF header
> > generation will still be needed, but I've omitted those here for
> > brevity.
>
> My series also implements W^X for both UEFI and non-UEFI boot paths, but
> I
> agree that we can just consider non-UEFI code legacy and it would be
> better
> to avoid touching it and encourage everyone to use UEFI code path on x86
> instead. If PE format will also get fixed with either my patches or some
> others, I do like your approach more than mine, as it removes a lot of
> old
> cruft but does not break things (as far as I see). Seems like a perfect
> compromise between [1] and my approach.
>

Thanks, I'm glad we agree.

> I've briefly tested the patches and looked through them and they look
> good
> to me. Two things I've noticed:
>   * there's one TSC-related TODO;
>   * probably we want to clear .bss in efi32_stub_entry and
> efi64_stub_entry
>     for UEFI handover protocol, since it's unfortunately still present
> and
>     .bss will contain garbage.

I'll look into that.

> I'll probably do some more testing on the weekend and let you know if I
> find something.
>

Yes, please.

> Please tell me if/when you are going to merge these or similar, and I
> will
> clean up and rebase PE-related patches on top of these.
>
> I'd also like to send W^X patches for EFISTUB (omitting the non-UEFI
> boot
> path) as a follow up after the PE file header will get fixed. They will
> be
> considerably smaller with this approach and will not touch legacy code.
>
  

On 4/24/23 11:57, Ard Biesheuvel wrote:
> This series is conceptually a combination of Evgeny's series [0] and
> mine [1], both of which attempt to make the early decompressor code more
> amenable to executing in the EFI environment with stricter handling of
> memory permissions.
> 
> My series [1] implemented zboot for x86, by getting rid of the entire
> x86 decompressor, and replacing it with existing EFI code that does the
> same but in a generic way. The downside of this is that only EFI boot is
> supported, making it unviable for distros, which need to support BIOS
> boot and hybrid EFI boot modes that omit the EFI stub.
> 
> Evgeny's series [0] adapted the entire decompressor code flow to allow
> it to execute in the EFI context as well as the bare metal context, and
> this involves changes to the 1:1 mapping code and the page fault
> handlers etc, none of which are really needed when doing EFI boot in the
> first place.
> 
> So this series attempts to occupy the middle ground here: it makes
> minimal changes to the existing decompressor so some of it can be called
> from the EFI stub. Then, it reimplements the EFI boot flow to decompress
> the kernel and boot it directly, without relying on the trampoline code,
> page table code or page fault handling code. This allows us to get rid
> of quite a bit of unsavory EFI stub code, and replace it with two clear
> invocations of the EFI firmware APIs to clear NX restrictions from
> allocations that have been populated with executable code.
> 
> The only code that is being reused is the decompression library itself,
> along with the minimal ELF parsing that is required to copy the ELF
> segments in place, and the relocation processing that fixes up absolute
> symbol references to refer to the correct virtual addresses.
> 
> Note that some of Evgeny's changes to clean up the PE/COFF header
> generation will still be needed, but I've omitted those here for
> brevity.

I tried booting an SEV and an SEV-ES guest using this and both failed to boot:

EFI stub: WARNING: Decompression failed: Out of memory while allocating 
z_stream

I'll have to take a closer look as to why, but it might be a couple of 
days before I can get to it.

Thanks,
Tom

> 
> Cc: Evgeniy Baskov <baskov@ispras.ru>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Alexey Khoroshilov <khoroshilov@ispras.ru>
> Cc: Peter Jones <pjones@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Dave Young <dyoung@redhat.com>
> Cc: Mario Limonciello <mario.limonciello@amd.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> 
> [0] https://lore.kernel.org/all/cover.1678785672.git.baskov@ispras.ru/
> [1] https://lore.kernel.org/all/20230416120729.2470762-1-ardb@kernel.org/
> 
> Ard Biesheuvel (6):
>    x86: decompressor: Move global symbol references to C code
>    x86: decompressor: Factor out kernel decompression and relocation
>    x86: efistub: Obtain ACPI RSDP address while running in the stub
>    x86: efistub: Perform 4/5 level paging switch from the stub
>    x86: efistub: Prefer EFI memory attributes protocol over DXE services
>    x86: efistub: Avoid legacy decompressor when doing EFI boot
> 
>   arch/x86/boot/compressed/efi_mixed.S           |  55 ---
>   arch/x86/boot/compressed/head_32.S             |  24 --
>   arch/x86/boot/compressed/head_64.S             |  39 +--
>   arch/x86/boot/compressed/misc.c                |  44 ++-
>   arch/x86/include/asm/efi.h                     |   2 +
>   drivers/firmware/efi/libstub/efi-stub-helper.c |   4 +
>   drivers/firmware/efi/libstub/x86-stub.c        | 360 +++++++++++++-------
>   7 files changed, 279 insertions(+), 249 deletions(-)
>
  

On Tue, 2 May 2023 at 15:37, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 4/24/23 11:57, Ard Biesheuvel wrote:
> > This series is conceptually a combination of Evgeny's series [0] and
> > mine [1], both of which attempt to make the early decompressor code more
> > amenable to executing in the EFI environment with stricter handling of
> > memory permissions.
> >
> > My series [1] implemented zboot for x86, by getting rid of the entire
> > x86 decompressor, and replacing it with existing EFI code that does the
> > same but in a generic way. The downside of this is that only EFI boot is
> > supported, making it unviable for distros, which need to support BIOS
> > boot and hybrid EFI boot modes that omit the EFI stub.
> >
> > Evgeny's series [0] adapted the entire decompressor code flow to allow
> > it to execute in the EFI context as well as the bare metal context, and
> > this involves changes to the 1:1 mapping code and the page fault
> > handlers etc, none of which are really needed when doing EFI boot in the
> > first place.
> >
> > So this series attempts to occupy the middle ground here: it makes
> > minimal changes to the existing decompressor so some of it can be called
> > from the EFI stub. Then, it reimplements the EFI boot flow to decompress
> > the kernel and boot it directly, without relying on the trampoline code,
> > page table code or page fault handling code. This allows us to get rid
> > of quite a bit of unsavory EFI stub code, and replace it with two clear
> > invocations of the EFI firmware APIs to clear NX restrictions from
> > allocations that have been populated with executable code.
> >
> > The only code that is being reused is the decompression library itself,
> > along with the minimal ELF parsing that is required to copy the ELF
> > segments in place, and the relocation processing that fixes up absolute
> > symbol references to refer to the correct virtual addresses.
> >
> > Note that some of Evgeny's changes to clean up the PE/COFF header
> > generation will still be needed, but I've omitted those here for
> > brevity.
>
> I tried booting an SEV and an SEV-ES guest using this and both failed to boot:
>
> EFI stub: WARNING: Decompression failed: Out of memory while allocating
> z_stream
>
> I'll have to take a closer look as to why, but it might be a couple of
> days before I can get to it.
>

Thanks Tom.

The internal malloc() seems to be failing, which is often caused by
BSS clearing problems. Could you elaborate a little bit on the boot
environment you are using here?
  

On 5/2/23 08:39, Ard Biesheuvel wrote:
> On Tue, 2 May 2023 at 15:37, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>
>> On 4/24/23 11:57, Ard Biesheuvel wrote:
>>> This series is conceptually a combination of Evgeny's series [0] and
>>> mine [1], both of which attempt to make the early decompressor code more
>>> amenable to executing in the EFI environment with stricter handling of
>>> memory permissions.
>>>
>>> My series [1] implemented zboot for x86, by getting rid of the entire
>>> x86 decompressor, and replacing it with existing EFI code that does the
>>> same but in a generic way. The downside of this is that only EFI boot is
>>> supported, making it unviable for distros, which need to support BIOS
>>> boot and hybrid EFI boot modes that omit the EFI stub.
>>>
>>> Evgeny's series [0] adapted the entire decompressor code flow to allow
>>> it to execute in the EFI context as well as the bare metal context, and
>>> this involves changes to the 1:1 mapping code and the page fault
>>> handlers etc, none of which are really needed when doing EFI boot in the
>>> first place.
>>>
>>> So this series attempts to occupy the middle ground here: it makes
>>> minimal changes to the existing decompressor so some of it can be called
>>> from the EFI stub. Then, it reimplements the EFI boot flow to decompress
>>> the kernel and boot it directly, without relying on the trampoline code,
>>> page table code or page fault handling code. This allows us to get rid
>>> of quite a bit of unsavory EFI stub code, and replace it with two clear
>>> invocations of the EFI firmware APIs to clear NX restrictions from
>>> allocations that have been populated with executable code.
>>>
>>> The only code that is being reused is the decompression library itself,
>>> along with the minimal ELF parsing that is required to copy the ELF
>>> segments in place, and the relocation processing that fixes up absolute
>>> symbol references to refer to the correct virtual addresses.
>>>
>>> Note that some of Evgeny's changes to clean up the PE/COFF header
>>> generation will still be needed, but I've omitted those here for
>>> brevity.
>>
>> I tried booting an SEV and an SEV-ES guest using this and both failed to boot:
>>
>> EFI stub: WARNING: Decompression failed: Out of memory while allocating
>> z_stream
>>
>> I'll have to take a closer look as to why, but it might be a couple of
>> days before I can get to it.
>>
> 
> Thanks Tom.
> 
> The internal malloc() seems to be failing, which is often caused by
> BSS clearing problems. Could you elaborate a little bit on the boot
> environment you are using here?

I'm using Qemu v7.2.1 as my VMM, Linux 6.3 with your series applied for my
host/hypervisor and guest kernel and the current OVMF tree built using
OvmfPkgX64.dsc.

I was originally using the current merge window Linux, but moved to the
release version just to . With the release version SEV and SEV-ES still fail to
boot, but SEV actually #GPs now. And some of the register contents look
like encrypted data:

ConvertPages: range 1000000 - 4FA1FFF covers multiple entries
!!!! X64 Exception Type - 0D(#GP - General Protection)  CPU Apic ID - 00000000 !!!!
ExceptionData - 0000000000000000
RIP  - 00000000597E71C1, CS  - 0000000000000038, RFLAGS - 0000000000210206
RAX  - 1FBA02A45943B920, RCX - 0000000000AF7009, RDX - A9DAE761B64A1F1B
RBX  - 1FBA02A45943B8C0, RSP - 000000007FD97320, RBP - 0000000002000000
RSI  - 0000000001000000, RDI - 1FBA02A45943DE68
R8   - 0000000003EF3C94, R9  - 0000000000000000, R10 - 000000007D7C6018
R11  - 0000000000000000, R12 - 0000000001000000, R13 - 00000000597EDD98
R14  - 0000000001000000, R15 - 000000007E0A5198
DS   - 0000000000000030, ES  - 0000000000000030, FS  - 0000000000000030
GS   - 0000000000000030, SS  - 0000000000000030
CR0  - 0000000080010033, CR2 - 0000000000000000, CR3 - 000000007FA01000
CR4  - 0000000000000668, CR8 - 0000000000000000
DR0  - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000
DR3  - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400
GDTR - 000000007F7DC000 0000000000000047, LDTR - 0000000000000000
IDTR - 000000007F34C018 0000000000000FFF,   TR - 0000000000000000
FXSAVE_STATE - 000000007FD96F80
!!!! Find image based on IP(0x597E71C1) /root/kernels/ovmf-build-X64/Build/OvmfX64/DEBUG_GCC5/X64/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe/DEBUG/Variable
RuntimeDxe.dll (ImageBase=0000000000D4792C, EntryPoint=0000000000D50CC3) !!!!

So, yes, probably an area of memory that was zeroes when mapped
unencrypted, but wasn't cleared after changing the mapping to
encrypted.

Thanks,
Tom
  

On Tue, 2 May 2023 at 18:08, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 5/2/23 08:39, Ard Biesheuvel wrote:
> > On Tue, 2 May 2023 at 15:37, Tom Lendacky <thomas.lendacky@amd.com> wrote:
> >>
> >> On 4/24/23 11:57, Ard Biesheuvel wrote:
> >>> This series is conceptually a combination of Evgeny's series [0] and
> >>> mine [1], both of which attempt to make the early decompressor code more
> >>> amenable to executing in the EFI environment with stricter handling of
> >>> memory permissions.
> >>>
> >>> My series [1] implemented zboot for x86, by getting rid of the entire
> >>> x86 decompressor, and replacing it with existing EFI code that does the
> >>> same but in a generic way. The downside of this is that only EFI boot is
> >>> supported, making it unviable for distros, which need to support BIOS
> >>> boot and hybrid EFI boot modes that omit the EFI stub.
> >>>
> >>> Evgeny's series [0] adapted the entire decompressor code flow to allow
> >>> it to execute in the EFI context as well as the bare metal context, and
> >>> this involves changes to the 1:1 mapping code and the page fault
> >>> handlers etc, none of which are really needed when doing EFI boot in the
> >>> first place.
> >>>
> >>> So this series attempts to occupy the middle ground here: it makes
> >>> minimal changes to the existing decompressor so some of it can be called
> >>> from the EFI stub. Then, it reimplements the EFI boot flow to decompress
> >>> the kernel and boot it directly, without relying on the trampoline code,
> >>> page table code or page fault handling code. This allows us to get rid
> >>> of quite a bit of unsavory EFI stub code, and replace it with two clear
> >>> invocations of the EFI firmware APIs to clear NX restrictions from
> >>> allocations that have been populated with executable code.
> >>>
> >>> The only code that is being reused is the decompression library itself,
> >>> along with the minimal ELF parsing that is required to copy the ELF
> >>> segments in place, and the relocation processing that fixes up absolute
> >>> symbol references to refer to the correct virtual addresses.
> >>>
> >>> Note that some of Evgeny's changes to clean up the PE/COFF header
> >>> generation will still be needed, but I've omitted those here for
> >>> brevity.
> >>
> >> I tried booting an SEV and an SEV-ES guest using this and both failed to boot:
> >>
> >> EFI stub: WARNING: Decompression failed: Out of memory while allocating
> >> z_stream
> >>
> >> I'll have to take a closer look as to why, but it might be a couple of
> >> days before I can get to it.
> >>
> >
> > Thanks Tom.
> >
> > The internal malloc() seems to be failing, which is often caused by
> > BSS clearing problems. Could you elaborate a little bit on the boot
> > environment you are using here?
>
> I'm using Qemu v7.2.1 as my VMM, Linux 6.3 with your series applied for my
> host/hypervisor and guest kernel and the current OVMF tree built using
> OvmfPkgX64.dsc.
>
> I was originally using the current merge window Linux, but moved to the
> release version just to . With the release version SEV and SEV-ES still fail to
> boot, but SEV actually #GPs now. And some of the register contents look
> like encrypted data:
>
> ConvertPages: range 1000000 - 4FA1FFF covers multiple entries
> !!!! X64 Exception Type - 0D(#GP - General Protection)  CPU Apic ID - 00000000 !!!!
> ExceptionData - 0000000000000000
> RIP  - 00000000597E71C1, CS  - 0000000000000038, RFLAGS - 0000000000210206
> RAX  - 1FBA02A45943B920, RCX - 0000000000AF7009, RDX - A9DAE761B64A1F1B
> RBX  - 1FBA02A45943B8C0, RSP - 000000007FD97320, RBP - 0000000002000000
> RSI  - 0000000001000000, RDI - 1FBA02A45943DE68
> R8   - 0000000003EF3C94, R9  - 0000000000000000, R10 - 000000007D7C6018
> R11  - 0000000000000000, R12 - 0000000001000000, R13 - 00000000597EDD98
> R14  - 0000000001000000, R15 - 000000007E0A5198
> DS   - 0000000000000030, ES  - 0000000000000030, FS  - 0000000000000030
> GS   - 0000000000000030, SS  - 0000000000000030
> CR0  - 0000000080010033, CR2 - 0000000000000000, CR3 - 000000007FA01000
> CR4  - 0000000000000668, CR8 - 0000000000000000
> DR0  - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000
> DR3  - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400
> GDTR - 000000007F7DC000 0000000000000047, LDTR - 0000000000000000
> IDTR - 000000007F34C018 0000000000000FFF,   TR - 0000000000000000
> FXSAVE_STATE - 000000007FD96F80
> !!!! Find image based on IP(0x597E71C1) /root/kernels/ovmf-build-X64/Build/OvmfX64/DEBUG_GCC5/X64/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe/DEBUG/Variable
> RuntimeDxe.dll (ImageBase=0000000000D4792C, EntryPoint=0000000000D50CC3) !!!!
>
> So, yes, probably an area of memory that was zeroes when mapped
> unencrypted, but wasn't cleared after changing the mapping to
> encrypted.
>

Thanks.

It seems I was a bit naive and underestimated the amount of SEV
related processing that goes on in the decompressor after the EFI stub
has handed over. I will have to take some time and go through this,
and decide whether there is a way we can share this code with the EFI
stub without introducing yet another permutation that requires testing
and maintenance.

Any suggestions on how to test this stuff is appreciated - does QEMU
emulate any of this? Does consumer-level AMD hardware implement the
pieces I'd need to run a SEV host with SNP support etc?
  

On 5/2/23 11:08, Tom Lendacky wrote:
> On 5/2/23 08:39, Ard Biesheuvel wrote:
>> On Tue, 2 May 2023 at 15:37, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>>
>>> On 4/24/23 11:57, Ard Biesheuvel wrote:
>>>> This series is conceptually a combination of Evgeny's series [0] and
>>>> mine [1], both of which attempt to make the early decompressor code more
>>>> amenable to executing in the EFI environment with stricter handling of
>>>> memory permissions.
>>>>
>>>> My series [1] implemented zboot for x86, by getting rid of the entire
>>>> x86 decompressor, and replacing it with existing EFI code that does the
>>>> same but in a generic way. The downside of this is that only EFI boot is
>>>> supported, making it unviable for distros, which need to support BIOS
>>>> boot and hybrid EFI boot modes that omit the EFI stub.
>>>>
>>>> Evgeny's series [0] adapted the entire decompressor code flow to allow
>>>> it to execute in the EFI context as well as the bare metal context, and
>>>> this involves changes to the 1:1 mapping code and the page fault
>>>> handlers etc, none of which are really needed when doing EFI boot in the
>>>> first place.
>>>>
>>>> So this series attempts to occupy the middle ground here: it makes
>>>> minimal changes to the existing decompressor so some of it can be called
>>>> from the EFI stub. Then, it reimplements the EFI boot flow to decompress
>>>> the kernel and boot it directly, without relying on the trampoline code,
>>>> page table code or page fault handling code. This allows us to get rid
>>>> of quite a bit of unsavory EFI stub code, and replace it with two clear
>>>> invocations of the EFI firmware APIs to clear NX restrictions from
>>>> allocations that have been populated with executable code.
>>>>
>>>> The only code that is being reused is the decompression library itself,
>>>> along with the minimal ELF parsing that is required to copy the ELF
>>>> segments in place, and the relocation processing that fixes up absolute
>>>> symbol references to refer to the correct virtual addresses.
>>>>
>>>> Note that some of Evgeny's changes to clean up the PE/COFF header
>>>> generation will still be needed, but I've omitted those here for
>>>> brevity.
>>>
>>> I tried booting an SEV and an SEV-ES guest using this and both failed 
>>> to boot:
>>>
>>> EFI stub: WARNING: Decompression failed: Out of memory while allocating
>>> z_stream
>>>
>>> I'll have to take a closer look as to why, but it might be a couple of
>>> days before I can get to it.
>>>
>>
>> Thanks Tom.
>>
>> The internal malloc() seems to be failing, which is often caused by
>> BSS clearing problems. Could you elaborate a little bit on the boot
>> environment you are using here?
> 
> I'm using Qemu v7.2.1 as my VMM, Linux 6.3 with your series applied for my
> host/hypervisor and guest kernel and the current OVMF tree built using
> OvmfPkgX64.dsc.
> 
> I was originally using the current merge window Linux, but moved to the
> release version just to . With the release version SEV and SEV-ES still 
> fail to
> boot, but SEV actually #GPs now. And some of the register contents look
> like encrypted data:
> 
> ConvertPages: range 1000000 - 4FA1FFF covers multiple entries
> !!!! X64 Exception Type - 0D(#GP - General Protection)  CPU Apic ID - 
> 00000000 !!!!
> ExceptionData - 0000000000000000
> RIP  - 00000000597E71C1, CS  - 0000000000000038, RFLAGS - 0000000000210206
> RAX  - 1FBA02A45943B920, RCX - 0000000000AF7009, RDX - A9DAE761B64A1F1B
> RBX  - 1FBA02A45943B8C0, RSP - 000000007FD97320, RBP - 0000000002000000
> RSI  - 0000000001000000, RDI - 1FBA02A45943DE68
> R8   - 0000000003EF3C94, R9  - 0000000000000000, R10 - 000000007D7C6018
> R11  - 0000000000000000, R12 - 0000000001000000, R13 - 00000000597EDD98
> R14  - 0000000001000000, R15 - 000000007E0A5198
> DS   - 0000000000000030, ES  - 0000000000000030, FS  - 0000000000000030
> GS   - 0000000000000030, SS  - 0000000000000030
> CR0  - 0000000080010033, CR2 - 0000000000000000, CR3 - 000000007FA01000
> CR4  - 0000000000000668, CR8 - 0000000000000000
> DR0  - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000
> DR3  - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400
> GDTR - 000000007F7DC000 0000000000000047, LDTR - 0000000000000000
> IDTR - 000000007F34C018 0000000000000FFF,   TR - 0000000000000000
> FXSAVE_STATE - 000000007FD96F80
> !!!! Find image based on IP(0x597E71C1) 
> /root/kernels/ovmf-build-X64/Build/OvmfX64/DEBUG_GCC5/X64/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe/DEBUG/Variable
> RuntimeDxe.dll (ImageBase=0000000000D4792C, EntryPoint=0000000000D50CC3) !!!!
> 
> So, yes, probably an area of memory that was zeroes when mapped
> unencrypted, but wasn't cleared after changing the mapping to
> encrypted.

Yes, looks like a bss clearing issue. If I add __section(".data") to 
free_mem_ptr and free_mem_end_ptr in arch/x86/boot/compressed/misc.c and 
to malloc_ptr and malloc_cnt in include/linux/decompress/mm.h, then I can 
boot an SEV guest.

However, an SEV-ES guest is triple faulting. This looks to be because 
we're still on the EFI CS of 0x38 after switching GDTs in 
arch/x86/kernel/head_64.S by calling startup_64_setup_env(). Before 
switching to the kernel CS, we take a #VC (from CPUID calls in sme_enable) 
and things blow up on the iretq. Moving the block headed by the comment 
"Now switch to __KERNEL_CS so IRET works reliably" to just after calling 
startup_64_setup_env() fixes it and an SEV-ES guest can boot.

This worked before because I believe we switched off the EFI CS as part of 
the kernel decompressor support and so this bug wasn't exposed. But this 
needs to be fixed regardless of this series.

Thanks,
Tom

> 
> Thanks,
> Tom
> 
>
  

On Wed, 3 May 2023 at 19:58, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 5/2/23 11:08, Tom Lendacky wrote:
> > On 5/2/23 08:39, Ard Biesheuvel wrote:
> >> On Tue, 2 May 2023 at 15:37, Tom Lendacky <thomas.lendacky@amd.com> wrote:
> >>>
> >>> On 4/24/23 11:57, Ard Biesheuvel wrote:
> >>>> This series is conceptually a combination of Evgeny's series [0] and
> >>>> mine [1], both of which attempt to make the early decompressor code more
> >>>> amenable to executing in the EFI environment with stricter handling of
> >>>> memory permissions.
> >>>>
> >>>> My series [1] implemented zboot for x86, by getting rid of the entire
> >>>> x86 decompressor, and replacing it with existing EFI code that does the
> >>>> same but in a generic way. The downside of this is that only EFI boot is
> >>>> supported, making it unviable for distros, which need to support BIOS
> >>>> boot and hybrid EFI boot modes that omit the EFI stub.
> >>>>
> >>>> Evgeny's series [0] adapted the entire decompressor code flow to allow
> >>>> it to execute in the EFI context as well as the bare metal context, and
> >>>> this involves changes to the 1:1 mapping code and the page fault
> >>>> handlers etc, none of which are really needed when doing EFI boot in the
> >>>> first place.
> >>>>
> >>>> So this series attempts to occupy the middle ground here: it makes
> >>>> minimal changes to the existing decompressor so some of it can be called
> >>>> from the EFI stub. Then, it reimplements the EFI boot flow to decompress
> >>>> the kernel and boot it directly, without relying on the trampoline code,
> >>>> page table code or page fault handling code. This allows us to get rid
> >>>> of quite a bit of unsavory EFI stub code, and replace it with two clear
> >>>> invocations of the EFI firmware APIs to clear NX restrictions from
> >>>> allocations that have been populated with executable code.
> >>>>
> >>>> The only code that is being reused is the decompression library itself,
> >>>> along with the minimal ELF parsing that is required to copy the ELF
> >>>> segments in place, and the relocation processing that fixes up absolute
> >>>> symbol references to refer to the correct virtual addresses.
> >>>>
> >>>> Note that some of Evgeny's changes to clean up the PE/COFF header
> >>>> generation will still be needed, but I've omitted those here for
> >>>> brevity.
> >>>
> >>> I tried booting an SEV and an SEV-ES guest using this and both failed
> >>> to boot:
> >>>
> >>> EFI stub: WARNING: Decompression failed: Out of memory while allocating
> >>> z_stream
> >>>
> >>> I'll have to take a closer look as to why, but it might be a couple of
> >>> days before I can get to it.
> >>>
> >>
> >> Thanks Tom.
> >>
> >> The internal malloc() seems to be failing, which is often caused by
> >> BSS clearing problems. Could you elaborate a little bit on the boot
> >> environment you are using here?
> >
> > I'm using Qemu v7.2.1 as my VMM, Linux 6.3 with your series applied for my
> > host/hypervisor and guest kernel and the current OVMF tree built using
> > OvmfPkgX64.dsc.
> >
> > I was originally using the current merge window Linux, but moved to the
> > release version just to . With the release version SEV and SEV-ES still
> > fail to
> > boot, but SEV actually #GPs now. And some of the register contents look
> > like encrypted data:
> >
> > ConvertPages: range 1000000 - 4FA1FFF covers multiple entries
> > !!!! X64 Exception Type - 0D(#GP - General Protection)  CPU Apic ID -
> > 00000000 !!!!
> > ExceptionData - 0000000000000000
> > RIP  - 00000000597E71C1, CS  - 0000000000000038, RFLAGS - 0000000000210206
> > RAX  - 1FBA02A45943B920, RCX - 0000000000AF7009, RDX - A9DAE761B64A1F1B
> > RBX  - 1FBA02A45943B8C0, RSP - 000000007FD97320, RBP - 0000000002000000
> > RSI  - 0000000001000000, RDI - 1FBA02A45943DE68
> > R8   - 0000000003EF3C94, R9  - 0000000000000000, R10 - 000000007D7C6018
> > R11  - 0000000000000000, R12 - 0000000001000000, R13 - 00000000597EDD98
> > R14  - 0000000001000000, R15 - 000000007E0A5198
> > DS   - 0000000000000030, ES  - 0000000000000030, FS  - 0000000000000030
> > GS   - 0000000000000030, SS  - 0000000000000030
> > CR0  - 0000000080010033, CR2 - 0000000000000000, CR3 - 000000007FA01000
> > CR4  - 0000000000000668, CR8 - 0000000000000000
> > DR0  - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000
> > DR3  - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400
> > GDTR - 000000007F7DC000 0000000000000047, LDTR - 0000000000000000
> > IDTR - 000000007F34C018 0000000000000FFF,   TR - 0000000000000000
> > FXSAVE_STATE - 000000007FD96F80
> > !!!! Find image based on IP(0x597E71C1)
> > /root/kernels/ovmf-build-X64/Build/OvmfX64/DEBUG_GCC5/X64/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe/DEBUG/Variable
> > RuntimeDxe.dll (ImageBase=0000000000D4792C, EntryPoint=0000000000D50CC3) !!!!
> >
> > So, yes, probably an area of memory that was zeroes when mapped
> > unencrypted, but wasn't cleared after changing the mapping to
> > encrypted.
>
> Yes, looks like a bss clearing issue. If I add __section(".data") to
> free_mem_ptr and free_mem_end_ptr in arch/x86/boot/compressed/misc.c and
> to malloc_ptr and malloc_cnt in include/linux/decompress/mm.h, then I can
> boot an SEV guest.
>
> However, an SEV-ES guest is triple faulting. This looks to be because
> we're still on the EFI CS of 0x38 after switching GDTs in
> arch/x86/kernel/head_64.S by calling startup_64_setup_env(). Before
> switching to the kernel CS, we take a #VC (from CPUID calls in sme_enable)
> and things blow up on the iretq. Moving the block headed by the comment
> "Now switch to __KERNEL_CS so IRET works reliably" to just after calling
> startup_64_setup_env() fixes it and an SEV-ES guest can boot.
>
> This worked before because I believe we switched off the EFI CS as part of
> the kernel decompressor support and so this bug wasn't exposed. But this
> needs to be fixed regardless of this series.
>

Very interesting. I was under the assumption that everything that goes
on in sev_enable() in the decompressor would be rather indispensable
to boot in SEV mode (which I only spotted today) so I am quite
surprised that things just appear to work. (There is some 32-bit SEV
code in the decompressor as well that obviously never gets called when
booting in long mode, but I hadn't quite grasped how much other SEV
code there actually is)
  

On Wed, May 03, 2023 at 08:17:28PM +0200, Ard Biesheuvel wrote:
> (There is some 32-bit SEV code in the decompressor as well that
> obviously never gets called when booting in long mode, but I hadn't
> quite grasped how much other SEV code there actually is)

If you mean the thing in startup_32 which does get_sev_encryption_bit()
etc, that is going away.
  

On Wed, 3 May 2023 at 20:24, Borislav Petkov <bp@alien8.de> wrote:
>
> On Wed, May 03, 2023 at 08:17:28PM +0200, Ard Biesheuvel wrote:
> > (There is some 32-bit SEV code in the decompressor as well that
> > obviously never gets called when booting in long mode, but I hadn't
> > quite grasped how much other SEV code there actually is)
>
> If you mean the thing in startup_32 which does get_sev_encryption_bit()
> etc, that is going away.
>

OK, good to know.
  

On 5/3/23 13:17, Ard Biesheuvel wrote:
> On Wed, 3 May 2023 at 19:58, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>
>> On 5/2/23 11:08, Tom Lendacky wrote:
>>> On 5/2/23 08:39, Ard Biesheuvel wrote:
>>>> On Tue, 2 May 2023 at 15:37, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>>>>
>>>>> On 4/24/23 11:57, Ard Biesheuvel wrote:
>>>>>> This series is conceptually a combination of Evgeny's series [0] and
>>>>>> mine [1], both of which attempt to make the early decompressor code more
>>>>>> amenable to executing in the EFI environment with stricter handling of
>>>>>> memory permissions.
>>>>>>
>>>>>> My series [1] implemented zboot for x86, by getting rid of the entire
>>>>>> x86 decompressor, and replacing it with existing EFI code that does the
>>>>>> same but in a generic way. The downside of this is that only EFI boot is
>>>>>> supported, making it unviable for distros, which need to support BIOS
>>>>>> boot and hybrid EFI boot modes that omit the EFI stub.
>>>>>>
>>>>>> Evgeny's series [0] adapted the entire decompressor code flow to allow
>>>>>> it to execute in the EFI context as well as the bare metal context, and
>>>>>> this involves changes to the 1:1 mapping code and the page fault
>>>>>> handlers etc, none of which are really needed when doing EFI boot in the
>>>>>> first place.
>>>>>>
>>>>>> So this series attempts to occupy the middle ground here: it makes
>>>>>> minimal changes to the existing decompressor so some of it can be called
>>>>>> from the EFI stub. Then, it reimplements the EFI boot flow to decompress
>>>>>> the kernel and boot it directly, without relying on the trampoline code,
>>>>>> page table code or page fault handling code. This allows us to get rid
>>>>>> of quite a bit of unsavory EFI stub code, and replace it with two clear
>>>>>> invocations of the EFI firmware APIs to clear NX restrictions from
>>>>>> allocations that have been populated with executable code.
>>>>>>
>>>>>> The only code that is being reused is the decompression library itself,
>>>>>> along with the minimal ELF parsing that is required to copy the ELF
>>>>>> segments in place, and the relocation processing that fixes up absolute
>>>>>> symbol references to refer to the correct virtual addresses.
>>>>>>
>>>>>> Note that some of Evgeny's changes to clean up the PE/COFF header
>>>>>> generation will still be needed, but I've omitted those here for
>>>>>> brevity.
>>>>>
>>>>> I tried booting an SEV and an SEV-ES guest using this and both failed
>>>>> to boot:
>>>>>
>>>>> EFI stub: WARNING: Decompression failed: Out of memory while allocating
>>>>> z_stream
>>>>>
>>>>> I'll have to take a closer look as to why, but it might be a couple of
>>>>> days before I can get to it.
>>>>>
>>>>
>>>> Thanks Tom.
>>>>
>>>> The internal malloc() seems to be failing, which is often caused by
>>>> BSS clearing problems. Could you elaborate a little bit on the boot
>>>> environment you are using here?
>>>
>>> I'm using Qemu v7.2.1 as my VMM, Linux 6.3 with your series applied for my
>>> host/hypervisor and guest kernel and the current OVMF tree built using
>>> OvmfPkgX64.dsc.
>>>
>>> I was originally using the current merge window Linux, but moved to the
>>> release version just to . With the release version SEV and SEV-ES still
>>> fail to
>>> boot, but SEV actually #GPs now. And some of the register contents look
>>> like encrypted data:
>>>
>>> ConvertPages: range 1000000 - 4FA1FFF covers multiple entries
>>> !!!! X64 Exception Type - 0D(#GP - General Protection)  CPU Apic ID -
>>> 00000000 !!!!
>>> ExceptionData - 0000000000000000
>>> RIP  - 00000000597E71C1, CS  - 0000000000000038, RFLAGS - 0000000000210206
>>> RAX  - 1FBA02A45943B920, RCX - 0000000000AF7009, RDX - A9DAE761B64A1F1B
>>> RBX  - 1FBA02A45943B8C0, RSP - 000000007FD97320, RBP - 0000000002000000
>>> RSI  - 0000000001000000, RDI - 1FBA02A45943DE68
>>> R8   - 0000000003EF3C94, R9  - 0000000000000000, R10 - 000000007D7C6018
>>> R11  - 0000000000000000, R12 - 0000000001000000, R13 - 00000000597EDD98
>>> R14  - 0000000001000000, R15 - 000000007E0A5198
>>> DS   - 0000000000000030, ES  - 0000000000000030, FS  - 0000000000000030
>>> GS   - 0000000000000030, SS  - 0000000000000030
>>> CR0  - 0000000080010033, CR2 - 0000000000000000, CR3 - 000000007FA01000
>>> CR4  - 0000000000000668, CR8 - 0000000000000000
>>> DR0  - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000
>>> DR3  - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400
>>> GDTR - 000000007F7DC000 0000000000000047, LDTR - 0000000000000000
>>> IDTR - 000000007F34C018 0000000000000FFF,   TR - 0000000000000000
>>> FXSAVE_STATE - 000000007FD96F80
>>> !!!! Find image based on IP(0x597E71C1)
>>> /root/kernels/ovmf-build-X64/Build/OvmfX64/DEBUG_GCC5/X64/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe/DEBUG/Variable
>>> RuntimeDxe.dll (ImageBase=0000000000D4792C, EntryPoint=0000000000D50CC3) !!!!
>>>
>>> So, yes, probably an area of memory that was zeroes when mapped
>>> unencrypted, but wasn't cleared after changing the mapping to
>>> encrypted.
>>
>> Yes, looks like a bss clearing issue. If I add __section(".data") to
>> free_mem_ptr and free_mem_end_ptr in arch/x86/boot/compressed/misc.c and
>> to malloc_ptr and malloc_cnt in include/linux/decompress/mm.h, then I can
>> boot an SEV guest.
>>
>> However, an SEV-ES guest is triple faulting. This looks to be because
>> we're still on the EFI CS of 0x38 after switching GDTs in
>> arch/x86/kernel/head_64.S by calling startup_64_setup_env(). Before
>> switching to the kernel CS, we take a #VC (from CPUID calls in sme_enable)
>> and things blow up on the iretq. Moving the block headed by the comment
>> "Now switch to __KERNEL_CS so IRET works reliably" to just after calling
>> startup_64_setup_env() fixes it and an SEV-ES guest can boot.
>>
>> This worked before because I believe we switched off the EFI CS as part of
>> the kernel decompressor support and so this bug wasn't exposed. But this
>> needs to be fixed regardless of this series.
>>
> 
> Very interesting. I was under the assumption that everything that goes
> on in sev_enable() in the decompressor would be rather indispensable
> to boot in SEV mode (which I only spotted today) so I am quite
> surprised that things just appear to work. (There is some 32-bit SEV
> code in the decompressor as well that obviously never gets called when
> booting in long mode, but I hadn't quite grasped how much other SEV
> code there actually is)

The sev_enable() function for SEV and SEV-ES is more for ensuring a proper 
environment (ensuring the proper CPUID bits are present for the guest, 
checking the GHCB protocol level, properly preparing pagetables, etc.). 
Since we're still in EFI and using the EFI #VC handler and pagetables, we 
don't require some of that stuff, but some of the it would need to be 
performed at some point (I wasn't trying to be a malicious hypervisor).

I haven't tested SEV-SNP yet because the hypervisor support is not 
upstream, yet, and I haven't applied your series to an SNP hypervisor 
tree, yet. There is a lot of support in sev_enable() for SNP that may 
likely cause a problem compared to SEV and SEV-ES.

SEV-SNP has a lot more checking and validation going on and it ensures it 
gets the confidential computing blob from EFI into the boot parameters. 
I'm not sure when I'll be able to test SNP, since I'm off next week and 
trying to wrap up a bunch of stuff before I leave.

Thanks,
Tom
  

On 5/3/23 12:44, Ard Biesheuvel wrote:
> On Tue, 2 May 2023 at 18:08, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>
>> On 5/2/23 08:39, Ard Biesheuvel wrote:
>>> On Tue, 2 May 2023 at 15:37, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>>>
>>>> On 4/24/23 11:57, Ard Biesheuvel wrote:
>>>>> This series is conceptually a combination of Evgeny's series [0] and
>>>>> mine [1], both of which attempt to make the early decompressor code more
>>>>> amenable to executing in the EFI environment with stricter handling of
>>>>> memory permissions.
>>>>>
>>>>> My series [1] implemented zboot for x86, by getting rid of the entire
>>>>> x86 decompressor, and replacing it with existing EFI code that does the
>>>>> same but in a generic way. The downside of this is that only EFI boot is
>>>>> supported, making it unviable for distros, which need to support BIOS
>>>>> boot and hybrid EFI boot modes that omit the EFI stub.
>>>>>
>>>>> Evgeny's series [0] adapted the entire decompressor code flow to allow
>>>>> it to execute in the EFI context as well as the bare metal context, and
>>>>> this involves changes to the 1:1 mapping code and the page fault
>>>>> handlers etc, none of which are really needed when doing EFI boot in the
>>>>> first place.
>>>>>
>>>>> So this series attempts to occupy the middle ground here: it makes
>>>>> minimal changes to the existing decompressor so some of it can be called
>>>>> from the EFI stub. Then, it reimplements the EFI boot flow to decompress
>>>>> the kernel and boot it directly, without relying on the trampoline code,
>>>>> page table code or page fault handling code. This allows us to get rid
>>>>> of quite a bit of unsavory EFI stub code, and replace it with two clear
>>>>> invocations of the EFI firmware APIs to clear NX restrictions from
>>>>> allocations that have been populated with executable code.
>>>>>
>>>>> The only code that is being reused is the decompression library itself,
>>>>> along with the minimal ELF parsing that is required to copy the ELF
>>>>> segments in place, and the relocation processing that fixes up absolute
>>>>> symbol references to refer to the correct virtual addresses.
>>>>>
>>>>> Note that some of Evgeny's changes to clean up the PE/COFF header
>>>>> generation will still be needed, but I've omitted those here for
>>>>> brevity.
>>>>
>>>> I tried booting an SEV and an SEV-ES guest using this and both failed to boot:
>>>>
>>>> EFI stub: WARNING: Decompression failed: Out of memory while allocating
>>>> z_stream
>>>>
>>>> I'll have to take a closer look as to why, but it might be a couple of
>>>> days before I can get to it.
>>>>
>>>
>>> Thanks Tom.
>>>
>>> The internal malloc() seems to be failing, which is often caused by
>>> BSS clearing problems. Could you elaborate a little bit on the boot
>>> environment you are using here?
>>
>> I'm using Qemu v7.2.1 as my VMM, Linux 6.3 with your series applied for my
>> host/hypervisor and guest kernel and the current OVMF tree built using
>> OvmfPkgX64.dsc.
>>
>> I was originally using the current merge window Linux, but moved to the
>> release version just to . With the release version SEV and SEV-ES still fail to
>> boot, but SEV actually #GPs now. And some of the register contents look
>> like encrypted data:
>>
>> ConvertPages: range 1000000 - 4FA1FFF covers multiple entries
>> !!!! X64 Exception Type - 0D(#GP - General Protection)  CPU Apic ID - 00000000 !!!!
>> ExceptionData - 0000000000000000
>> RIP  - 00000000597E71C1, CS  - 0000000000000038, RFLAGS - 0000000000210206
>> RAX  - 1FBA02A45943B920, RCX - 0000000000AF7009, RDX - A9DAE761B64A1F1B
>> RBX  - 1FBA02A45943B8C0, RSP - 000000007FD97320, RBP - 0000000002000000
>> RSI  - 0000000001000000, RDI - 1FBA02A45943DE68
>> R8   - 0000000003EF3C94, R9  - 0000000000000000, R10 - 000000007D7C6018
>> R11  - 0000000000000000, R12 - 0000000001000000, R13 - 00000000597EDD98
>> R14  - 0000000001000000, R15 - 000000007E0A5198
>> DS   - 0000000000000030, ES  - 0000000000000030, FS  - 0000000000000030
>> GS   - 0000000000000030, SS  - 0000000000000030
>> CR0  - 0000000080010033, CR2 - 0000000000000000, CR3 - 000000007FA01000
>> CR4  - 0000000000000668, CR8 - 0000000000000000
>> DR0  - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000
>> DR3  - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400
>> GDTR - 000000007F7DC000 0000000000000047, LDTR - 0000000000000000
>> IDTR - 000000007F34C018 0000000000000FFF,   TR - 0000000000000000
>> FXSAVE_STATE - 000000007FD96F80
>> !!!! Find image based on IP(0x597E71C1) /root/kernels/ovmf-build-X64/Build/OvmfX64/DEBUG_GCC5/X64/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe/DEBUG/Variable
>> RuntimeDxe.dll (ImageBase=0000000000D4792C, EntryPoint=0000000000D50CC3) !!!!
>>
>> So, yes, probably an area of memory that was zeroes when mapped
>> unencrypted, but wasn't cleared after changing the mapping to
>> encrypted.
>>
> 
> Thanks.
> 
> It seems I was a bit naive and underestimated the amount of SEV
> related processing that goes on in the decompressor after the EFI stub
> has handed over. I will have to take some time and go through this,
> and decide whether there is a way we can share this code with the EFI
> stub without introducing yet another permutation that requires testing
> and maintenance.
> 
> Any suggestions on how to test this stuff is appreciated - does QEMU
> emulate any of this? Does consumer-level AMD hardware implement the
> pieces I'd need to run a SEV host with SNP support etc?

Unfortunately Qemu doesn't emulate any of this and SEV support, 
specifically the SEV firmware, isn't available on consumer-level parts, so 
you would need an EPYC part to do SEV.

Thanks,
Tom
  

On Wed, 3 May 2023 at 20:49, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 5/3/23 13:17, Ard Biesheuvel wrote:
> > On Wed, 3 May 2023 at 19:58, Tom Lendacky <thomas.lendacky@amd.com> wrote:
> >>
> >> On 5/2/23 11:08, Tom Lendacky wrote:
> >>> On 5/2/23 08:39, Ard Biesheuvel wrote:
> >>>> On Tue, 2 May 2023 at 15:37, Tom Lendacky <thomas.lendacky@amd.com> wrote:
> >>>>>
> >>>>> On 4/24/23 11:57, Ard Biesheuvel wrote:
> >>>>>> This series is conceptually a combination of Evgeny's series [0] and
> >>>>>> mine [1], both of which attempt to make the early decompressor code more
> >>>>>> amenable to executing in the EFI environment with stricter handling of
> >>>>>> memory permissions.
> >>>>>>
> >>>>>> My series [1] implemented zboot for x86, by getting rid of the entire
> >>>>>> x86 decompressor, and replacing it with existing EFI code that does the
> >>>>>> same but in a generic way. The downside of this is that only EFI boot is
> >>>>>> supported, making it unviable for distros, which need to support BIOS
> >>>>>> boot and hybrid EFI boot modes that omit the EFI stub.
> >>>>>>
> >>>>>> Evgeny's series [0] adapted the entire decompressor code flow to allow
> >>>>>> it to execute in the EFI context as well as the bare metal context, and
> >>>>>> this involves changes to the 1:1 mapping code and the page fault
> >>>>>> handlers etc, none of which are really needed when doing EFI boot in the
> >>>>>> first place.
> >>>>>>
> >>>>>> So this series attempts to occupy the middle ground here: it makes
> >>>>>> minimal changes to the existing decompressor so some of it can be called
> >>>>>> from the EFI stub. Then, it reimplements the EFI boot flow to decompress
> >>>>>> the kernel and boot it directly, without relying on the trampoline code,
> >>>>>> page table code or page fault handling code. This allows us to get rid
> >>>>>> of quite a bit of unsavory EFI stub code, and replace it with two clear
> >>>>>> invocations of the EFI firmware APIs to clear NX restrictions from
> >>>>>> allocations that have been populated with executable code.
> >>>>>>
> >>>>>> The only code that is being reused is the decompression library itself,
> >>>>>> along with the minimal ELF parsing that is required to copy the ELF
> >>>>>> segments in place, and the relocation processing that fixes up absolute
> >>>>>> symbol references to refer to the correct virtual addresses.
> >>>>>>
> >>>>>> Note that some of Evgeny's changes to clean up the PE/COFF header
> >>>>>> generation will still be needed, but I've omitted those here for
> >>>>>> brevity.
> >>>>>
> >>>>> I tried booting an SEV and an SEV-ES guest using this and both failed
> >>>>> to boot:
> >>>>>
> >>>>> EFI stub: WARNING: Decompression failed: Out of memory while allocating
> >>>>> z_stream
> >>>>>
> >>>>> I'll have to take a closer look as to why, but it might be a couple of
> >>>>> days before I can get to it.
> >>>>>
> >>>>
> >>>> Thanks Tom.
> >>>>
> >>>> The internal malloc() seems to be failing, which is often caused by
> >>>> BSS clearing problems. Could you elaborate a little bit on the boot
> >>>> environment you are using here?
> >>>
> >>> I'm using Qemu v7.2.1 as my VMM, Linux 6.3 with your series applied for my
> >>> host/hypervisor and guest kernel and the current OVMF tree built using
> >>> OvmfPkgX64.dsc.
> >>>
> >>> I was originally using the current merge window Linux, but moved to the
> >>> release version just to . With the release version SEV and SEV-ES still
> >>> fail to
> >>> boot, but SEV actually #GPs now. And some of the register contents look
> >>> like encrypted data:
> >>>
> >>> ConvertPages: range 1000000 - 4FA1FFF covers multiple entries
> >>> !!!! X64 Exception Type - 0D(#GP - General Protection)  CPU Apic ID -
> >>> 00000000 !!!!
> >>> ExceptionData - 0000000000000000
> >>> RIP  - 00000000597E71C1, CS  - 0000000000000038, RFLAGS - 0000000000210206
> >>> RAX  - 1FBA02A45943B920, RCX - 0000000000AF7009, RDX - A9DAE761B64A1F1B
> >>> RBX  - 1FBA02A45943B8C0, RSP - 000000007FD97320, RBP - 0000000002000000
> >>> RSI  - 0000000001000000, RDI - 1FBA02A45943DE68
> >>> R8   - 0000000003EF3C94, R9  - 0000000000000000, R10 - 000000007D7C6018
> >>> R11  - 0000000000000000, R12 - 0000000001000000, R13 - 00000000597EDD98
> >>> R14  - 0000000001000000, R15 - 000000007E0A5198
> >>> DS   - 0000000000000030, ES  - 0000000000000030, FS  - 0000000000000030
> >>> GS   - 0000000000000030, SS  - 0000000000000030
> >>> CR0  - 0000000080010033, CR2 - 0000000000000000, CR3 - 000000007FA01000
> >>> CR4  - 0000000000000668, CR8 - 0000000000000000
> >>> DR0  - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000
> >>> DR3  - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400
> >>> GDTR - 000000007F7DC000 0000000000000047, LDTR - 0000000000000000
> >>> IDTR - 000000007F34C018 0000000000000FFF,   TR - 0000000000000000
> >>> FXSAVE_STATE - 000000007FD96F80
> >>> !!!! Find image based on IP(0x597E71C1)
> >>> /root/kernels/ovmf-build-X64/Build/OvmfX64/DEBUG_GCC5/X64/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe/DEBUG/Variable
> >>> RuntimeDxe.dll (ImageBase=0000000000D4792C, EntryPoint=0000000000D50CC3) !!!!
> >>>
> >>> So, yes, probably an area of memory that was zeroes when mapped
> >>> unencrypted, but wasn't cleared after changing the mapping to
> >>> encrypted.
> >>
> >> Yes, looks like a bss clearing issue. If I add __section(".data") to
> >> free_mem_ptr and free_mem_end_ptr in arch/x86/boot/compressed/misc.c and
> >> to malloc_ptr and malloc_cnt in include/linux/decompress/mm.h, then I can
> >> boot an SEV guest.
> >>
> >> However, an SEV-ES guest is triple faulting. This looks to be because
> >> we're still on the EFI CS of 0x38 after switching GDTs in
> >> arch/x86/kernel/head_64.S by calling startup_64_setup_env(). Before
> >> switching to the kernel CS, we take a #VC (from CPUID calls in sme_enable)
> >> and things blow up on the iretq. Moving the block headed by the comment
> >> "Now switch to __KERNEL_CS so IRET works reliably" to just after calling
> >> startup_64_setup_env() fixes it and an SEV-ES guest can boot.
> >>
> >> This worked before because I believe we switched off the EFI CS as part of
> >> the kernel decompressor support and so this bug wasn't exposed. But this
> >> needs to be fixed regardless of this series.
> >>
> >
> > Very interesting. I was under the assumption that everything that goes
> > on in sev_enable() in the decompressor would be rather indispensable
> > to boot in SEV mode (which I only spotted today) so I am quite
> > surprised that things just appear to work. (There is some 32-bit SEV
> > code in the decompressor as well that obviously never gets called when
> > booting in long mode, but I hadn't quite grasped how much other SEV
> > code there actually is)
>
> The sev_enable() function for SEV and SEV-ES is more for ensuring a proper
> environment (ensuring the proper CPUID bits are present for the guest,
> checking the GHCB protocol level, properly preparing pagetables, etc.).
> Since we're still in EFI and using the EFI #VC handler and pagetables, we
> don't require some of that stuff, but some of the it would need to be
> performed at some point (I wasn't trying to be a malicious hypervisor).
>

OK, good to know.

> I haven't tested SEV-SNP yet because the hypervisor support is not
> upstream, yet, and I haven't applied your series to an SNP hypervisor
> tree, yet. There is a lot of support in sev_enable() for SNP that may
> likely cause a problem compared to SEV and SEV-ES.
>
> SEV-SNP has a lot more checking and validation going on and it ensures it
> gets the confidential computing blob from EFI into the boot parameters.
> I'm not sure when I'll be able to test SNP, since I'm off next week and
> trying to wrap up a bunch of stuff before I leave.
>

Would it make sense, given the apparent direct dependency on EFI, to
move that stuff into the EFI stub instead of doing it from the
decompressor?

Thanks for giving this a spin - I can wait for more testing until you
get back, and hopefully, we'll have converged a bit more by that time
and there will be a new revision of the series available for testing.

BTW any clue whether your boot path is relying on the EFI handover
protocol? I.e., QemuStartLegacyImage() in OVMF? Evgeny mentioned that
BSS needs clearing in that case, and I was wondering if that is the
case you are hitting.
(If you have a DEBUG OVMF boot log you could share, I can go through it myself)
  

On 5/3/23 13:59, Ard Biesheuvel wrote:
> On Wed, 3 May 2023 at 20:49, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>
>> On 5/3/23 13:17, Ard Biesheuvel wrote:
>>> On Wed, 3 May 2023 at 19:58, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>>>
>>>> On 5/2/23 11:08, Tom Lendacky wrote:
>>>>> On 5/2/23 08:39, Ard Biesheuvel wrote:
>>>>>> On Tue, 2 May 2023 at 15:37, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>>>>>>
>>>>>>> On 4/24/23 11:57, Ard Biesheuvel wrote:
>>>>>>>> This series is conceptually a combination of Evgeny's series [0] and
>>>>>>>> mine [1], both of which attempt to make the early decompressor code more
>>>>>>>> amenable to executing in the EFI environment with stricter handling of
>>>>>>>> memory permissions.
>>>>>>>>
>>>>>>>> My series [1] implemented zboot for x86, by getting rid of the entire
>>>>>>>> x86 decompressor, and replacing it with existing EFI code that does the
>>>>>>>> same but in a generic way. The downside of this is that only EFI boot is
>>>>>>>> supported, making it unviable for distros, which need to support BIOS
>>>>>>>> boot and hybrid EFI boot modes that omit the EFI stub.
>>>>>>>>
>>>>>>>> Evgeny's series [0] adapted the entire decompressor code flow to allow
>>>>>>>> it to execute in the EFI context as well as the bare metal context, and
>>>>>>>> this involves changes to the 1:1 mapping code and the page fault
>>>>>>>> handlers etc, none of which are really needed when doing EFI boot in the
>>>>>>>> first place.
>>>>>>>>
>>>>>>>> So this series attempts to occupy the middle ground here: it makes
>>>>>>>> minimal changes to the existing decompressor so some of it can be called
>>>>>>>> from the EFI stub. Then, it reimplements the EFI boot flow to decompress
>>>>>>>> the kernel and boot it directly, without relying on the trampoline code,
>>>>>>>> page table code or page fault handling code. This allows us to get rid
>>>>>>>> of quite a bit of unsavory EFI stub code, and replace it with two clear
>>>>>>>> invocations of the EFI firmware APIs to clear NX restrictions from
>>>>>>>> allocations that have been populated with executable code.
>>>>>>>>
>>>>>>>> The only code that is being reused is the decompression library itself,
>>>>>>>> along with the minimal ELF parsing that is required to copy the ELF
>>>>>>>> segments in place, and the relocation processing that fixes up absolute
>>>>>>>> symbol references to refer to the correct virtual addresses.
>>>>>>>>
>>>>>>>> Note that some of Evgeny's changes to clean up the PE/COFF header
>>>>>>>> generation will still be needed, but I've omitted those here for
>>>>>>>> brevity.
>>>>>>>
>>>>>>> I tried booting an SEV and an SEV-ES guest using this and both failed
>>>>>>> to boot:
>>>>>>>
>>>>>>> EFI stub: WARNING: Decompression failed: Out of memory while allocating
>>>>>>> z_stream
>>>>>>>
>>>>>>> I'll have to take a closer look as to why, but it might be a couple of
>>>>>>> days before I can get to it.
>>>>>>>
>>>>>>
>>>>>> Thanks Tom.
>>>>>>
>>>>>> The internal malloc() seems to be failing, which is often caused by
>>>>>> BSS clearing problems. Could you elaborate a little bit on the boot
>>>>>> environment you are using here?
>>>>>
>>>>> I'm using Qemu v7.2.1 as my VMM, Linux 6.3 with your series applied for my
>>>>> host/hypervisor and guest kernel and the current OVMF tree built using
>>>>> OvmfPkgX64.dsc.
>>>>>
>>>>> I was originally using the current merge window Linux, but moved to the
>>>>> release version just to . With the release version SEV and SEV-ES still
>>>>> fail to
>>>>> boot, but SEV actually #GPs now. And some of the register contents look
>>>>> like encrypted data:
>>>>>
>>>>> ConvertPages: range 1000000 - 4FA1FFF covers multiple entries
>>>>> !!!! X64 Exception Type - 0D(#GP - General Protection)  CPU Apic ID -
>>>>> 00000000 !!!!
>>>>> ExceptionData - 0000000000000000
>>>>> RIP  - 00000000597E71C1, CS  - 0000000000000038, RFLAGS - 0000000000210206
>>>>> RAX  - 1FBA02A45943B920, RCX - 0000000000AF7009, RDX - A9DAE761B64A1F1B
>>>>> RBX  - 1FBA02A45943B8C0, RSP - 000000007FD97320, RBP - 0000000002000000
>>>>> RSI  - 0000000001000000, RDI - 1FBA02A45943DE68
>>>>> R8   - 0000000003EF3C94, R9  - 0000000000000000, R10 - 000000007D7C6018
>>>>> R11  - 0000000000000000, R12 - 0000000001000000, R13 - 00000000597EDD98
>>>>> R14  - 0000000001000000, R15 - 000000007E0A5198
>>>>> DS   - 0000000000000030, ES  - 0000000000000030, FS  - 0000000000000030
>>>>> GS   - 0000000000000030, SS  - 0000000000000030
>>>>> CR0  - 0000000080010033, CR2 - 0000000000000000, CR3 - 000000007FA01000
>>>>> CR4  - 0000000000000668, CR8 - 0000000000000000
>>>>> DR0  - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000
>>>>> DR3  - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400
>>>>> GDTR - 000000007F7DC000 0000000000000047, LDTR - 0000000000000000
>>>>> IDTR - 000000007F34C018 0000000000000FFF,   TR - 0000000000000000
>>>>> FXSAVE_STATE - 000000007FD96F80
>>>>> !!!! Find image based on IP(0x597E71C1)
>>>>> /root/kernels/ovmf-build-X64/Build/OvmfX64/DEBUG_GCC5/X64/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe/DEBUG/Variable
>>>>> RuntimeDxe.dll (ImageBase=0000000000D4792C, EntryPoint=0000000000D50CC3) !!!!
>>>>>
>>>>> So, yes, probably an area of memory that was zeroes when mapped
>>>>> unencrypted, but wasn't cleared after changing the mapping to
>>>>> encrypted.
>>>>
>>>> Yes, looks like a bss clearing issue. If I add __section(".data") to
>>>> free_mem_ptr and free_mem_end_ptr in arch/x86/boot/compressed/misc.c and
>>>> to malloc_ptr and malloc_cnt in include/linux/decompress/mm.h, then I can
>>>> boot an SEV guest.
>>>>
>>>> However, an SEV-ES guest is triple faulting. This looks to be because
>>>> we're still on the EFI CS of 0x38 after switching GDTs in
>>>> arch/x86/kernel/head_64.S by calling startup_64_setup_env(). Before
>>>> switching to the kernel CS, we take a #VC (from CPUID calls in sme_enable)
>>>> and things blow up on the iretq. Moving the block headed by the comment
>>>> "Now switch to __KERNEL_CS so IRET works reliably" to just after calling
>>>> startup_64_setup_env() fixes it and an SEV-ES guest can boot.
>>>>
>>>> This worked before because I believe we switched off the EFI CS as part of
>>>> the kernel decompressor support and so this bug wasn't exposed. But this
>>>> needs to be fixed regardless of this series.
>>>>
>>>
>>> Very interesting. I was under the assumption that everything that goes
>>> on in sev_enable() in the decompressor would be rather indispensable
>>> to boot in SEV mode (which I only spotted today) so I am quite
>>> surprised that things just appear to work. (There is some 32-bit SEV
>>> code in the decompressor as well that obviously never gets called when
>>> booting in long mode, but I hadn't quite grasped how much other SEV
>>> code there actually is)
>>
>> The sev_enable() function for SEV and SEV-ES is more for ensuring a proper
>> environment (ensuring the proper CPUID bits are present for the guest,
>> checking the GHCB protocol level, properly preparing pagetables, etc.).
>> Since we're still in EFI and using the EFI #VC handler and pagetables, we
>> don't require some of that stuff, but some of the it would need to be
>> performed at some point (I wasn't trying to be a malicious hypervisor).
>>
> 
> OK, good to know.
> 
>> I haven't tested SEV-SNP yet because the hypervisor support is not
>> upstream, yet, and I haven't applied your series to an SNP hypervisor
>> tree, yet. There is a lot of support in sev_enable() for SNP that may
>> likely cause a problem compared to SEV and SEV-ES.
>>
>> SEV-SNP has a lot more checking and validation going on and it ensures it
>> gets the confidential computing blob from EFI into the boot parameters.
>> I'm not sure when I'll be able to test SNP, since I'm off next week and
>> trying to wrap up a bunch of stuff before I leave.
>>
> 
> Would it make sense, given the apparent direct dependency on EFI, to
> move that stuff into the EFI stub instead of doing it from the
> decompressor?

We tried to make the support for SNP not be reliant on EFI. If EFI is 
present, then get the confidential computing blob from it. Otherwise, the 
cc blob can be supplied as part of the setup data in the Linux boot 
protocol (see find_cc_block() in arch/x86/boot/compressed/sev.c).

So I guess some of it could be moved, but I'm not sure how much.

> 
> Thanks for giving this a spin - I can wait for more testing until you
> get back, and hopefully, we'll have converged a bit more by that time
> and there will be a new revision of the series available for testing.
> 
> BTW any clue whether your boot path is relying on the EFI handover
> protocol? I.e., QemuStartLegacyImage() in OVMF? Evgeny mentioned that

I don't believe it is. It's using the Grub EFI bootloader to load and boot 
Linux.

> BSS needs clearing in that case, and I was wondering if that is the
> case you are hitting.
> (If you have a DEBUG OVMF boot log you could share, I can go through it myself)

Sure, I'll send you a copy in a separate email.

Thanks,
Tom
  

On Wed, 3 May 2023 at 23:23, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 5/3/23 13:59, Ard Biesheuvel wrote:
> > On Wed, 3 May 2023 at 20:49, Tom Lendacky <thomas.lendacky@amd.com> wrote:
> >>
> >> On 5/3/23 13:17, Ard Biesheuvel wrote:
> >>> On Wed, 3 May 2023 at 19:58, Tom Lendacky <thomas.lendacky@amd.com> wrote:
> >>>>
> >>>> On 5/2/23 11:08, Tom Lendacky wrote:
> >>>>> On 5/2/23 08:39, Ard Biesheuvel wrote:
> >>>>>> On Tue, 2 May 2023 at 15:37, Tom Lendacky <thomas.lendacky@amd.com> wrote:
> >>>>>>>
> >>>>>>> On 4/24/23 11:57, Ard Biesheuvel wrote:
> >>>>>>>> This series is conceptually a combination of Evgeny's series [0] and
> >>>>>>>> mine [1], both of which attempt to make the early decompressor code more
> >>>>>>>> amenable to executing in the EFI environment with stricter handling of
> >>>>>>>> memory permissions.
> >>>>>>>>
> >>>>>>>> My series [1] implemented zboot for x86, by getting rid of the entire
> >>>>>>>> x86 decompressor, and replacing it with existing EFI code that does the
> >>>>>>>> same but in a generic way. The downside of this is that only EFI boot is
> >>>>>>>> supported, making it unviable for distros, which need to support BIOS
> >>>>>>>> boot and hybrid EFI boot modes that omit the EFI stub.
> >>>>>>>>
> >>>>>>>> Evgeny's series [0] adapted the entire decompressor code flow to allow
> >>>>>>>> it to execute in the EFI context as well as the bare metal context, and
> >>>>>>>> this involves changes to the 1:1 mapping code and the page fault
> >>>>>>>> handlers etc, none of which are really needed when doing EFI boot in the
> >>>>>>>> first place.
> >>>>>>>>
> >>>>>>>> So this series attempts to occupy the middle ground here: it makes
> >>>>>>>> minimal changes to the existing decompressor so some of it can be called
> >>>>>>>> from the EFI stub. Then, it reimplements the EFI boot flow to decompress
> >>>>>>>> the kernel and boot it directly, without relying on the trampoline code,
> >>>>>>>> page table code or page fault handling code. This allows us to get rid
> >>>>>>>> of quite a bit of unsavory EFI stub code, and replace it with two clear
> >>>>>>>> invocations of the EFI firmware APIs to clear NX restrictions from
> >>>>>>>> allocations that have been populated with executable code.
> >>>>>>>>
> >>>>>>>> The only code that is being reused is the decompression library itself,
> >>>>>>>> along with the minimal ELF parsing that is required to copy the ELF
> >>>>>>>> segments in place, and the relocation processing that fixes up absolute
> >>>>>>>> symbol references to refer to the correct virtual addresses.
> >>>>>>>>
> >>>>>>>> Note that some of Evgeny's changes to clean up the PE/COFF header
> >>>>>>>> generation will still be needed, but I've omitted those here for
> >>>>>>>> brevity.
> >>>>>>>
> >>>>>>> I tried booting an SEV and an SEV-ES guest using this and both failed
> >>>>>>> to boot:
> >>>>>>>
> >>>>>>> EFI stub: WARNING: Decompression failed: Out of memory while allocating
> >>>>>>> z_stream
> >>>>>>>
> >>>>>>> I'll have to take a closer look as to why, but it might be a couple of
> >>>>>>> days before I can get to it.
> >>>>>>>
> >>>>>>
> >>>>>> Thanks Tom.
> >>>>>>
> >>>>>> The internal malloc() seems to be failing, which is often caused by
> >>>>>> BSS clearing problems. Could you elaborate a little bit on the boot
> >>>>>> environment you are using here?
> >>>>>
> >>>>> I'm using Qemu v7.2.1 as my VMM, Linux 6.3 with your series applied for my
> >>>>> host/hypervisor and guest kernel and the current OVMF tree built using
> >>>>> OvmfPkgX64.dsc.
> >>>>>
> >>>>> I was originally using the current merge window Linux, but moved to the
> >>>>> release version just to . With the release version SEV and SEV-ES still
> >>>>> fail to
> >>>>> boot, but SEV actually #GPs now. And some of the register contents look
> >>>>> like encrypted data:
> >>>>>
> >>>>> ConvertPages: range 1000000 - 4FA1FFF covers multiple entries
> >>>>> !!!! X64 Exception Type - 0D(#GP - General Protection)  CPU Apic ID -
> >>>>> 00000000 !!!!
> >>>>> ExceptionData - 0000000000000000
> >>>>> RIP  - 00000000597E71C1, CS  - 0000000000000038, RFLAGS - 0000000000210206
> >>>>> RAX  - 1FBA02A45943B920, RCX - 0000000000AF7009, RDX - A9DAE761B64A1F1B
> >>>>> RBX  - 1FBA02A45943B8C0, RSP - 000000007FD97320, RBP - 0000000002000000
> >>>>> RSI  - 0000000001000000, RDI - 1FBA02A45943DE68
> >>>>> R8   - 0000000003EF3C94, R9  - 0000000000000000, R10 - 000000007D7C6018
> >>>>> R11  - 0000000000000000, R12 - 0000000001000000, R13 - 00000000597EDD98
> >>>>> R14  - 0000000001000000, R15 - 000000007E0A5198
> >>>>> DS   - 0000000000000030, ES  - 0000000000000030, FS  - 0000000000000030
> >>>>> GS   - 0000000000000030, SS  - 0000000000000030
> >>>>> CR0  - 0000000080010033, CR2 - 0000000000000000, CR3 - 000000007FA01000
> >>>>> CR4  - 0000000000000668, CR8 - 0000000000000000
> >>>>> DR0  - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000
> >>>>> DR3  - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400
> >>>>> GDTR - 000000007F7DC000 0000000000000047, LDTR - 0000000000000000
> >>>>> IDTR - 000000007F34C018 0000000000000FFF,   TR - 0000000000000000
> >>>>> FXSAVE_STATE - 000000007FD96F80
> >>>>> !!!! Find image based on IP(0x597E71C1)
> >>>>> /root/kernels/ovmf-build-X64/Build/OvmfX64/DEBUG_GCC5/X64/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe/DEBUG/Variable
> >>>>> RuntimeDxe.dll (ImageBase=0000000000D4792C, EntryPoint=0000000000D50CC3) !!!!
> >>>>>
> >>>>> So, yes, probably an area of memory that was zeroes when mapped
> >>>>> unencrypted, but wasn't cleared after changing the mapping to
> >>>>> encrypted.
> >>>>
> >>>> Yes, looks like a bss clearing issue. If I add __section(".data") to
> >>>> free_mem_ptr and free_mem_end_ptr in arch/x86/boot/compressed/misc.c and
> >>>> to malloc_ptr and malloc_cnt in include/linux/decompress/mm.h, then I can
> >>>> boot an SEV guest.
> >>>>
> >>>> However, an SEV-ES guest is triple faulting. This looks to be because
> >>>> we're still on the EFI CS of 0x38 after switching GDTs in
> >>>> arch/x86/kernel/head_64.S by calling startup_64_setup_env(). Before
> >>>> switching to the kernel CS, we take a #VC (from CPUID calls in sme_enable)
> >>>> and things blow up on the iretq. Moving the block headed by the comment
> >>>> "Now switch to __KERNEL_CS so IRET works reliably" to just after calling
> >>>> startup_64_setup_env() fixes it and an SEV-ES guest can boot.
> >>>>
> >>>> This worked before because I believe we switched off the EFI CS as part of
> >>>> the kernel decompressor support and so this bug wasn't exposed. But this
> >>>> needs to be fixed regardless of this series.
> >>>>
> >>>
> >>> Very interesting. I was under the assumption that everything that goes
> >>> on in sev_enable() in the decompressor would be rather indispensable
> >>> to boot in SEV mode (which I only spotted today) so I am quite
> >>> surprised that things just appear to work. (There is some 32-bit SEV
> >>> code in the decompressor as well that obviously never gets called when
> >>> booting in long mode, but I hadn't quite grasped how much other SEV
> >>> code there actually is)
> >>
> >> The sev_enable() function for SEV and SEV-ES is more for ensuring a proper
> >> environment (ensuring the proper CPUID bits are present for the guest,
> >> checking the GHCB protocol level, properly preparing pagetables, etc.).
> >> Since we're still in EFI and using the EFI #VC handler and pagetables, we
> >> don't require some of that stuff, but some of the it would need to be
> >> performed at some point (I wasn't trying to be a malicious hypervisor).
> >>
> >
> > OK, good to know.
> >
> >> I haven't tested SEV-SNP yet because the hypervisor support is not
> >> upstream, yet, and I haven't applied your series to an SNP hypervisor
> >> tree, yet. There is a lot of support in sev_enable() for SNP that may
> >> likely cause a problem compared to SEV and SEV-ES.
> >>
> >> SEV-SNP has a lot more checking and validation going on and it ensures it
> >> gets the confidential computing blob from EFI into the boot parameters.
> >> I'm not sure when I'll be able to test SNP, since I'm off next week and
> >> trying to wrap up a bunch of stuff before I leave.
> >>
> >
> > Would it make sense, given the apparent direct dependency on EFI, to
> > move that stuff into the EFI stub instead of doing it from the
> > decompressor?
>
> We tried to make the support for SNP not be reliant on EFI. If EFI is
> present, then get the confidential computing blob from it. Otherwise, the
> cc blob can be supplied as part of the setup data in the Linux boot
> protocol (see find_cc_block() in arch/x86/boot/compressed/sev.c).
>
> So I guess some of it could be moved, but I'm not sure how much.
>

Fair enough.

> >
> > Thanks for giving this a spin - I can wait for more testing until you
> > get back, and hopefully, we'll have converged a bit more by that time
> > and there will be a new revision of the series available for testing.
> >
> > BTW any clue whether your boot path is relying on the EFI handover
> > protocol? I.e., QemuStartLegacyImage() in OVMF? Evgeny mentioned that
>
> I don't believe it is. It's using the Grub EFI bootloader to load and boot
> Linux.
>

Looking at the log you just sent (thanks), it appears you are using
distro shim+grub, which implement their own rough approximation of PE
file loading, and typically use the EFI handover protocol. This is
good news, because we already know how broken shim and GRUB are, and I
was concerned that there might be another issue to track down here.

I'll implement Evgeny's suggestion to clear BSS explicitly in the EFI
handover protocol entrypoints for the next revision of the series.

Thanks,