[v1] Measured SEV boot with kernel/initrd/cmdline

[edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by Dov Murik 2 years, 11 months ago

Booting with SEV prevented the loading of kernel, initrd, and kernel
command-line via QEMU fw_cfg interface because they arrive from the VMM
which is untrusted in SEV.

However, in some cases the kernel, initrd, and cmdline are not secret
but should not be modified by the host.  In such a case, we want to
verify inside the trusted VM that the kernel, initrd, and cmdline are
indeed the ones expected by the Guest Owner, and only if that is the
case go on and boot them up (removing the need for grub inside OVMF in
that mode).

This patch series declares a new page in MEMFD which will contain the
hashes of these three blobs (kernel, initrd, cmdline), each under its
own GUID entry.  This tables of hashes is populated by QEMU before
launch, and encrypted as part of the initial VM memory; this makes sure
theses hashes are part of the SEV measurement (which has to be approved
by the Guest Owner for secret injection, for example).  Note that this
requires a new QEMU patch which will be submitted soon.

OVMF parses the table of hashes populated by QEMU (patch 5), and as it
reads the fw_cfg blobs from QEMU, it will verify each one against the
expected hash (kernel and initrd verifiers are introduced in patch 6,
and command-line verifier is introduced in patches 7+8).  This is all
done inside the trusted VM context.  If all the hashes are correct, boot
of the kernel is allowed to continue.

Any attempt by QEMU to modify the kernel, initrd, cmdline (including
dropping one of them), or to modify the OVMF code that verifies those
hashes, will cause the initial SEV measurement to change and therefore
will be detectable by the Guest Owner during launch before secret
injection.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ashish Kalra <ashish.kalra@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>

James Bottomley (8):
  OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming
  OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg
  OvmfPkg/AmdSev: add a page to the MEMFD for firmware config hashes
  OvmfPkg/QemuKernelLoaderFsDxe: Add ability to verify loaded items
  OvmfPkg/AmdSev: Add library to find encrypted hashes for the FwCfg
    device
  OvmfPkg/AmdSev: Add firmware file plugin to verifier
  OvmfPkg: GenericQemuLoadImageLib: Allow verifying fw_cfg command line
  OvmfPkg/AmdSev: add SevQemuLoadImageLib

 OvmfPkg/OvmfPkg.dec                                                       |  10 ++
 OvmfPkg/AmdSev/AmdSevX64.dsc                                              |   9 +-
 OvmfPkg/AmdSev/AmdSevX64.fdf                                              |   3 +
 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf              |  30 +++++
 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf              |  34 ++++++
 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf        |  30 +++++
 OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf |   2 +
 OvmfPkg/ResetVector/ResetVector.inf                                       |   2 +
 OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h                         |  47 ++++++++
 OvmfPkg/Include/Library/QemuFwCfgLib.h                                    |  35 ++++++
 OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h                  |  11 ++
 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c                |  60 ++++++++++
 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c                | 126 ++++++++++++++++++++
 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c          |  52 ++++++++
 OvmfPkg/AmdSev/SecretDxe/SecretDxe.c                                      |   2 +-
 OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c         |  29 +++++
 OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c                  |   5 +
 OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c                   |  50 ++++++++
 OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c                     |  31 +++++
 OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm                              |  20 ++++
 OvmfPkg/ResetVector/ResetVector.nasmb                                     |   2 +
 21 files changed, 587 insertions(+), 3 deletions(-)
 create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf
 create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf
 create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf
 create mode 100644 OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h
 create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c
 create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c
 create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c
 create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c

-- 
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75567): https://edk2.groups.io/g/devel/message/75567
Mute This Topic: https://groups.io/mt/83074450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by Brijesh Singh 2 years, 11 months ago

On 5/25/21 12:31 AM, Dov Murik wrote:
> Booting with SEV prevented the loading of kernel, initrd, and kernel
> command-line via QEMU fw_cfg interface because they arrive from the VMM
> which is untrusted in SEV.
>
> However, in some cases the kernel, initrd, and cmdline are not secret
> but should not be modified by the host.  In such a case, we want to
> verify inside the trusted VM that the kernel, initrd, and cmdline are
> indeed the ones expected by the Guest Owner, and only if that is the
> case go on and boot them up (removing the need for grub inside OVMF in
> that mode).
>
> This patch series declares a new page in MEMFD which will contain the
> hashes of these three blobs (kernel, initrd, cmdline), each under its
> own GUID entry.  This tables of hashes is populated by QEMU before
> launch, and encrypted as part of the initial VM memory; this makes sure
> theses hashes are part of the SEV measurement (which has to be approved
> by the Guest Owner for secret injection, for example).  Note that this
> requires a new QEMU patch which will be submitted soon.

I have not looked at the patches, but trying to brainstorm if we can
avoid reserving a new page in the MEMFD and use the existing EDK2
infrastructure to verify the blobs (kernel, initrd) loaded through the
FW_CFG interface in the guest memory.

If I understand correctly, then in your proposed approach, guest owner
wants to ensure that the hypevisor passing its preferred kernel, initrd
and cmdline. The guest owner basically knows the hashes of these
components in advance. So, can we do something like this:

- The secret blob provided by the guest owner should contains the hashes
(sha384) of these components.

- Use openssl API available in the edk2 to calculate the hash while
loading the kernel, initrd and cmdline.

- Before booting the kernel, compare the calculated hash with the one
listed in the secret page. If they don't match then fail otherwise continue.

Did I miss something ?

-Brijesh

> OVMF parses the table of hashes populated by QEMU (patch 5), and as it
> reads the fw_cfg blobs from QEMU, it will verify each one against the
> expected hash (kernel and initrd verifiers are introduced in patch 6,
> and command-line verifier is introduced in patches 7+8).  This is all
> done inside the trusted VM context.  If all the hashes are correct, boot
> of the kernel is allowed to continue.
>
> Any attempt by QEMU to modify the kernel, initrd, cmdline (including
> dropping one of them), or to modify the OVMF code that verifies those
> hashes, will cause the initial SEV measurement to change and therefore
> will be detectable by the Guest Owner during launch before secret
> injection.
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ashish Kalra <ashish.kalra@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>
> James Bottomley (8):
>   OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming
>   OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg
>   OvmfPkg/AmdSev: add a page to the MEMFD for firmware config hashes
>   OvmfPkg/QemuKernelLoaderFsDxe: Add ability to verify loaded items
>   OvmfPkg/AmdSev: Add library to find encrypted hashes for the FwCfg
>     device
>   OvmfPkg/AmdSev: Add firmware file plugin to verifier
>   OvmfPkg: GenericQemuLoadImageLib: Allow verifying fw_cfg command line
>   OvmfPkg/AmdSev: add SevQemuLoadImageLib
>
>  OvmfPkg/OvmfPkg.dec                                                       |  10 ++
>  OvmfPkg/AmdSev/AmdSevX64.dsc                                              |   9 +-
>  OvmfPkg/AmdSev/AmdSevX64.fdf                                              |   3 +
>  OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf              |  30 +++++
>  OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf              |  34 ++++++
>  OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf        |  30 +++++
>  OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf |   2 +
>  OvmfPkg/ResetVector/ResetVector.inf                                       |   2 +
>  OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h                         |  47 ++++++++
>  OvmfPkg/Include/Library/QemuFwCfgLib.h                                    |  35 ++++++
>  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h                  |  11 ++
>  OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c                |  60 ++++++++++
>  OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c                | 126 ++++++++++++++++++++
>  OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c          |  52 ++++++++
>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.c                                      |   2 +-
>  OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c         |  29 +++++
>  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c                  |   5 +
>  OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c                   |  50 ++++++++
>  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c                     |  31 +++++
>  OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm                              |  20 ++++
>  OvmfPkg/ResetVector/ResetVector.nasmb                                     |   2 +
>  21 files changed, 587 insertions(+), 3 deletions(-)
>  create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf
>  create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf
>  create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf
>  create mode 100644 OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h
>  create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c
>  create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c
>  create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c
>  create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75585): https://edk2.groups.io/g/devel/message/75585
Mute This Topic: https://groups.io/mt/83074450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by Dov Murik 2 years, 11 months ago

Hi Brijesh,

On 25/05/2021 18:48, Brijesh Singh wrote:
> 
> On 5/25/21 12:31 AM, Dov Murik wrote:
>> Booting with SEV prevented the loading of kernel, initrd, and kernel
>> command-line via QEMU fw_cfg interface because they arrive from the VMM
>> which is untrusted in SEV.
>>
>> However, in some cases the kernel, initrd, and cmdline are not secret
>> but should not be modified by the host.  In such a case, we want to
>> verify inside the trusted VM that the kernel, initrd, and cmdline are
>> indeed the ones expected by the Guest Owner, and only if that is the
>> case go on and boot them up (removing the need for grub inside OVMF in
>> that mode).
>>
>> This patch series declares a new page in MEMFD which will contain the
>> hashes of these three blobs (kernel, initrd, cmdline), each under its
>> own GUID entry.  This tables of hashes is populated by QEMU before
>> launch, and encrypted as part of the initial VM memory; this makes sure
>> theses hashes are part of the SEV measurement (which has to be approved
>> by the Guest Owner for secret injection, for example).  Note that this
>> requires a new QEMU patch which will be submitted soon.
> 
> I have not looked at the patches, but trying to brainstorm if we can
> avoid reserving a new page in the MEMFD and use the existing EDK2
> infrastructure to verify the blobs (kernel, initrd) loaded through the
> FW_CFG interface in the guest memory.
> 
> If I understand correctly, then in your proposed approach, guest owner
> wants to ensure that the hypevisor passing its preferred kernel, initrd
> and cmdline. The guest owner basically knows the hashes of these
> components in advance.

Yes, that's correct.

> So, can we do something like this:
> 
> - The secret blob provided by the guest owner should contains the hashes
> (sha384) of these components.
> 
> - Use openssl API available in the edk2 to calculate the hash while
> loading the kernel, initrd and cmdline.

Indeed we do something similar already - we use Sha256HashAll (see patch
5 in this series).


> 
> - Before booting the kernel, compare the calculated hash with the one
> listed in the secret page. If they don't match then fail otherwise continue.

That is indeed what we do in patch 6 (the calls to our ValidateHashEntry).


> 
> Did I miss something ?

Thanks for proposing this.

Your approach has the advantage that there's no need for extra
pre-allocated MEMFD page for the hashes, and also it makes the QEMU flow
simpler (QEMU doesn't need to compute the hashes and put them in that
special MEMFD page).  I think that the only change we'll need from QEMU
in the x86_load_linux flow (which is when the user supplies
-kernel/-initrd) is that it won't modify any memory in a way that the
modifies the hashes that Guest Owner expects (for example, avoid writing
over the kernel's setup area).

However, the disadvantage is that it unifies boot measurement with the
secret injection.  The Guest Owner _must_ inject the hashes, otherwise
the system doesn't boot; whereas in our current suggestion the Guest
Owner can check the measurement, verify that everything is OK, and just
let the guest continue.

But as I write this, I think that maybe without secret injection the
guest is not really secure? Because the host could just continue
execution of the guest without waiting for measurement check... If the
Guest Owner _must_ inject a secret for SEV to be secure in any case, we
might as well choose your path and let the Guest Owner inject the table
of hashes themselves.

I'd like to hear your (and others') thoughts.

-Dov



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75587): https://edk2.groups.io/g/devel/message/75587
Mute This Topic: https://groups.io/mt/83074450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by Lendacky, Thomas 2 years, 11 months ago

On 5/25/21 3:08 PM, Dov Murik wrote:
> Hi Brijesh,
> 
> On 25/05/2021 18:48, Brijesh Singh wrote:
>>
>> On 5/25/21 12:31 AM, Dov Murik wrote:
>>> Booting with SEV prevented the loading of kernel, initrd, and kernel
>>> command-line via QEMU fw_cfg interface because they arrive from the VMM
>>> which is untrusted in SEV.
>>>
>>> However, in some cases the kernel, initrd, and cmdline are not secret
>>> but should not be modified by the host.  In such a case, we want to
>>> verify inside the trusted VM that the kernel, initrd, and cmdline are
>>> indeed the ones expected by the Guest Owner, and only if that is the
>>> case go on and boot them up (removing the need for grub inside OVMF in
>>> that mode).
>>>
>>> This patch series declares a new page in MEMFD which will contain the
>>> hashes of these three blobs (kernel, initrd, cmdline), each under its
>>> own GUID entry.  This tables of hashes is populated by QEMU before
>>> launch, and encrypted as part of the initial VM memory; this makes sure
>>> theses hashes are part of the SEV measurement (which has to be approved
>>> by the Guest Owner for secret injection, for example).  Note that this
>>> requires a new QEMU patch which will be submitted soon.
>>
>> I have not looked at the patches, but trying to brainstorm if we can
>> avoid reserving a new page in the MEMFD and use the existing EDK2
>> infrastructure to verify the blobs (kernel, initrd) loaded through the
>> FW_CFG interface in the guest memory.
>>
>> If I understand correctly, then in your proposed approach, guest owner
>> wants to ensure that the hypevisor passing its preferred kernel, initrd
>> and cmdline. The guest owner basically knows the hashes of these
>> components in advance.
> 
> Yes, that's correct.
> 
>> So, can we do something like this:
>>
>> - The secret blob provided by the guest owner should contains the hashes
>> (sha384) of these components.
>>
>> - Use openssl API available in the edk2 to calculate the hash while
>> loading the kernel, initrd and cmdline.
> 
> Indeed we do something similar already - we use Sha256HashAll (see patch
> 5 in this series).
> 
> 
>>
>> - Before booting the kernel, compare the calculated hash with the one
>> listed in the secret page. If they don't match then fail otherwise continue.
> 
> That is indeed what we do in patch 6 (the calls to our ValidateHashEntry).
> 
> 
>>
>> Did I miss something ?
> 
> Thanks for proposing this.
> 
> Your approach has the advantage that there's no need for extra
> pre-allocated MEMFD page for the hashes, and also it makes the QEMU flow
> simpler (QEMU doesn't need to compute the hashes and put them in that
> special MEMFD page).  I think that the only change we'll need from QEMU
> in the x86_load_linux flow (which is when the user supplies
> -kernel/-initrd) is that it won't modify any memory in a way that the
> modifies the hashes that Guest Owner expects (for example, avoid writing
> over the kernel's setup area).
> 
> However, the disadvantage is that it unifies boot measurement with the
> secret injection.  The Guest Owner _must_ inject the hashes, otherwise
> the system doesn't boot; whereas in our current suggestion the Guest
> Owner can check the measurement, verify that everything is OK, and just
> let the guest continue.
> 
> But as I write this, I think that maybe without secret injection the
> guest is not really secure? Because the host could just continue
> execution of the guest without waiting for measurement check... If the
> Guest Owner _must_ inject a secret for SEV to be secure in any case, we
> might as well choose your path and let the Guest Owner inject the table
> of hashes themselves.
> 
> I'd like to hear your (and others') thoughts.

Brijesh and I had a long discussion over this. I think that if you're
dealing with a malicious hypervisor, then it could, in fact, measure all
the components that it wants to be used and, using LAUNCH_UPDATE_DATA, add
a page, that matches the format of the guest secret page, to the guest and
indicate that page as the guest secret. Even though the measurement would
fail validation by the guest owner, the hypervisor could ignore it and
continue to run the guest.

So you need something that proves ownership of the guest secret - like a
disk key that would fail to unlock the disk if the hypervisor is faking
the guest secret.

Does all that make sense?

Thanks,
Tom

> 
> -Dov
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75588): https://edk2.groups.io/g/devel/message/75588
Mute This Topic: https://groups.io/mt/83074450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by James Bottomley 2 years, 11 months ago

On Tue, 2021-05-25 at 15:33 -0500, Tom Lendacky wrote:
> On 5/25/21 3:08 PM, Dov Murik wrote:
> > Hi Brijesh,
> > 
> > On 25/05/2021 18:48, Brijesh Singh wrote:
> > > On 5/25/21 12:31 AM, Dov Murik wrote:
> > > > Booting with SEV prevented the loading of kernel, initrd, and
> > > > kernel command-line via QEMU fw_cfg interface because they
> > > > arrive from the VMM which is untrusted in SEV.
> > > > 
> > > > However, in some cases the kernel, initrd, and cmdline are not
> > > > secret but should not be modified by the host.  In such a case,
> > > > we want to verify inside the trusted VM that the kernel,
> > > > initrd, and cmdline are indeed the ones expected by the Guest
> > > > Owner, and only if that is the case go on and boot them up
> > > > (removing the need for grub inside OVMF in that mode).
> > > > 
> > > > This patch series declares a new page in MEMFD which will
> > > > contain the hashes of these three blobs (kernel, initrd,
> > > > cmdline), each under its own GUID entry.  This tables of hashes
> > > > is populated by QEMU before launch, and encrypted as part of
> > > > the initial VM memory; this makes sure theses hashes are part
> > > > of the SEV measurement (which has to be approved by the Guest
> > > > Owner for secret injection, for example).  Note that this
> > > > requires a new QEMU patch which will be submitted soon.
> > > 
> > > I have not looked at the patches, but trying to brainstorm if we
> > > can avoid reserving a new page in the MEMFD and use the existing
> > > EDK2 infrastructure to verify the blobs (kernel, initrd) loaded
> > > through the FW_CFG interface in the guest memory.
> > > 
> > > If I understand correctly, then in your proposed approach, guest
> > > owner wants to ensure that the hypevisor passing its preferred
> > > kernel, initrd and cmdline. The guest owner basically knows the
> > > hashes of these components in advance.
> > 
> > Yes, that's correct.
> > 
> > > So, can we do something like this:
> > > 
> > > - The secret blob provided by the guest owner should contains the
> > > hashes (sha384) of these components.
> > > 
> > > - Use openssl API available in the edk2 to calculate the hash
> > > while loading the kernel, initrd and cmdline.
> > 
> > Indeed we do something similar already - we use Sha256HashAll (see
> > patch 5 in this series).
> > 
> > 
> > > - Before booting the kernel, compare the calculated hash with the
> > > one listed in the secret page. If they don't match then fail
> > > otherwise continue.
> > 
> > That is indeed what we do in patch 6 (the calls to our
> > ValidateHashEntry).
> > 
> > 
> > > Did I miss something ?
> > 
> > Thanks for proposing this.
> > 
> > Your approach has the advantage that there's no need for extra
> > pre-allocated MEMFD page for the hashes, and also it makes the QEMU
> > flow simpler (QEMU doesn't need to compute the hashes and put them
> > in that special MEMFD page).  I think that the only change we'll
> > need from QEMU in the x86_load_linux flow (which is when the user
> > supplies -kernel/-initrd) is that it won't modify any memory in a
> > way that the modifies the hashes that Guest Owner expects (for
> > example, avoid writing over the kernel's setup area).
> > 
> > However, the disadvantage is that it unifies boot measurement with
> > the secret injection.  The Guest Owner _must_ inject the hashes,
> > otherwise the system doesn't boot; whereas in our current
> > suggestion the Guest Owner can check the measurement, verify that
> > everything is OK, and just let the guest continue.
> > 
> > But as I write this, I think that maybe without secret injection
> > the guest is not really secure? Because the host could just
> > continue execution of the guest without waiting for measurement
> > check... If the Guest Owner _must_ inject a secret for SEV to be
> > secure in any case, we might as well choose your path and let the
> > Guest Owner inject the table of hashes themselves.
> > 
> > I'd like to hear your (and others') thoughts.
> 
> Brijesh and I had a long discussion over this. I think that if you're
> dealing with a malicious hypervisor, then it could, in fact, measure
> all the components that it wants to be used and, using
> LAUNCH_UPDATE_DATA, add a page, that matches the format of the guest
> secret page, to the guest and indicate that page as the guest secret.
> Even though the measurement would fail validation by the guest owner,
> the hypervisor could ignore it and continue to run the guest.
> 
> So you need something that proves ownership of the guest secret -
> like a disk key that would fail to unlock the disk if the hypervisor
> is faking the guest secret.
> 
> Does all that make sense?

I think it does for the unencrypted boot case.  For the encrypted boot
case, the HV can't inject the decryption key, because it doesn't know
it, so the interior guest will know there's a problem as soon as it
can't decrypt the image.

But I get the point that we can't rely on the secrets page for hashes
if we have nothing cryptographic in it.  However, the actual threat
from this is somewhat unclear.  As you note: the guest owner knows
there's a problem, but the actual guest is still executing because of
intervention by a malicious hypervisor owner.  In the cloud that's more
or less equivalent to me taking over the guest IP and trying to man in
the middle the services ... once the guest owner knows this happened,
they're going to be looking for a new CSP. So I think the threat would
be potent if you could convince the guest owner that nothing were
amiss, so they think the modified guest is legitimate, but less so
otherwise.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75589): https://edk2.groups.io/g/devel/message/75589
Mute This Topic: https://groups.io/mt/83074450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by Brijesh Singh 2 years, 11 months ago

On 5/25/21 6:15 PM, James Bottomley wrote:
> On Tue, 2021-05-25 at 15:33 -0500, Tom Lendacky wrote:
>> On 5/25/21 3:08 PM, Dov Murik wrote:
>>> Hi Brijesh,
>>>
>>> On 25/05/2021 18:48, Brijesh Singh wrote:
>>>> On 5/25/21 12:31 AM, Dov Murik wrote:
>>>>> Booting with SEV prevented the loading of kernel, initrd, and
>>>>> kernel command-line via QEMU fw_cfg interface because they
>>>>> arrive from the VMM which is untrusted in SEV.
>>>>>
>>>>> However, in some cases the kernel, initrd, and cmdline are not
>>>>> secret but should not be modified by the host.  In such a case,
>>>>> we want to verify inside the trusted VM that the kernel,
>>>>> initrd, and cmdline are indeed the ones expected by the Guest
>>>>> Owner, and only if that is the case go on and boot them up
>>>>> (removing the need for grub inside OVMF in that mode).
>>>>>
>>>>> This patch series declares a new page in MEMFD which will
>>>>> contain the hashes of these three blobs (kernel, initrd,
>>>>> cmdline), each under its own GUID entry.  This tables of hashes
>>>>> is populated by QEMU before launch, and encrypted as part of
>>>>> the initial VM memory; this makes sure theses hashes are part
>>>>> of the SEV measurement (which has to be approved by the Guest
>>>>> Owner for secret injection, for example).  Note that this
>>>>> requires a new QEMU patch which will be submitted soon.
>>>> I have not looked at the patches, but trying to brainstorm if we
>>>> can avoid reserving a new page in the MEMFD and use the existing
>>>> EDK2 infrastructure to verify the blobs (kernel, initrd) loaded
>>>> through the FW_CFG interface in the guest memory.
>>>>
>>>> If I understand correctly, then in your proposed approach, guest
>>>> owner wants to ensure that the hypevisor passing its preferred
>>>> kernel, initrd and cmdline. The guest owner basically knows the
>>>> hashes of these components in advance.
>>> Yes, that's correct.
>>>
>>>> So, can we do something like this:
>>>>
>>>> - The secret blob provided by the guest owner should contains the
>>>> hashes (sha384) of these components.
>>>>
>>>> - Use openssl API available in the edk2 to calculate the hash
>>>> while loading the kernel, initrd and cmdline.
>>> Indeed we do something similar already - we use Sha256HashAll (see
>>> patch 5 in this series).
>>>
>>>
>>>> - Before booting the kernel, compare the calculated hash with the
>>>> one listed in the secret page. If they don't match then fail
>>>> otherwise continue.
>>> That is indeed what we do in patch 6 (the calls to our
>>> ValidateHashEntry).
>>>
>>>
>>>> Did I miss something ?
>>> Thanks for proposing this.
>>>
>>> Your approach has the advantage that there's no need for extra
>>> pre-allocated MEMFD page for the hashes, and also it makes the QEMU
>>> flow simpler (QEMU doesn't need to compute the hashes and put them
>>> in that special MEMFD page).  I think that the only change we'll
>>> need from QEMU in the x86_load_linux flow (which is when the user
>>> supplies -kernel/-initrd) is that it won't modify any memory in a
>>> way that the modifies the hashes that Guest Owner expects (for
>>> example, avoid writing over the kernel's setup area).
>>>
>>> However, the disadvantage is that it unifies boot measurement with
>>> the secret injection.  The Guest Owner _must_ inject the hashes,
>>> otherwise the system doesn't boot; whereas in our current
>>> suggestion the Guest Owner can check the measurement, verify that
>>> everything is OK, and just let the guest continue.
>>>
>>> But as I write this, I think that maybe without secret injection
>>> the guest is not really secure? Because the host could just
>>> continue execution of the guest without waiting for measurement
>>> check... If the Guest Owner _must_ inject a secret for SEV to be
>>> secure in any case, we might as well choose your path and let the
>>> Guest Owner inject the table of hashes themselves.
>>>
>>> I'd like to hear your (and others') thoughts.
>> Brijesh and I had a long discussion over this. I think that if you're
>> dealing with a malicious hypervisor, then it could, in fact, measure
>> all the components that it wants to be used and, using
>> LAUNCH_UPDATE_DATA, add a page, that matches the format of the guest
>> secret page, to the guest and indicate that page as the guest secret.
>> Even though the measurement would fail validation by the guest owner,
>> the hypervisor could ignore it and continue to run the guest.
>>
>> So you need something that proves ownership of the guest secret -
>> like a disk key that would fail to unlock the disk if the hypervisor
>> is faking the guest secret.
>>
>> Does all that make sense?
> I think it does for the unencrypted boot case.  For the encrypted boot
> case, the HV can't inject the decryption key, because it doesn't know
> it, so the interior guest will know there's a problem as soon as it
> can't decrypt the image.
>
> But I get the point that we can't rely on the secrets page for hashes
> if we have nothing cryptographic in it.  However, the actual threat
> from this is somewhat unclear.  As you note: the guest owner knows
> there's a problem, but the actual guest is still executing because of
> intervention by a malicious hypervisor owner.  In the cloud that's more
> or less equivalent to me taking over the guest IP and trying to man in
> the middle the services ... once the guest owner knows this happened,
> they're going to be looking for a new CSP. So I think the threat would
> be potent if you could convince the guest owner that nothing were
> amiss, so they think the modified guest is legitimate, but less so
> otherwise.

I guess if we can have the guest BIOS communicate with the guest owner
using a secure channel to obtain the hash'es or key to decrypt the
hashes then all of it be okay. To establish the secret channel the guest
BIOS will use the secret injected through the LAUNCH_SECRET. If OVMF
fails to establish the secret channel with the guest owner, then its an
indicate that malicious hypervisor booted the guest without injecting a
valid secret or skipped the attestation flow.


>
> James
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75590): https://edk2.groups.io/g/devel/message/75590
Mute This Topic: https://groups.io/mt/83074450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by Dov Murik 2 years, 11 months ago


On 26/05/2021 2:37, Brijesh Singh wrote:
> 
> On 5/25/21 6:15 PM, James Bottomley wrote:
>> On Tue, 2021-05-25 at 15:33 -0500, Tom Lendacky wrote:
>>> On 5/25/21 3:08 PM, Dov Murik wrote:
>>>> Hi Brijesh,
>>>>
>>>> On 25/05/2021 18:48, Brijesh Singh wrote:
>>>>> On 5/25/21 12:31 AM, Dov Murik wrote:
>>>>>> Booting with SEV prevented the loading of kernel, initrd, and
>>>>>> kernel command-line via QEMU fw_cfg interface because they
>>>>>> arrive from the VMM which is untrusted in SEV.
>>>>>>
>>>>>> However, in some cases the kernel, initrd, and cmdline are not
>>>>>> secret but should not be modified by the host.  In such a case,
>>>>>> we want to verify inside the trusted VM that the kernel,
>>>>>> initrd, and cmdline are indeed the ones expected by the Guest
>>>>>> Owner, and only if that is the case go on and boot them up
>>>>>> (removing the need for grub inside OVMF in that mode).
>>>>>>
>>>>>> This patch series declares a new page in MEMFD which will
>>>>>> contain the hashes of these three blobs (kernel, initrd,
>>>>>> cmdline), each under its own GUID entry.  This tables of hashes
>>>>>> is populated by QEMU before launch, and encrypted as part of
>>>>>> the initial VM memory; this makes sure theses hashes are part
>>>>>> of the SEV measurement (which has to be approved by the Guest
>>>>>> Owner for secret injection, for example).  Note that this
>>>>>> requires a new QEMU patch which will be submitted soon.
>>>>> I have not looked at the patches, but trying to brainstorm if we
>>>>> can avoid reserving a new page in the MEMFD and use the existing
>>>>> EDK2 infrastructure to verify the blobs (kernel, initrd) loaded
>>>>> through the FW_CFG interface in the guest memory.
>>>>>
>>>>> If I understand correctly, then in your proposed approach, guest
>>>>> owner wants to ensure that the hypevisor passing its preferred
>>>>> kernel, initrd and cmdline. The guest owner basically knows the
>>>>> hashes of these components in advance.
>>>> Yes, that's correct.
>>>>
>>>>> So, can we do something like this:
>>>>>
>>>>> - The secret blob provided by the guest owner should contains the
>>>>> hashes (sha384) of these components.
>>>>>
>>>>> - Use openssl API available in the edk2 to calculate the hash
>>>>> while loading the kernel, initrd and cmdline.
>>>> Indeed we do something similar already - we use Sha256HashAll (see
>>>> patch 5 in this series).
>>>>
>>>>
>>>>> - Before booting the kernel, compare the calculated hash with the
>>>>> one listed in the secret page. If they don't match then fail
>>>>> otherwise continue.
>>>> That is indeed what we do in patch 6 (the calls to our
>>>> ValidateHashEntry).
>>>>
>>>>
>>>>> Did I miss something ?
>>>> Thanks for proposing this.
>>>>
>>>> Your approach has the advantage that there's no need for extra
>>>> pre-allocated MEMFD page for the hashes, and also it makes the QEMU
>>>> flow simpler (QEMU doesn't need to compute the hashes and put them
>>>> in that special MEMFD page).  I think that the only change we'll
>>>> need from QEMU in the x86_load_linux flow (which is when the user
>>>> supplies -kernel/-initrd) is that it won't modify any memory in a
>>>> way that the modifies the hashes that Guest Owner expects (for
>>>> example, avoid writing over the kernel's setup area).
>>>>
>>>> However, the disadvantage is that it unifies boot measurement with
>>>> the secret injection.  The Guest Owner _must_ inject the hashes,
>>>> otherwise the system doesn't boot; whereas in our current
>>>> suggestion the Guest Owner can check the measurement, verify that
>>>> everything is OK, and just let the guest continue.
>>>>
>>>> But as I write this, I think that maybe without secret injection
>>>> the guest is not really secure? Because the host could just
>>>> continue execution of the guest without waiting for measurement
>>>> check... If the Guest Owner _must_ inject a secret for SEV to be
>>>> secure in any case, we might as well choose your path and let the
>>>> Guest Owner inject the table of hashes themselves.
>>>>
>>>> I'd like to hear your (and others') thoughts.
>>> Brijesh and I had a long discussion over this. I think that if you're
>>> dealing with a malicious hypervisor, then it could, in fact, measure
>>> all the components that it wants to be used and, using
>>> LAUNCH_UPDATE_DATA, add a page, that matches the format of the guest
>>> secret page, to the guest and indicate that page as the guest secret.
>>> Even though the measurement would fail validation by the guest owner,
>>> the hypervisor could ignore it and continue to run the guest.
>>>
>>> So you need something that proves ownership of the guest secret -
>>> like a disk key that would fail to unlock the disk if the hypervisor
>>> is faking the guest secret.
>>>
>>> Does all that make sense?
>> I think it does for the unencrypted boot case.  For the encrypted boot
>> case, the HV can't inject the decryption key, because it doesn't know
>> it, so the interior guest will know there's a problem as soon as it
>> can't decrypt the image.

I think we all agree that encrypted disk is one good solution to prove
that the guest owner actually provided the secret.


>>
>> But I get the point that we can't rely on the secrets page for hashes
>> if we have nothing cryptographic in it.  However, the actual threat
>> from this is somewhat unclear.  As you note: the guest owner knows
>> there's a problem, but the actual guest is still executing because of
>> intervention by a malicious hypervisor owner.  In the cloud that's more
>> or less equivalent to me taking over the guest IP and trying to man in
>> the middle the services ... once the guest owner knows this happened,
>> they're going to be looking for a new CSP. So I think the threat would
>> be potent if you could convince the guest owner that nothing were
>> amiss, so they think the modified guest is legitimate, but less so
>> otherwise.
> 
> I guess if we can have the guest BIOS communicate with the guest owner
> using a secure channel to obtain the hash'es or key to decrypt the
> hashes then all of it be okay. To establish the secret channel the guest
> BIOS will use the secret injected through the LAUNCH_SECRET. If OVMF
> fails to establish the secret channel with the guest owner, then its an
> indicate that malicious hypervisor booted the guest without injecting a
> valid secret or skipped the attestation flow.
> 

We can separate to two cases:

(1-lax) Guest owner wants proof of integrity/trust for the VMs it asked
to launch (checking the measurement). Cloud provider may be able to
launch additional instances and nobody will know about it.

(2-strict) Guest owner wants control of all VM launches; doesn't allow
cloud provider to launch VMs without approval.




For (1-lax) -- the current proposal works OK.  When Guest Owner asks to
launch a new VM, it'll receive the measurement and check it.  If it is
wrong, the guest owner will stop paying the cloud provider.

In such a case nothing prevents the cloud provider to start the VM on
its own terms -- with SEV disabled, with its own OVMF that performs no
hash checks, etc.



For (2-strict) -- Any check that relies on a simple 'if' statement (like
'if hash(kernel)==expected_hash' or 'if verify_sig(some_blob,
guest_owner_pub_key)') can be circumvented by modifying the code of the
if statement itself (or simply using an OVMF that doesn't perform these
checks).  This includes our original proposal in this patch series, and
Brijesh's idea of communicating with guest owner to get the hashes and
then check them.  Like Tom said, measurement doesn't match, but
malicious cloud provider doesn't care because it doesn't need the secret
injection from the guest owner.

It seems like the guest workload *must* heavily rely on an injected
secret which can be one (or more) of:

(a) a decryption key; it can be an encrypted root partition, or some
other crucial (and unguessable[*]) part of the guest workload.

(b) an API key/credentials that are checked *outside* the cloud
provider's reach; the response from that API is a crucial part of the
guest workload.

(c) asymmetric key (like SSH private key) to access a crucial service
which is part of the guest workload.

The crux here is the "crucial part of the guest workload"; the cloud
provider can start the VM without the secret, but such VM can't perform
its actual (confidential) mission.


[*] Initially I thought about adding encrypted-kernel (similar to
encrypted boot partition; the kernel is decrypted in OVMF using the
injected secret).  But I think a malicious hypervisor can simply guess
what kernel is needed, and provide its own cleartext kernel (with plain
OVMF) and successfully boot the guest without the secret.


-Dov


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75596): https://edk2.groups.io/g/devel/message/75596
Mute This Topic: https://groups.io/mt/83074450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by Dov Murik 2 years, 11 months ago


On 25/05/2021 8:31, Dov Murik wrote:
> Booting with SEV prevented the loading of kernel, initrd, and kernel
> command-line via QEMU fw_cfg interface because they arrive from the VMM
> which is untrusted in SEV.
> 
> However, in some cases the kernel, initrd, and cmdline are not secret
> but should not be modified by the host.  In such a case, we want to
> verify inside the trusted VM that the kernel, initrd, and cmdline are
> indeed the ones expected by the Guest Owner, and only if that is the
> case go on and boot them up (removing the need for grub inside OVMF in
> that mode).
> 
> This patch series declares a new page in MEMFD which will contain the
> hashes of these three blobs (kernel, initrd, cmdline), each under its
> own GUID entry.  This tables of hashes is populated by QEMU before
> launch, and encrypted as part of the initial VM memory; this makes sure
> theses hashes are part of the SEV measurement (which has to be approved
> by the Guest Owner for secret injection, for example).  Note that this
> requires a new QEMU patch which will be submitted soon.


The QEMU patch is submitted at:

https://lore.kernel.org/qemu-devel/20210525065931.1628554-1-dovmurik@linux.ibm.com/

-Dov



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75577): https://edk2.groups.io/g/devel/message/75577
Mute This Topic: https://groups.io/mt/83074450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by Laszlo Ersek 2 years, 11 months ago

Ard,

I'll have a specific question for you below; please feel free to jump
forward (search for your name). Thanks.

Dov, my comments below:

On 05/25/21 07:31, Dov Murik wrote:
> Booting with SEV prevented the loading of kernel, initrd, and kernel
> command-line via QEMU fw_cfg interface because they arrive from the VMM
> which is untrusted in SEV.
> 
> However, in some cases the kernel, initrd, and cmdline are not secret
> but should not be modified by the host.  In such a case, we want to
> verify inside the trusted VM that the kernel, initrd, and cmdline are
> indeed the ones expected by the Guest Owner, and only if that is the
> case go on and boot them up (removing the need for grub inside OVMF in
> that mode).
> 
> This patch series declares a new page in MEMFD which will contain the
> hashes of these three blobs (kernel, initrd, cmdline), each under its
> own GUID entry.  This tables of hashes is populated by QEMU before
> launch, and encrypted as part of the initial VM memory; this makes sure
> theses hashes are part of the SEV measurement (which has to be approved
> by the Guest Owner for secret injection, for example).  Note that this
> requires a new QEMU patch which will be submitted soon.
> 
> OVMF parses the table of hashes populated by QEMU (patch 5), and as it
> reads the fw_cfg blobs from QEMU, it will verify each one against the
> expected hash (kernel and initrd verifiers are introduced in patch 6,
> and command-line verifier is introduced in patches 7+8).  This is all
> done inside the trusted VM context.  If all the hashes are correct, boot
> of the kernel is allowed to continue.
> 
> Any attempt by QEMU to modify the kernel, initrd, cmdline (including
> dropping one of them), or to modify the OVMF code that verifies those
> hashes, will cause the initial SEV measurement to change and therefore
> will be detectable by the Guest Owner during launch before secret
> injection.

Please catch the error in my reasoning below.

The goal is for the guest firmware to ensure the authenticity
(integrity) of kernel, initrd, cmdline.

This is not really different from a normal Secure Boot setup, where the
guest firmware verifies the kernel image (presented as a UEFI
application, i.e. with the UEFI stub). It is up to the kernel to verify
the integrity of the initrd. The command line is not particularly
verified (as far as I know?), but if that's a problem, it should be
solved even for bare metal Secure Boot use cases. (Because, if the
"root" user is compromised on a running Linux system, they can modify
the kernel params for next boot in the grub config.)

The AmdSevX64 platform uses a unified firmware image (executable +
varstore are presented as one blob, no separate CODE and VARS). There is
one pflash chip, and the initial guest-owner-side measurement covers the
whole blob, including the varstore.

This suggests that the guest owner could boot the unified firmware image
in a trusted guest environment first, and use UEFI-level tools to enroll
various SB certificates. Then this modified image would be deployed
every time to the untrusted cloud.

The AmdSevX64 platform could adopt a PlatformBootManagerLib instance
where the TryRunningQemuKernel() call is reinstated, backed by the usual
QemuLoadImageLib class APIs QemuLoadKernelImage() and
QemuStartKernelImage().

edk2 offers two QemuLoadImageLib instances, GenericQemuLoadImageLib and
X86QemuLoadImageLib. The former strictly enforces SB verification. That
was in fact a *problem* for the traditional OvmfPkg platforms; please
refer to commit 82808b422617 ("Revert "OvmfPkg: use generic QEMU image
loader for secure boot enabled ..."", 2020-06-16). But the same rigor
seems just right here, for the AmdSevX64 platform.

Where I see a gap in all this myself -- and of course there could be
plenty other gaps that I just don't see -- is the varstore's protection
from the hypervisor, once the guest is up and running. Can we discuss
that perhaps?

If necessary, we could perhaps rework the AmdSevX64 platform to drop the
pflash-backed variable driver stack, and use in-RAM (memory-only)
variable emulation. Actual persistence / non-volatility of UEFI
variables may not really be relevant for the remotely attested platform,
but keeping all the variables in RAM would subject the varstore to
memory encryption / protection. And perhaps we could integrate the
enrollment of SB certificates into the *code* part of the firmware, with
gRT->SetVariable() calls. (Normally this would be absolutely horrible,
but for the remotely attested platform, anything goes.)

I simply dislike adding brand new code for a use case which at least
*appears* to significantly overlap with that of Secure Boot. Secure Boot
is about image verification, and it's rooted in tamper-resistant storage
of certificates and/or image hashes. If we can figure out "tamper
resistant" in the current context, we could perhaps reuse much of the
existent SB infrastructure.

----*----

Considering the particular approach in this set:

- To reiterate Brijesh's point, I feel a new MEMFD page is wasteful. If
we really need the MEMFD approach, I'd *really* like us to extend one of
the existent structures. If necessary, introduce a new GUID, for a table
that contains both previously injected data, and the new data. If all
that's impossible or too awkward, please document why.

- Modifying the QemuFwCfgLib class for this purpose is inappropriate.
Even if we do our own home-brewed verifier, none of it must go into
QemuFwCfgLib class. QemuFwCfgLib is for transport.

[Ard, please see this one question:]

- A major complication for hashing all three of: kernel, initrd,
cmdline, is that the *fetching* of this triplet is split between two
places. (Well, it is split between *three* places in fact, but I'm going
to ignore LinuxInitrdDynamicShellCommand for now, because the AmdSevX64
platform sets BUILD_SHELL to FALSE for production.)

The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but the
command line is fetched in (both) QemuLoadImageLib instances. This
requires that all these modules be littered with hashing as well, which
I find *really bad*. Even if we factor out the actual logic, I strongly
dislike having *just hooks* for hashing in multiple modules.

Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe: don't
expose kernel command line", 2020-03-05). If we first

(a) reverted that commit, and

(b) modified *both* QemuLoadImageLib instances, to load the kernel
command line from the *synthetic filesystem* (rather than directly from
fw_cfg),

then we could centralize the hashing to just QemuKernelLoaderFsDxe.

Ard -- what's your thought on this?

And then, we could eliminate the dynamic callback registration, plus the
separate SevFwCfgVerifier, SevHashFinderLib, and SevQemuLoadImageLib stuff.

We'd only need one new lib class, with *statically linked* hooks for
QemuKernelLoaderFsDxe, and two instances of this new class, a Null one,
and an actual (SEV hash verifier) one. The latter instance would locate
the hash values, calculate the fresh hashes, and perform the
comparisons. Only the AmdSevX64 platform would use the non-Null instance
of this library class.

(NB QemuKernelLoaderFsDxe is used by some ArmVirtPkg platforms, so
resolutions to the Null instance would be required there too.)

Thanks
Laszlo

> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ashish Kalra <ashish.kalra@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> 
> James Bottomley (8):
>   OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming
>   OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg
>   OvmfPkg/AmdSev: add a page to the MEMFD for firmware config hashes
>   OvmfPkg/QemuKernelLoaderFsDxe: Add ability to verify loaded items
>   OvmfPkg/AmdSev: Add library to find encrypted hashes for the FwCfg
>     device
>   OvmfPkg/AmdSev: Add firmware file plugin to verifier
>   OvmfPkg: GenericQemuLoadImageLib: Allow verifying fw_cfg command line
>   OvmfPkg/AmdSev: add SevQemuLoadImageLib
> 
>  OvmfPkg/OvmfPkg.dec                                                       |  10 ++
>  OvmfPkg/AmdSev/AmdSevX64.dsc                                              |   9 +-
>  OvmfPkg/AmdSev/AmdSevX64.fdf                                              |   3 +
>  OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf              |  30 +++++
>  OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf              |  34 ++++++
>  OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf        |  30 +++++
>  OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf |   2 +
>  OvmfPkg/ResetVector/ResetVector.inf                                       |   2 +
>  OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h                         |  47 ++++++++
>  OvmfPkg/Include/Library/QemuFwCfgLib.h                                    |  35 ++++++
>  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h                  |  11 ++
>  OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c                |  60 ++++++++++
>  OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c                | 126 ++++++++++++++++++++
>  OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c          |  52 ++++++++
>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.c                                      |   2 +-
>  OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c         |  29 +++++
>  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c                  |   5 +
>  OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c                   |  50 ++++++++
>  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c                     |  31 +++++
>  OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm                              |  20 ++++
>  OvmfPkg/ResetVector/ResetVector.nasmb                                     |   2 +
>  21 files changed, 587 insertions(+), 3 deletions(-)
>  create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf
>  create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf
>  create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf
>  create mode 100644 OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h
>  create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c
>  create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c
>  create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c
>  create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c
> 

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75896): https://edk2.groups.io/g/devel/message/75896
Mute This Topic: https://groups.io/mt/83074450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by Ard Biesheuvel 2 years, 11 months ago

On Tue, 1 Jun 2021 at 14:12, Laszlo Ersek <lersek@redhat.com> wrote:
>
...
> - A major complication for hashing all three of: kernel, initrd,
> cmdline, is that the *fetching* of this triplet is split between two
> places. (Well, it is split between *three* places in fact, but I'm going
> to ignore LinuxInitrdDynamicShellCommand for now, because the AmdSevX64
> platform sets BUILD_SHELL to FALSE for production.)
>
> The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but the
> command line is fetched in (both) QemuLoadImageLib instances. This
> requires that all these modules be littered with hashing as well, which
> I find *really bad*. Even if we factor out the actual logic, I strongly
> dislike having *just hooks* for hashing in multiple modules.
>
> Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe: don't
> expose kernel command line", 2020-03-05). If we first
>
> (a) reverted that commit, and
>
> (b) modified *both* QemuLoadImageLib instances, to load the kernel
> command line from the *synthetic filesystem* (rather than directly from
> fw_cfg),
>
> then we could centralize the hashing to just QemuKernelLoaderFsDxe.
>
> Ard -- what's your thought on this?
>

I don't have any problems with that - I take it this means we can drop
the QemuFwCfgLib dependency from GenericQemuLoadImageLib altogether,
right?

>
> And then, we could eliminate the dynamic callback registration, plus the
> separate SevFwCfgVerifier, SevHashFinderLib, and SevQemuLoadImageLib stuff.
>
> We'd only need one new lib class, with *statically linked* hooks for
> QemuKernelLoaderFsDxe, and two instances of this new class, a Null one,
> and an actual (SEV hash verifier) one. The latter instance would locate
> the hash values, calculate the fresh hashes, and perform the
> comparisons. Only the AmdSevX64 platform would use the non-Null instance
> of this library class.
>
> (NB QemuKernelLoaderFsDxe is used by some ArmVirtPkg platforms, so
> resolutions to the Null instance would be required there too.)
>

This sounds like a good approach to me.


>
> >
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Cc: Ashish Kalra <ashish.kalra@amd.com>
> > Cc: Brijesh Singh <brijesh.singh@amd.com>
> > Cc: Erdem Aktas <erdemaktas@google.com>
> > Cc: James Bottomley <jejb@linux.ibm.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Min Xu <min.m.xu@intel.com>
> > Cc: Tom Lendacky <thomas.lendacky@amd.com>
> >
> > James Bottomley (8):
> >   OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming
> >   OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg
> >   OvmfPkg/AmdSev: add a page to the MEMFD for firmware config hashes
> >   OvmfPkg/QemuKernelLoaderFsDxe: Add ability to verify loaded items
> >   OvmfPkg/AmdSev: Add library to find encrypted hashes for the FwCfg
> >     device
> >   OvmfPkg/AmdSev: Add firmware file plugin to verifier
> >   OvmfPkg: GenericQemuLoadImageLib: Allow verifying fw_cfg command line
> >   OvmfPkg/AmdSev: add SevQemuLoadImageLib
> >
> >  OvmfPkg/OvmfPkg.dec                                                       |  10 ++
> >  OvmfPkg/AmdSev/AmdSevX64.dsc                                              |   9 +-
> >  OvmfPkg/AmdSev/AmdSevX64.fdf                                              |   3 +
> >  OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf              |  30 +++++
> >  OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf              |  34 ++++++
> >  OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf        |  30 +++++
> >  OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf |   2 +
> >  OvmfPkg/ResetVector/ResetVector.inf                                       |   2 +
> >  OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h                         |  47 ++++++++
> >  OvmfPkg/Include/Library/QemuFwCfgLib.h                                    |  35 ++++++
> >  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h                  |  11 ++
> >  OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c                |  60 ++++++++++
> >  OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c                | 126 ++++++++++++++++++++
> >  OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c          |  52 ++++++++
> >  OvmfPkg/AmdSev/SecretDxe/SecretDxe.c                                      |   2 +-
> >  OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c         |  29 +++++
> >  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c                  |   5 +
> >  OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c                   |  50 ++++++++
> >  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c                     |  31 +++++
> >  OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm                              |  20 ++++
> >  OvmfPkg/ResetVector/ResetVector.nasmb                                     |   2 +
> >  21 files changed, 587 insertions(+), 3 deletions(-)
> >  create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf
> >  create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf
> >  create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf
> >  create mode 100644 OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h
> >  create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c
> >  create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c
> >  create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c
> >  create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c
> >
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75908): https://edk2.groups.io/g/devel/message/75908
Mute This Topic: https://groups.io/mt/83074450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by Laszlo Ersek 2 years, 11 months ago

On 06/01/21 15:20, Ard Biesheuvel wrote:
> On Tue, 1 Jun 2021 at 14:12, Laszlo Ersek <lersek@redhat.com> wrote:
>>
> ...
>> - A major complication for hashing all three of: kernel, initrd,
>> cmdline, is that the *fetching* of this triplet is split between two
>> places. (Well, it is split between *three* places in fact, but I'm
>> going to ignore LinuxInitrdDynamicShellCommand for now, because the
>> AmdSevX64 platform sets BUILD_SHELL to FALSE for production.)
>>
>> The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but
>> the command line is fetched in (both) QemuLoadImageLib instances.
>> This requires that all these modules be littered with hashing as
>> well, which I find *really bad*. Even if we factor out the actual
>> logic, I strongly dislike having *just hooks* for hashing in multiple
>> modules.
>>
>> Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe:
>> don't expose kernel command line", 2020-03-05). If we first
>>
>> (a) reverted that commit, and
>>
>> (b) modified *both* QemuLoadImageLib instances, to load the kernel
>> command line from the *synthetic filesystem* (rather than directly
>> from fw_cfg),
>>
>> then we could centralize the hashing to just QemuKernelLoaderFsDxe.
>>
>> Ard -- what's your thought on this?
>>
>
> I don't have any problems with that - I take it this means we can drop
> the QemuFwCfgLib dependency from GenericQemuLoadImageLib altogether,
> right?

A bit more work is needed for that (but I agree it should be done),
because we have this additionally:

  QemuFwCfgSelectItem (QemuFwCfgItemInitrdSize);
  InitrdSize = (UINTN)QemuFwCfgRead32 ();

  if (InitrdSize > 0) {
    //
    // Append ' initrd=initrd' in UTF-16.
    //
    KernelLoadedImage->LoadOptionsSize += sizeof (L" initrd=initrd") - 2;
  }

This should also be rebased on top of the synthetic filesystem [*], and
then no more QemuFwCfgLib calls should remain.

[*] From StubFileOpen() in "QemuKernelLoaderFsDxe.c", it seems that we
    successfully open zero-sized fw_cfg files. (We also list them, when
    reading the root directory, in StubFileRead()). That's not a problem
    at all, but it means that, after opening the initrd file temporarily
    in QemuLoadImageLib, EFI_FILE_PROTOCOL.GetInfo() should be used for
    fetching an EFI_FILE_INFO, and then EFI_FILE_INFO.FileSize needs to
    be compared to 0.


>
>>
>> And then, we could eliminate the dynamic callback registration, plus
>> the separate SevFwCfgVerifier, SevHashFinderLib, and
>> SevQemuLoadImageLib stuff.
>>
>> We'd only need one new lib class, with *statically linked* hooks for
>> QemuKernelLoaderFsDxe, and two instances of this new class, a Null
>> one, and an actual (SEV hash verifier) one. The latter instance would
>> locate the hash values, calculate the fresh hashes, and perform the
>> comparisons. Only the AmdSevX64 platform would use the non-Null
>> instance of this library class.
>>
>> (NB QemuKernelLoaderFsDxe is used by some ArmVirtPkg platforms, so
>> resolutions to the Null instance would be required there too.)
>>
>
> This sounds like a good approach to me.

Thank you!
Laszlo

>
>
>>
>>>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Ashish Kalra <ashish.kalra@amd.com>
>>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>>> Cc: Erdem Aktas <erdemaktas@google.com>
>>> Cc: James Bottomley <jejb@linux.ibm.com>
>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>> Cc: Min Xu <min.m.xu@intel.com>
>>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>>>
>>> James Bottomley (8):
>>>   OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming
>>>   OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg
>>>   OvmfPkg/AmdSev: add a page to the MEMFD for firmware config hashes
>>>   OvmfPkg/QemuKernelLoaderFsDxe: Add ability to verify loaded items
>>>   OvmfPkg/AmdSev: Add library to find encrypted hashes for the FwCfg
>>>     device
>>>   OvmfPkg/AmdSev: Add firmware file plugin to verifier
>>>   OvmfPkg: GenericQemuLoadImageLib: Allow verifying fw_cfg command line
>>>   OvmfPkg/AmdSev: add SevQemuLoadImageLib
>>>
>>>  OvmfPkg/OvmfPkg.dec                                                       |  10 ++
>>>  OvmfPkg/AmdSev/AmdSevX64.dsc                                              |   9 +-
>>>  OvmfPkg/AmdSev/AmdSevX64.fdf                                              |   3 +
>>>  OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf              |  30 +++++
>>>  OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf              |  34 ++++++
>>>  OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf        |  30 +++++
>>>  OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf |   2 +
>>>  OvmfPkg/ResetVector/ResetVector.inf                                       |   2 +
>>>  OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h                         |  47 ++++++++
>>>  OvmfPkg/Include/Library/QemuFwCfgLib.h                                    |  35 ++++++
>>>  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h                  |  11 ++
>>>  OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c                |  60 ++++++++++
>>>  OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c                | 126 ++++++++++++++++++++
>>>  OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c          |  52 ++++++++
>>>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.c                                      |   2 +-
>>>  OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c         |  29 +++++
>>>  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c                  |   5 +
>>>  OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c                   |  50 ++++++++
>>>  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c                     |  31 +++++
>>>  OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm                              |  20 ++++
>>>  OvmfPkg/ResetVector/ResetVector.nasmb                                     |   2 +
>>>  21 files changed, 587 insertions(+), 3 deletions(-)
>>>  create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf
>>>  create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf
>>>  create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf
>>>  create mode 100644 OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h
>>>  create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c
>>>  create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c
>>>  create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c
>>>  create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c
>>>
>>
>
>
> 
>
>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75919): https://edk2.groups.io/g/devel/message/75919
Mute This Topic: https://groups.io/mt/83074450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by James Bottomley 2 years, 11 months ago

On Tue, 2021-06-01 at 14:11 +0200, Laszlo Ersek wrote:
> Ard,
> 
> I'll have a specific question for you below; please feel free to jump
> forward (search for your name). Thanks.
> 
> Dov, my comments below:
> 
> On 05/25/21 07:31, Dov Murik wrote:
> > Booting with SEV prevented the loading of kernel, initrd, and
> > kernel command-line via QEMU fw_cfg interface because they arrive
> > from the VMM which is untrusted in SEV.
> > 
> > However, in some cases the kernel, initrd, and cmdline are not
> > secret but should not be modified by the host.  In such a case, we
> > want to verify inside the trusted VM that the kernel, initrd, and
> > cmdline are indeed the ones expected by the Guest Owner, and only
> > if that is the case go on and boot them up (removing the need for
> > grub inside OVMF in that mode).
> > 
> > This patch series declares a new page in MEMFD which will contain
> > the hashes of these three blobs (kernel, initrd, cmdline), each
> > under its own GUID entry.  This tables of hashes is populated by
> > QEMU before launch, and encrypted as part of the initial VM memory;
> > this makes sure theses hashes are part of the SEV measurement
> > (which has to be approved by the Guest Owner for secret injection,
> > for example).  Note that this requires a new QEMU patch which will
> > be submitted soon.
> > 
> > OVMF parses the table of hashes populated by QEMU (patch 5), and as
> > it reads the fw_cfg blobs from QEMU, it will verify each one
> > against the expected hash (kernel and initrd verifiers are
> > introduced in patch 6, and command-line verifier is introduced in
> > patches 7+8).  This is all done inside the trusted VM context.  If
> > all the hashes are correct, boot of the kernel is allowed to continue.
> > 
> > Any attempt by QEMU to modify the kernel, initrd, cmdline
> > (including dropping one of them), or to modify the OVMF code that
> > verifies those hashes, will cause the initial SEV measurement to
> > change and therefore will be detectable by the Guest Owner during
> > launch before secret injection.
> 
> Please catch the error in my reasoning below.
> 
> The goal is for the guest firmware to ensure the authenticity
> (integrity) of kernel, initrd, cmdline.

That's right

> This is not really different from a normal Secure Boot setup, where
> the guest firmware verifies the kernel image (presented as a UEFI
> application, i.e. with the UEFI stub). It is up to the kernel to
> verify the integrity of the initrd.

Right, but this doesn't happen today (there's no initrd measure in the
kernel), so the only current choice you have is to combine the kernel
and initrd then sign the whole combination, which gives one signature
for both.

>  The command line is not particularly verified (as far as I know?), 

Right, it's not, which means secure boot can't replace the
verification, because the command line is an essential thing to measure
given the things it can do to the booting kernel.

> but if that's a problem, it should be solved even for bare metal
> Secure Boot use cases. (Because, if the "root" user is compromised on
> a running Linux system, they can modify the kernel params for next
> boot in the grub config.)

The usual statement to this is that secure boot doesn't need to do this
because the commandline is a measured boot problem, so grub measures
its entire execution to PCR 8.  However, then you have to grope around
in the measurement log and verify it via a TPM quote, which is
incredibly sophisticated stuff compared to taking a single measurement
hash.  Plus, we have no secure TPM currently for virtual machines, so
there's no measured boot measurement the guest owner can trust.

> The AmdSevX64 platform uses a unified firmware image (executable +
> varstore are presented as one blob, no separate CODE and VARS). There
> is one pflash chip, and the initial guest-owner-side measurement
> covers the whole blob, including the varstore.

That's right, except being part of a single rom volume, the varstore is
read only.  This is a deliberate design choice: the absence of SMM and
the fact that a R/W interface wouldn't get measured properly and also
because NV  config changes can be used to effect secret leakage means
it needs to be immutable.

If it's mutable, you need to store it separately, protect it with
something like SMM and be aware of how the measurement would change
when the store was updated ... which is another thing that's not that
easy because of the way flash operates on overwrite.

Finally there's the secure write problem: the DMA for the variable
write would have to go through an unencrypted buffer (because that's
the only way DMA works in confidential computing today).  For disks,
including boot, we cope with this by using an encrypted disk, so we
take the hardware protected page, encrypt it and place it in the clear
DMA buffer for disk write, meaning confidentiality and integrity are
preserved.  We'd have to add an encryption mechanism to pflash or
something to match this process.

> This suggests that the guest owner could boot the unified firmware
> image in a trusted guest environment first, and use UEFI-level tools
> to enroll various SB certificates. Then this modified image would be
> deployed every time to the untrusted cloud.

No, because of the read only nature of the NV store.  You could alter
it from outside using EDK tools, but you can't update it from inside. 
There's also the trust problem: in a current boot OVMF is provided by
the cloud service provider (this could be changed by re-engineering the
control plane, but it's the way it works today), we were thinking
having a single trusted OVMF provided by an outside entity (i.e. Red
Hat for the IBM cloud) would give higher confidence in the solution. 
There may be cases where customers insist on rolling their own, but the
hope is most of them would use a standard package.

> The AmdSevX64 platform could adopt a PlatformBootManagerLib instance
> where the TryRunningQemuKernel() call is reinstated, backed by the
> usual QemuLoadImageLib class APIs QemuLoadKernelImage() and
> QemuStartKernelImage().
> 
> edk2 offers two QemuLoadImageLib instances, GenericQemuLoadImageLib
> and X86QemuLoadImageLib. The former strictly enforces SB
> verification. That was in fact a *problem* for the traditional
> OvmfPkg platforms; please refer to commit 82808b422617 ("Revert
> "OvmfPkg: use generic QEMU image loader for secure boot enabled
> ..."", 2020-06-16). But the same rigor seems just right here, for the
> AmdSevX64 platform.
> 
> Where I see a gap in all this myself -- and of course there could be
> plenty other gaps that I just don't see -- is the varstore's
> protection from the hypervisor, once the guest is up and running. Can
> we discuss that perhaps?

I think it could possibly be done, but we're missing the encrypted
pflash, the measurement computation, and the kernel measuring initrd
and cmdline, all of which have to be controlled by the guest owner.

I did consider the secure boot variable paths, but given the complexity
explosion and all the missing pieces it looked like a bit of a non
starter compared to adding the hashes, which is fairly simple.

> If necessary, we could perhaps rework the AmdSevX64 platform to drop
> the pflash-backed variable driver stack, and use in-RAM (memory-only)
> variable emulation. Actual persistence / non-volatility of UEFI
> variables may not really be relevant for the remotely attested
> platform, but keeping all the variables in RAM would subject the
> varstore to memory encryption / protection. And perhaps we could
> integrate the enrollment of SB certificates into the *code* part of
> the firmware, with gRT->SetVariable() calls. (Normally this would be
> absolutely horrible, but for the remotely attested platform, anything
> goes.)
> 
> I simply dislike adding brand new code for a use case which at least
> *appears* to significantly overlap with that of Secure Boot. Secure
> Boot is about image verification, and it's rooted in tamper-resistant
> storage of certificates and/or image hashes. If we can figure out
> "tamper resistant" in the current context, we could perhaps reuse
> much of the existent SB infrastructure.

The problem even with sharing code paths is secure boot it about
authenticode hashes.  Most of what we're hashing for confidential
computing isn't even authenticode, so we'd have to ream the SecurityPkg
fairly comprehensively to get it to take either authenticode on the
image path or linear hashes for the other elements.  The way grub does
this is to have a separate verifier plugin for the measurements of any
opened file, but even in the grub case that does a linear hash of the
kernel rather than an authenticode one.

I'll let Dov answer the implementation details.

Regards,

James

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75995): https://edk2.groups.io/g/devel/message/75995
Mute This Topic: https://groups.io/mt/83074450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by Laszlo Ersek 2 years, 11 months ago

Hi James,

thanks for the answer, one comment below:

On 06/02/21 20:10, James Bottomley wrote:
> On Tue, 2021-06-01 at 14:11 +0200, Laszlo Ersek wrote:
>> Ard,
>>
>> I'll have a specific question for you below; please feel free to jump
>> forward (search for your name). Thanks.
>>
>> Dov, my comments below:
>>
>> On 05/25/21 07:31, Dov Murik wrote:
>>> Booting with SEV prevented the loading of kernel, initrd, and
>>> kernel command-line via QEMU fw_cfg interface because they arrive
>>> from the VMM which is untrusted in SEV.
>>>
>>> However, in some cases the kernel, initrd, and cmdline are not
>>> secret but should not be modified by the host.  In such a case, we
>>> want to verify inside the trusted VM that the kernel, initrd, and
>>> cmdline are indeed the ones expected by the Guest Owner, and only
>>> if that is the case go on and boot them up (removing the need for
>>> grub inside OVMF in that mode).
>>>
>>> This patch series declares a new page in MEMFD which will contain
>>> the hashes of these three blobs (kernel, initrd, cmdline), each
>>> under its own GUID entry.  This tables of hashes is populated by
>>> QEMU before launch, and encrypted as part of the initial VM memory;
>>> this makes sure theses hashes are part of the SEV measurement
>>> (which has to be approved by the Guest Owner for secret injection,
>>> for example).  Note that this requires a new QEMU patch which will
>>> be submitted soon.
>>>
>>> OVMF parses the table of hashes populated by QEMU (patch 5), and as
>>> it reads the fw_cfg blobs from QEMU, it will verify each one
>>> against the expected hash (kernel and initrd verifiers are
>>> introduced in patch 6, and command-line verifier is introduced in
>>> patches 7+8).  This is all done inside the trusted VM context.  If
>>> all the hashes are correct, boot of the kernel is allowed to continue.
>>>
>>> Any attempt by QEMU to modify the kernel, initrd, cmdline
>>> (including dropping one of them), or to modify the OVMF code that
>>> verifies those hashes, will cause the initial SEV measurement to
>>> change and therefore will be detectable by the Guest Owner during
>>> launch before secret injection.
>>
>> Please catch the error in my reasoning below.
>>
>> The goal is for the guest firmware to ensure the authenticity
>> (integrity) of kernel, initrd, cmdline.
> 
> That's right
> 
>> This is not really different from a normal Secure Boot setup, where
>> the guest firmware verifies the kernel image (presented as a UEFI
>> application, i.e. with the UEFI stub). It is up to the kernel to
>> verify the integrity of the initrd.
> 
> Right, but this doesn't happen today (there's no initrd measure in the
> kernel), so the only current choice you have is to combine the kernel
> and initrd then sign the whole combination, which gives one signature
> for both.
> 
>>  The command line is not particularly verified (as far as I know?), 
> 
> Right, it's not, which means secure boot can't replace the
> verification, because the command line is an essential thing to measure
> given the things it can do to the booting kernel.
> 
>> but if that's a problem, it should be solved even for bare metal
>> Secure Boot use cases. (Because, if the "root" user is compromised on
>> a running Linux system, they can modify the kernel params for next
>> boot in the grub config.)
> 
> The usual statement to this is that secure boot doesn't need to do this
> because the commandline is a measured boot problem, so grub measures
> its entire execution to PCR 8.  However, then you have to grope around
> in the measurement log and verify it via a TPM quote, which is
> incredibly sophisticated stuff compared to taking a single measurement
> hash.  Plus, we have no secure TPM currently for virtual machines, so
> there's no measured boot measurement the guest owner can trust.
> 
>> The AmdSevX64 platform uses a unified firmware image (executable +
>> varstore are presented as one blob, no separate CODE and VARS). There
>> is one pflash chip, and the initial guest-owner-side measurement
>> covers the whole blob, including the varstore.
> 
> That's right, except being part of a single rom volume, the varstore is
> read only.  This is a deliberate design choice: the absence of SMM and
> the fact that a R/W interface wouldn't get measured properly and also
> because NV  config changes can be used to effect secret leakage means
> it needs to be immutable.
> 
> If it's mutable, you need to store it separately, protect it with
> something like SMM and be aware of how the measurement would change
> when the store was updated ... which is another thing that's not that
> easy because of the way flash operates on overwrite.
> 
> Finally there's the secure write problem: the DMA for the variable
> write would have to go through an unencrypted buffer (because that's
> the only way DMA works in confidential computing today).  For disks,
> including boot, we cope with this by using an encrypted disk, so we
> take the hardware protected page, encrypt it and place it in the clear
> DMA buffer for disk write, meaning confidentiality and integrity are
> preserved.  We'd have to add an encryption mechanism to pflash or
> something to match this process.
> 
>> This suggests that the guest owner could boot the unified firmware
>> image in a trusted guest environment first, and use UEFI-level tools
>> to enroll various SB certificates. Then this modified image would be
>> deployed every time to the untrusted cloud.
> 
> No, because of the read only nature of the NV store.  You could alter
> it from outside using EDK tools, but you can't update it from inside. 
> There's also the trust problem: in a current boot OVMF is provided by
> the cloud service provider (this could be changed by re-engineering the
> control plane, but it's the way it works today), we were thinking
> having a single trusted OVMF provided by an outside entity (i.e. Red
> Hat for the IBM cloud) would give higher confidence in the solution. 
> There may be cases where customers insist on rolling their own, but the
> hope is most of them would use a standard package.

Here I meant, by "booting the unified firmware image in a trusted guest
environment first", that the guest owner would do that on their own
premises, not on an untrusted cloud. In other words, in that initial
run, they wouldn't use any kind of memory encryption. "Trusted
environment" meant that the guest owner would trust their own
environment not to compromise their image in any way. And in this
environment, they could use EnrollDefaultKeys.efi or some other
guest-side tool to pre-enroll certificates.

In the actual production (cloud) environment, read-only access to the
varstore would be sufficient (with the certificates already existing in it).

But, anyway, the rest of your explanation is convincing; and I agree the
general idea of this series is fine. We just need to sort out the
technical details / solution structure, per the second half of my review.

Thanks!
Laszlo


> 
>> The AmdSevX64 platform could adopt a PlatformBootManagerLib instance
>> where the TryRunningQemuKernel() call is reinstated, backed by the
>> usual QemuLoadImageLib class APIs QemuLoadKernelImage() and
>> QemuStartKernelImage().
>>
>> edk2 offers two QemuLoadImageLib instances, GenericQemuLoadImageLib
>> and X86QemuLoadImageLib. The former strictly enforces SB
>> verification. That was in fact a *problem* for the traditional
>> OvmfPkg platforms; please refer to commit 82808b422617 ("Revert
>> "OvmfPkg: use generic QEMU image loader for secure boot enabled
>> ..."", 2020-06-16). But the same rigor seems just right here, for the
>> AmdSevX64 platform.
>>
>> Where I see a gap in all this myself -- and of course there could be
>> plenty other gaps that I just don't see -- is the varstore's
>> protection from the hypervisor, once the guest is up and running. Can
>> we discuss that perhaps?
> 
> I think it could possibly be done, but we're missing the encrypted
> pflash, the measurement computation, and the kernel measuring initrd
> and cmdline, all of which have to be controlled by the guest owner.
> 
> I did consider the secure boot variable paths, but given the complexity
> explosion and all the missing pieces it looked like a bit of a non
> starter compared to adding the hashes, which is fairly simple.
> 
>> If necessary, we could perhaps rework the AmdSevX64 platform to drop
>> the pflash-backed variable driver stack, and use in-RAM (memory-only)
>> variable emulation. Actual persistence / non-volatility of UEFI
>> variables may not really be relevant for the remotely attested
>> platform, but keeping all the variables in RAM would subject the
>> varstore to memory encryption / protection. And perhaps we could
>> integrate the enrollment of SB certificates into the *code* part of
>> the firmware, with gRT->SetVariable() calls. (Normally this would be
>> absolutely horrible, but for the remotely attested platform, anything
>> goes.)
>>
>> I simply dislike adding brand new code for a use case which at least
>> *appears* to significantly overlap with that of Secure Boot. Secure
>> Boot is about image verification, and it's rooted in tamper-resistant
>> storage of certificates and/or image hashes. If we can figure out
>> "tamper resistant" in the current context, we could perhaps reuse
>> much of the existent SB infrastructure.
> 
> The problem even with sharing code paths is secure boot it about
> authenticode hashes.  Most of what we're hashing for confidential
> computing isn't even authenticode, so we'd have to ream the SecurityPkg
> fairly comprehensively to get it to take either authenticode on the
> image path or linear hashes for the other elements.  The way grub does
> this is to have a separate verifier plugin for the measurements of any
> opened file, but even in the grub case that does a linear hash of the
> kernel rather than an authenticode one.
> 
> I'll let Dov answer the implementation details.
> 
> Regards,
> 
> James
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76016): https://edk2.groups.io/g/devel/message/76016
Mute This Topic: https://groups.io/mt/83074450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by Dov Murik 2 years, 11 months ago

Thank you Laszlo for reviewing this.


On 01/06/2021 15:11, Laszlo Ersek wrote:
> Ard,
> 
> I'll have a specific question for you below; please feel free to jump
> forward (search for your name). Thanks.
> 
> Dov, my comments below:
> 
> On 05/25/21 07:31, Dov Murik wrote:
>> Booting with SEV prevented the loading of kernel, initrd, and kernel
>> command-line via QEMU fw_cfg interface because they arrive from the VMM
>> which is untrusted in SEV.
>>
>> However, in some cases the kernel, initrd, and cmdline are not secret
>> but should not be modified by the host.  In such a case, we want to
>> verify inside the trusted VM that the kernel, initrd, and cmdline are
>> indeed the ones expected by the Guest Owner, and only if that is the
>> case go on and boot them up (removing the need for grub inside OVMF in
>> that mode).
>>
>> This patch series declares a new page in MEMFD which will contain the
>> hashes of these three blobs (kernel, initrd, cmdline), each under its
>> own GUID entry.  This tables of hashes is populated by QEMU before
>> launch, and encrypted as part of the initial VM memory; this makes sure
>> theses hashes are part of the SEV measurement (which has to be approved
>> by the Guest Owner for secret injection, for example).  Note that this
>> requires a new QEMU patch which will be submitted soon.
>>
>> OVMF parses the table of hashes populated by QEMU (patch 5), and as it
>> reads the fw_cfg blobs from QEMU, it will verify each one against the
>> expected hash (kernel and initrd verifiers are introduced in patch 6,
>> and command-line verifier is introduced in patches 7+8).  This is all
>> done inside the trusted VM context.  If all the hashes are correct, boot
>> of the kernel is allowed to continue.
>>
>> Any attempt by QEMU to modify the kernel, initrd, cmdline (including
>> dropping one of them), or to modify the OVMF code that verifies those
>> hashes, will cause the initial SEV measurement to change and therefore
>> will be detectable by the Guest Owner during launch before secret
>> injection.
> 
> Please catch the error in my reasoning below.
> 
> The goal is for the guest firmware to ensure the authenticity
> (integrity) of kernel, initrd, cmdline.
> 
> This is not really different from a normal Secure Boot setup, where the
> guest firmware verifies the kernel image (presented as a UEFI
> application, i.e. with the UEFI stub). It is up to the kernel to verify
> the integrity of the initrd. The command line is not particularly
> verified (as far as I know?), but if that's a problem, it should be
> solved even for bare metal Secure Boot use cases. (Because, if the
> "root" user is compromised on a running Linux system, they can modify
> the kernel params for next boot in the grub config.)

James explained the difference from Secure Boot setup and our choice of
hash validation of kernel+initrd+cmdline.


[Skipping...]


> 
> ----*----
> 
> Considering the particular approach in this set:
> 
> - To reiterate Brijesh's point, I feel a new MEMFD page is wasteful. If
> we really need the MEMFD approach, I'd *really* like us to extend one of
> the existent structures. If necessary, introduce a new GUID, for a table
> that contains both previously injected data, and the new data. If all
> that's impossible or too awkward, please document why.
> 

Brijesh's approach mandates the guest owner use of SEV secret injection,
because the approved hashes are injected into the secrets page at
launch.  There are two disadvantages for this approach:

(a) It requires the use of SEV's LAUNCH_SECRET during launch, thus tying
together measurement of approved boot binaries and the secrets (which in
this case will be used by later kernel or userspace processes).  Note
also that the hashes are not confidential (in fact, the host has access
to the full kernel+initrd).

(b) AMD SEV-SNP doesn't support LAUNCH_SECRET any more; instead, in SNP
(/me hand-waving) there's a mechanism to securely verify measurement
later (e.g. during initrd) and if measurement matches then there's a
secure way to retrieve secrets.

My hope is that the approach we proposed (QEMU is building a "hashes
page" which is measured at launch) will be useful also for
secure-booting SNP (and TDX?) guests.  The idea is that in SNP the
initial encrypted memory will include OVMF and the hahses page (as in
SEV); this will be measured at launch and saved by SNP; at a later step,
a userspace process requests the initial measurement from the PSP
hardware (in a secure way - that's only possible in SNP); if that
measurement matches then the userspace process may request some secrets.
 That said, I'm only beginning to learn about these new generations.

So I argue to keep the existing approach with two separate areas:
existing one for injected secrets, and new one for a table of approved
hashes (filled by QEMU and updated as initial encrypted measured guest
memory).

If the issue is MEMFD space, maybe we can do something like: use the
existing secrets page (4KB) for two uses: first 3KB for secrets, and
last 1KB for hashes.  If this is not enough, the hashes are even smaller
than 1KB; and we can even publish only one hash - the hash of all 3
hashes (need to think about edge cases when there's no cmdline/initrd).
 But all these "solutions" feel a bit hacky for me and might complicate
the code.

I don't understand your suggestion: "I'd *really* like us to extend one
of the existent structures. If necessary, introduce a new GUID, for a
table that contains both previously injected data, and the new data.";
does this mean to use a single MEMFD page for the injected secrets and
the hashes?

Also, in general, I don't really understand the implications of running
out of MEMFD place; maybe you have other ideas around this (for example,
can we make MEMFD bigger only for AmdSevX64 platform?).


> - Modifying the QemuFwCfgLib class for this purpose is inappropriate.
> Even if we do our own home-brewed verifier, none of it must go into
> QemuFwCfgLib class. QemuFwCfgLib is for transport.
> 

OK, we'll take the verifier out (as you suggested below - to a
BlobVerifierLib with two implementations).


> [Ard, please see this one question:]
> 
> - A major complication for hashing all three of: kernel, initrd,
> cmdline, is that the *fetching* of this triplet is split between two
> places. (Well, it is split between *three* places in fact, but I'm going
> to ignore LinuxInitrdDynamicShellCommand for now, because the AmdSevX64
> platform sets BUILD_SHELL to FALSE for production.)
> 
> The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but the
> command line is fetched in (both) QemuLoadImageLib instances. This
> requires that all these modules be littered with hashing as well, which
> I find *really bad*. Even if we factor out the actual logic, I strongly
> dislike having *just hooks* for hashing in multiple modules.
> 
> Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe: don't
> expose kernel command line", 2020-03-05). If we first
> 
> (a) reverted that commit, and
> 
> (b) modified *both* QemuLoadImageLib instances, to load the kernel
> command line from the *synthetic filesystem* (rather than directly from
> fw_cfg),
> 
> then we could centralize the hashing to just QemuKernelLoaderFsDxe.
> 
> Ard -- what's your thought on this?
> 

I understand there's agreement here, and that both this suggested change
(use the synthetic filesystem) and my patch series (add hash
verification) touch the same code areas.  How do you envision this
process in the mailing list?  Seperate patch serieses with dependency?
One long patch series with both changes?  What goes first?


> 
> And then, we could eliminate the dynamic callback registration, plus the
> separate SevFwCfgVerifier, SevHashFinderLib, and SevQemuLoadImageLib stuff.
> 
> We'd only need one new lib class, with *statically linked* hooks for
> QemuKernelLoaderFsDxe, and two instances of this new class, a Null one,
> and an actual (SEV hash verifier) one. The latter instance would locate
> the hash values, calculate the fresh hashes, and perform the
> comparisons. Only the AmdSevX64 platform would use the non-Null instance
> of this library class.

OK, I'll refactor to static linking with two BlobVerifierLib imlementations.


> 
> (NB QemuKernelLoaderFsDxe is used by some ArmVirtPkg platforms, so
> resolutions to the Null instance would be required there too.)

I'll need to learn how to build edk2 for Arm to test this.  Thanks for
the heads-up.


-Dov


> 
> Thanks
> Laszlo
> 
> 
> 
>>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Ashish Kalra <ashish.kalra@amd.com>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Cc: Erdem Aktas <erdemaktas@google.com>
>> Cc: James Bottomley <jejb@linux.ibm.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Min Xu <min.m.xu@intel.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> James Bottomley (8):
>>   OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming
>>   OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg
>>   OvmfPkg/AmdSev: add a page to the MEMFD for firmware config hashes
>>   OvmfPkg/QemuKernelLoaderFsDxe: Add ability to verify loaded items
>>   OvmfPkg/AmdSev: Add library to find encrypted hashes for the FwCfg
>>     device
>>   OvmfPkg/AmdSev: Add firmware file plugin to verifier
>>   OvmfPkg: GenericQemuLoadImageLib: Allow verifying fw_cfg command line
>>   OvmfPkg/AmdSev: add SevQemuLoadImageLib
>>
>>  OvmfPkg/OvmfPkg.dec                                                       |  10 ++
>>  OvmfPkg/AmdSev/AmdSevX64.dsc                                              |   9 +-
>>  OvmfPkg/AmdSev/AmdSevX64.fdf                                              |   3 +
>>  OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf              |  30 +++++
>>  OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf              |  34 ++++++
>>  OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf        |  30 +++++
>>  OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf |   2 +
>>  OvmfPkg/ResetVector/ResetVector.inf                                       |   2 +
>>  OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h                         |  47 ++++++++
>>  OvmfPkg/Include/Library/QemuFwCfgLib.h                                    |  35 ++++++
>>  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h                  |  11 ++
>>  OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c                |  60 ++++++++++
>>  OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c                | 126 ++++++++++++++++++++
>>  OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c          |  52 ++++++++
>>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.c                                      |   2 +-
>>  OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c         |  29 +++++
>>  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c                  |   5 +
>>  OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c                   |  50 ++++++++
>>  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c                     |  31 +++++
>>  OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm                              |  20 ++++
>>  OvmfPkg/ResetVector/ResetVector.nasmb                                     |   2 +
>>  21 files changed, 587 insertions(+), 3 deletions(-)
>>  create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf
>>  create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf
>>  create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf
>>  create mode 100644 OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h
>>  create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c
>>  create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c
>>  create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c
>>  create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c
>>
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76067): https://edk2.groups.io/g/devel/message/76067
Mute This Topic: https://groups.io/mt/83074450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by Laszlo Ersek 2 years, 11 months ago

On 06/04/21 12:30, Dov Murik wrote:

> So I argue to keep the existing approach with two separate areas:
> existing one for injected secrets, and new one for a table of approved
> hashes (filled by QEMU and updated as initial encrypted measured guest
> memory).

OK.

> If the issue is MEMFD space,

Yes, that's it.

> maybe we can do something like: use the
> existing secrets page (4KB) for two uses: first 3KB for secrets, and
> last 1KB for hashes.  If this is not enough, the hashes are even
> smaller than 1KB; and we can even publish only one hash - the hash of
> all 3 hashes (need to think about edge cases when there's no
> cmdline/initrd). But all these "solutions" feel a bit hacky for me and
> might complicate the code.

All these PCDs come in pairs -- base and size. (IIRC.) If there's no
architectural requirement to keep these two kinds of info in different
pages (such as different page protections or whatever), then packing
them into a single page is something I'd like. The above 3K+1K
subdivision sounds OK to me.

>
> I don't understand your suggestion: "I'd *really* like us to extend
> one of the existent structures. If necessary, introduce a new GUID,
> for a table that contains both previously injected data, and the new
> data."; does this mean to use a single MEMFD page for the injected
> secrets and the hashes?

Yes, it's the same (say, 3K+1K) idea, just expressed differently. In one
case, you have two GUIDed structs in the (plaintext, not compressed)
reset vector in the pflash, and the base+size structures associated wth
those two separate GUIDs happen to identify distinct ranges of the same
MEMFD page. In the other case, you have just one GUIDed structure (with
base+size contents), and the page pointed-to by this base+size pair is
subdivided by *internal* structuring -- such as internal GUIDs and so
on. Whichever is simpler to implement in both QEMU and edk2; I just want
to avoid wasing a full page for three hashes.

>
> Also, in general, I don't really understand the implications of
> running out of MEMFD place;

Here's one implication of enlarging MEMFD. It pushes BS Code, BS Data,
Loader Code, Loader Data, perhaps some AcpiNVS and Reserved memory
allocations to higher addresses. Then when the kernel is loaded, its
load address may be higher too. I'm not worried about wasted guest
memory, but abut various silent assumptions as to where the kernel
"should be". For example, after one round of enlarging DXEFV, the
"crash" utility stopped opening guest memory dumps, because it couldn't
find a kernel signature in the (low) address range that it used to scan.
The fix wasn't too difficult (the range to scan could be specified on
the "crash" commadn line, and then my colleague Dave Anderson just
modified "crash"), but it was a *surprise*. I don't like those.

> maybe you have other ideas around this (for example,
> can we make MEMFD bigger only for AmdSevX64 platform?).

Yes, experimenting with a larger MEMFD in just the AmdSevX64 platform is
fine.

NB reordering various PCDs between each other, so that their relative
relationships (orders) change, is a *lot* more risky than just enlarging
existing areas. The code in OVMF tends not to rely on actual bases and
sizes, but it may very well rely on a particular BasePCD + SizePCD sum
not exceeding another particular BasePCD.

>
>
>> - Modifying the QemuFwCfgLib class for this purpose is inappropriate.
>> Even if we do our own home-brewed verifier, none of it must go into
>> QemuFwCfgLib class. QemuFwCfgLib is for transport.
>>
>
> OK, we'll take the verifier out (as you suggested below - to a
> BlobVerifierLib with two implementations).
>
>
>> [Ard, please see this one question:]
>>
>> - A major complication for hashing all three of: kernel, initrd,
>> cmdline, is that the *fetching* of this triplet is split between two
>> places. (Well, it is split between *three* places in fact, but I'm
>> going to ignore LinuxInitrdDynamicShellCommand for now, because the
>> AmdSevX64 platform sets BUILD_SHELL to FALSE for production.)
>>
>> The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but
>> the command line is fetched in (both) QemuLoadImageLib instances.
>> This requires that all these modules be littered with hashing as
>> well, which I find *really bad*. Even if we factor out the actual
>> logic, I strongly dislike having *just hooks* for hashing in multiple
>> modules.
>>
>> Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe:
>> don't expose kernel command line", 2020-03-05). If we first
>>
>> (a) reverted that commit, and
>>
>> (b) modified *both* QemuLoadImageLib instances, to load the kernel
>> command line from the *synthetic filesystem* (rather than directly
>> from fw_cfg),
>>
>> then we could centralize the hashing to just QemuKernelLoaderFsDxe.
>>
>> Ard -- what's your thought on this?
>>
>
> I understand there's agreement here, and that both this suggested
> change (use the synthetic filesystem) and my patch series (add hash
> verification) touch the same code areas.  How do you envision this
> process in the mailing list?  Seperate patch serieses with dependency?
> One long patch series with both changes?  What goes first?

Good point. I do have a kind of patch order laid out in my mind, but I
didn't think of whether we should have the patches in one patch series,
or in two "waves".

OK, let's go with two patch sets.

In the first set, we should just focus on the above steps (a) and (b).
Step (a) shouldn't be too hard. In step (b), you'd modify both
QemuLoadImageLib instances (two separate patches), replacing the
QemuFwCfgLib APIs for fetching the cmdline with
EFI_SIMPLE_FILE_SYSTEM_PROTOCOL and EFI_FILE_PROTOCOL APIs.

Speaking from memory, the synthetic filesystem has a unique device path,
so the first step would be calling gBS->LocateDevicePath(), for finding
SimpleFs on the unique device path. Once you have the SimpleFs
interface, you can call OpenVolume, then open the "cmdline" file using
the EFI_FILE_PROTOCOL output by OpenVolume.

Once we merge this series (basically just three patches), there is no
QemuFwCfgLib dependency left in either QemuLoadImageLib instance, I
reckon. Then you can post the second wave, in which:

- a new "firmware config verifier" library class is introduced,

- two library instances for that class are introduced (null, and the
  real thing),

- the AmdSevX64.dsc platform resolves the new lib class to the "real"
  (hashing) instance,

- all other platform DSCs using QemuKernelLoaderFsDxe resolve the new
  lib class to the null instance,

- QemuKernelLoaderFsDxe is extended with a dependency on the new class,
  calling the proper APIs to (a) initialize the verifier, and (b) verify
  every fw_cfg blob that is about to be exposed as a synthetic file.

Then QemuLoadImageLib needs no changes, as it will not depend on fw_cfg,
and every synthetic file it may want to access will have been verified
by QemuKernelLoaderFsDxe already, according to the verifier lib instance
that's used in the respective platform DSC file.

I would recommend only posting the first patch set initially. It has a
very well defined goal (--> hide the fw_cfg dependency in both
QemuLoadImageLib instances behind the synthetic filesystem); we can
validate / review that regardless of the ultimate crypto / security
goal. Using the SimpleFs / FILE protocol APIs is not trivial IMO, so
it's possible that just the first wave will require a v2.

>
>
>>
>> And then, we could eliminate the dynamic callback registration, plus
>> the separate SevFwCfgVerifier, SevHashFinderLib, and
>> SevQemuLoadImageLib stuff.
>>
>> We'd only need one new lib class, with *statically linked* hooks for
>> QemuKernelLoaderFsDxe, and two instances of this new class, a Null
>> one, and an actual (SEV hash verifier) one. The latter instance would
>> locate the hash values, calculate the fresh hashes, and perform the
>> comparisons. Only the AmdSevX64 platform would use the non-Null
>> instance of this library class.
>
> OK, I'll refactor to static linking with two BlobVerifierLib
> imlementations.
>
>
>>
>> (NB QemuKernelLoaderFsDxe is used by some ArmVirtPkg platforms, so
>> resolutions to the Null instance would be required there too.)
>
> I'll need to learn how to build edk2 for Arm to test this.  Thanks for
> the heads-up.

With regard to QemuKernelLoaderFsDxe specifically:

  build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemu.dsc               -a AARCH64
  build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemu.dsc       -a ARM
  build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemuKernel.dsc         -a AARCH64
  build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemuKernel.dsc -a ARM

If you work on an x86_64 machine, you'll need cross-gcc and
cross-binutils for this. I have the following packages installed on my
laptop:

  binutils-aarch64-linux-gnu-2.31.1-3.el7.x86_64
  binutils-arm-linux-gnu-2.31.1-3.el7.x86_64
  cross-binutils-common-2.31.1-3.el7.noarch

  cross-gcc-common-9.2.1-3.el7.1.noarch
  gcc-aarch64-linux-gnu-9.2.1-3.el7.1.x86_64
  gcc-arm-linux-gnu-9.2.1-3.el7.1.x86_64
  gcc-c++-aarch64-linux-gnu-9.2.1-3.el7.1.x86_64
  gcc-c++-arm-linux-gnu-9.2.1-3.el7.1.x86_64

(I don't remember why I have the c++ cross-compiler installed.)

Thanks
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76069): https://edk2.groups.io/g/devel/message/76069
Mute This Topic: https://groups.io/mt/83074450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by Dov Murik 2 years, 11 months ago


On 04/06/2021 14:26, Laszlo Ersek wrote:
> On 06/04/21 12:30, Dov Murik wrote:
> 
>> So I argue to keep the existing approach with two separate areas:
>> existing one for injected secrets, and new one for a table of approved
>> hashes (filled by QEMU and updated as initial encrypted measured guest
>> memory).
> 
> OK.
> 
>> If the issue is MEMFD space,
> 
> Yes, that's it.
> 
>> maybe we can do something like: use the
>> existing secrets page (4KB) for two uses: first 3KB for secrets, and
>> last 1KB for hashes.  If this is not enough, the hashes are even
>> smaller than 1KB; and we can even publish only one hash - the hash of
>> all 3 hashes (need to think about edge cases when there's no
>> cmdline/initrd). But all these "solutions" feel a bit hacky for me and
>> might complicate the code.
> 
> All these PCDs come in pairs -- base and size. (IIRC.) If there's no
> architectural requirement to keep these two kinds of info in different
> pages (such as different page protections or whatever), then packing
> them into a single page is something I'd like. The above 3K+1K
> subdivision sounds OK to me.
> 

I'll go with 3KB secrets + 1KB hashes.


>>
>> I don't understand your suggestion: "I'd *really* like us to extend
>> one of the existent structures. If necessary, introduce a new GUID,
>> for a table that contains both previously injected data, and the new
>> data."; does this mean to use a single MEMFD page for the injected
>> secrets and the hashes?
> 
> Yes, it's the same (say, 3K+1K) idea, just expressed differently. In one
> case, you have two GUIDed structs in the (plaintext, not compressed)
> reset vector in the pflash, and the base+size structures associated wth
> those two separate GUIDs happen to identify distinct ranges of the same
> MEMFD page. In the other case, you have just one GUIDed structure (with
> base+size contents), and the page pointed-to by this base+size pair is
> subdivided by *internal* structuring -- such as internal GUIDs and so
> on. Whichever is simpler to implement in both QEMU and edk2; I just want
> to avoid wasing a full page for three hashes.
> 

I'll go with the two GUIDed structures in the reset vector (which will
point to distinct parts of a single 4KB page).

That actually means shortening the existing secrets MEMFD area from 4KB
to 3KB. Is that OK?



>>
>> Also, in general, I don't really understand the implications of
>> running out of MEMFD place;
> 
> Here's one implication of enlarging MEMFD. It pushes BS Code, BS Data,
> Loader Code, Loader Data, perhaps some AcpiNVS and Reserved memory
> allocations to higher addresses. Then when the kernel is loaded, its
> load address may be higher too. I'm not worried about wasted guest
> memory, but abut various silent assumptions as to where the kernel
> "should be". For example, after one round of enlarging DXEFV, the
> "crash" utility stopped opening guest memory dumps, because it couldn't
> find a kernel signature in the (low) address range that it used to scan.
> The fix wasn't too difficult (the range to scan could be specified on
> the "crash" commadn line, and then my colleague Dave Anderson just
> modified "crash"), but it was a *surprise*. I don't like those.
> 
>> maybe you have other ideas around this (for example,
>> can we make MEMFD bigger only for AmdSevX64 platform?).
> 
> Yes, experimenting with a larger MEMFD in just the AmdSevX64 platform is
> fine.
> 

But now I understand that failures can appear way later in userspace
(the crash utility), so just testing that a modern AMD VM boots fine is
not enough to get confidence here.


> NB reordering various PCDs between each other, so that their relative
> relationships (orders) change, is a *lot* more risky than just enlarging
> existing areas. The code in OVMF tends not to rely on actual bases and
> sizes, but it may very well rely on a particular BasePCD + SizePCD sum
> not exceeding another particular BasePCD.
> 

Thanks for pointing this out. I'll avoid reordering.


>>
>>
>>> - Modifying the QemuFwCfgLib class for this purpose is inappropriate.
>>> Even if we do our own home-brewed verifier, none of it must go into
>>> QemuFwCfgLib class. QemuFwCfgLib is for transport.
>>>
>>
>> OK, we'll take the verifier out (as you suggested below - to a
>> BlobVerifierLib with two implementations).
>>
>>
>>> [Ard, please see this one question:]
>>>
>>> - A major complication for hashing all three of: kernel, initrd,
>>> cmdline, is that the *fetching* of this triplet is split between two
>>> places. (Well, it is split between *three* places in fact, but I'm
>>> going to ignore LinuxInitrdDynamicShellCommand for now, because the
>>> AmdSevX64 platform sets BUILD_SHELL to FALSE for production.)
>>>
>>> The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but
>>> the command line is fetched in (both) QemuLoadImageLib instances.
>>> This requires that all these modules be littered with hashing as
>>> well, which I find *really bad*. Even if we factor out the actual
>>> logic, I strongly dislike having *just hooks* for hashing in multiple
>>> modules.
>>>
>>> Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe:
>>> don't expose kernel command line", 2020-03-05). If we first
>>>
>>> (a) reverted that commit, and
>>>
>>> (b) modified *both* QemuLoadImageLib instances, to load the kernel
>>> command line from the *synthetic filesystem* (rather than directly
>>> from fw_cfg),
>>>
>>> then we could centralize the hashing to just QemuKernelLoaderFsDxe.
>>>
>>> Ard -- what's your thought on this?
>>>
>>
>> I understand there's agreement here, and that both this suggested
>> change (use the synthetic filesystem) and my patch series (add hash
>> verification) touch the same code areas.  How do you envision this
>> process in the mailing list?  Seperate patch serieses with dependency?
>> One long patch series with both changes?  What goes first?
> 
> Good point. I do have a kind of patch order laid out in my mind, but I
> didn't think of whether we should have the patches in one patch series,
> or in two "waves".
> 
> OK, let's go with two patch sets.
> 
> In the first set, we should just focus on the above steps (a) and (b).
> Step (a) shouldn't be too hard. In step (b), you'd modify both
> QemuLoadImageLib instances (two separate patches), replacing the
> QemuFwCfgLib APIs for fetching the cmdline with
> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL and EFI_FILE_PROTOCOL APIs.
> 
> Speaking from memory, the synthetic filesystem has a unique device path,
> so the first step would be calling gBS->LocateDevicePath(), for finding
> SimpleFs on the unique device path. Once you have the SimpleFs
> interface, you can call OpenVolume, then open the "cmdline" file using
> the EFI_FILE_PROTOCOL output by OpenVolume.
> 
> Once we merge this series (basically just three patches), there is no
> QemuFwCfgLib dependency left in either QemuLoadImageLib instance, I
> reckon. Then you can post the second wave, in which:
> 
> - a new "firmware config verifier" library class is introduced,
> 
> - two library instances for that class are introduced (null, and the
>   real thing),
> 
> - the AmdSevX64.dsc platform resolves the new lib class to the "real"
>   (hashing) instance,
> 
> - all other platform DSCs using QemuKernelLoaderFsDxe resolve the new
>   lib class to the null instance,
> 
> - QemuKernelLoaderFsDxe is extended with a dependency on the new class,
>   calling the proper APIs to (a) initialize the verifier, and (b) verify
>   every fw_cfg blob that is about to be exposed as a synthetic file.
> 
> Then QemuLoadImageLib needs no changes, as it will not depend on fw_cfg,
> and every synthetic file it may want to access will have been verified
> by QemuKernelLoaderFsDxe already, according to the verifier lib instance
> that's used in the respective platform DSC file.
> 
> I would recommend only posting the first patch set initially. It has a
> very well defined goal (--> hide the fw_cfg dependency in both
> QemuLoadImageLib instances behind the synthetic filesystem); we can
> validate / review that regardless of the ultimate crypto / security
> goal. Using the SimpleFs / FILE protocol APIs is not trivial IMO, so
> it's possible that just the first wave will require a v2.
> 

OK, I'll try to follow this plan.

>>
>>
>>>
>>> And then, we could eliminate the dynamic callback registration, plus
>>> the separate SevFwCfgVerifier, SevHashFinderLib, and
>>> SevQemuLoadImageLib stuff.
>>>
>>> We'd only need one new lib class, with *statically linked* hooks for
>>> QemuKernelLoaderFsDxe, and two instances of this new class, a Null
>>> one, and an actual (SEV hash verifier) one. The latter instance would
>>> locate the hash values, calculate the fresh hashes, and perform the
>>> comparisons. Only the AmdSevX64 platform would use the non-Null
>>> instance of this library class.
>>
>> OK, I'll refactor to static linking with two BlobVerifierLib
>> imlementations.
>>
>>
>>>
>>> (NB QemuKernelLoaderFsDxe is used by some ArmVirtPkg platforms, so
>>> resolutions to the Null instance would be required there too.)
>>
>> I'll need to learn how to build edk2 for Arm to test this.  Thanks for
>> the heads-up.
> 
> With regard to QemuKernelLoaderFsDxe specifically:
> 
>   build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemu.dsc               -a AARCH64
>   build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemu.dsc       -a ARM
>   build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemuKernel.dsc         -a AARCH64
>   build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemuKernel.dsc -a ARM
> 
> If you work on an x86_64 machine, you'll need cross-gcc and
> cross-binutils for this. I have the following packages installed on my
> laptop:
> 
>   binutils-aarch64-linux-gnu-2.31.1-3.el7.x86_64
>   binutils-arm-linux-gnu-2.31.1-3.el7.x86_64
>   cross-binutils-common-2.31.1-3.el7.noarch
> 
>   cross-gcc-common-9.2.1-3.el7.1.noarch
>   gcc-aarch64-linux-gnu-9.2.1-3.el7.1.x86_64
>   gcc-arm-linux-gnu-9.2.1-3.el7.1.x86_64
>   gcc-c++-aarch64-linux-gnu-9.2.1-3.el7.1.x86_64
>   gcc-c++-arm-linux-gnu-9.2.1-3.el7.1.x86_64
> 
> (I don't remember why I have the c++ cross-compiler installed.)
> 

Thanks for the details; I'll try it.

-Dov


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76111): https://edk2.groups.io/g/devel/message/76111
Mute This Topic: https://groups.io/mt/83074450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by Laszlo Ersek 2 years, 11 months ago

On 06/06/21 15:21, Dov Murik wrote:
> 
> 
> On 04/06/2021 14:26, Laszlo Ersek wrote:
>> On 06/04/21 12:30, Dov Murik wrote:
>>
>>> So I argue to keep the existing approach with two separate areas:
>>> existing one for injected secrets, and new one for a table of approved
>>> hashes (filled by QEMU and updated as initial encrypted measured guest
>>> memory).
>>
>> OK.
>>
>>> If the issue is MEMFD space,
>>
>> Yes, that's it.
>>
>>> maybe we can do something like: use the
>>> existing secrets page (4KB) for two uses: first 3KB for secrets, and
>>> last 1KB for hashes.  If this is not enough, the hashes are even
>>> smaller than 1KB; and we can even publish only one hash - the hash of
>>> all 3 hashes (need to think about edge cases when there's no
>>> cmdline/initrd). But all these "solutions" feel a bit hacky for me and
>>> might complicate the code.
>>
>> All these PCDs come in pairs -- base and size. (IIRC.) If there's no
>> architectural requirement to keep these two kinds of info in different
>> pages (such as different page protections or whatever), then packing
>> them into a single page is something I'd like. The above 3K+1K
>> subdivision sounds OK to me.
>>
> 
> I'll go with 3KB secrets + 1KB hashes.
> 
> 
>>>
>>> I don't understand your suggestion: "I'd *really* like us to extend
>>> one of the existent structures. If necessary, introduce a new GUID,
>>> for a table that contains both previously injected data, and the new
>>> data."; does this mean to use a single MEMFD page for the injected
>>> secrets and the hashes?
>>
>> Yes, it's the same (say, 3K+1K) idea, just expressed differently. In one
>> case, you have two GUIDed structs in the (plaintext, not compressed)
>> reset vector in the pflash, and the base+size structures associated wth
>> those two separate GUIDs happen to identify distinct ranges of the same
>> MEMFD page. In the other case, you have just one GUIDed structure (with
>> base+size contents), and the page pointed-to by this base+size pair is
>> subdivided by *internal* structuring -- such as internal GUIDs and so
>> on. Whichever is simpler to implement in both QEMU and edk2; I just want
>> to avoid wasing a full page for three hashes.
>>
> 
> I'll go with the two GUIDed structures in the reset vector (which will
> point to distinct parts of a single 4KB page).
> 
> That actually means shortening the existing secrets MEMFD area from 4KB
> to 3KB. Is that OK?

I don't know how that area is used in practice; from my perspective,
shortening it to 3KB is OK.

> 
> 
> 
>>>
>>> Also, in general, I don't really understand the implications of
>>> running out of MEMFD place;
>>
>> Here's one implication of enlarging MEMFD. It pushes BS Code, BS Data,
>> Loader Code, Loader Data, perhaps some AcpiNVS and Reserved memory
>> allocations to higher addresses. Then when the kernel is loaded, its
>> load address may be higher too. I'm not worried about wasted guest
>> memory, but abut various silent assumptions as to where the kernel
>> "should be". For example, after one round of enlarging DXEFV, the
>> "crash" utility stopped opening guest memory dumps, because it couldn't
>> find a kernel signature in the (low) address range that it used to scan.
>> The fix wasn't too difficult (the range to scan could be specified on
>> the "crash" commadn line, and then my colleague Dave Anderson just
>> modified "crash"), but it was a *surprise*. I don't like those.
>>
>>> maybe you have other ideas around this (for example,
>>> can we make MEMFD bigger only for AmdSevX64 platform?).
>>
>> Yes, experimenting with a larger MEMFD in just the AmdSevX64 platform is
>> fine.
>>
> 
> But now I understand that failures can appear way later in userspace
> (the crash utility), so just testing that a modern AMD VM boots fine is
> not enough to get confidence here.

Indeed if you expect the same userspace to work seamlessly, there is a
risk.

Cheers
Laszlo

> 
> 
>> NB reordering various PCDs between each other, so that their relative
>> relationships (orders) change, is a *lot* more risky than just enlarging
>> existing areas. The code in OVMF tends not to rely on actual bases and
>> sizes, but it may very well rely on a particular BasePCD + SizePCD sum
>> not exceeding another particular BasePCD.
>>
> 
> Thanks for pointing this out. I'll avoid reordering.
> 
> 
>>>
>>>
>>>> - Modifying the QemuFwCfgLib class for this purpose is inappropriate.
>>>> Even if we do our own home-brewed verifier, none of it must go into
>>>> QemuFwCfgLib class. QemuFwCfgLib is for transport.
>>>>
>>>
>>> OK, we'll take the verifier out (as you suggested below - to a
>>> BlobVerifierLib with two implementations).
>>>
>>>
>>>> [Ard, please see this one question:]
>>>>
>>>> - A major complication for hashing all three of: kernel, initrd,
>>>> cmdline, is that the *fetching* of this triplet is split between two
>>>> places. (Well, it is split between *three* places in fact, but I'm
>>>> going to ignore LinuxInitrdDynamicShellCommand for now, because the
>>>> AmdSevX64 platform sets BUILD_SHELL to FALSE for production.)
>>>>
>>>> The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but
>>>> the command line is fetched in (both) QemuLoadImageLib instances.
>>>> This requires that all these modules be littered with hashing as
>>>> well, which I find *really bad*. Even if we factor out the actual
>>>> logic, I strongly dislike having *just hooks* for hashing in multiple
>>>> modules.
>>>>
>>>> Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe:
>>>> don't expose kernel command line", 2020-03-05). If we first
>>>>
>>>> (a) reverted that commit, and
>>>>
>>>> (b) modified *both* QemuLoadImageLib instances, to load the kernel
>>>> command line from the *synthetic filesystem* (rather than directly
>>>> from fw_cfg),
>>>>
>>>> then we could centralize the hashing to just QemuKernelLoaderFsDxe.
>>>>
>>>> Ard -- what's your thought on this?
>>>>
>>>
>>> I understand there's agreement here, and that both this suggested
>>> change (use the synthetic filesystem) and my patch series (add hash
>>> verification) touch the same code areas.  How do you envision this
>>> process in the mailing list?  Seperate patch serieses with dependency?
>>> One long patch series with both changes?  What goes first?
>>
>> Good point. I do have a kind of patch order laid out in my mind, but I
>> didn't think of whether we should have the patches in one patch series,
>> or in two "waves".
>>
>> OK, let's go with two patch sets.
>>
>> In the first set, we should just focus on the above steps (a) and (b).
>> Step (a) shouldn't be too hard. In step (b), you'd modify both
>> QemuLoadImageLib instances (two separate patches), replacing the
>> QemuFwCfgLib APIs for fetching the cmdline with
>> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL and EFI_FILE_PROTOCOL APIs.
>>
>> Speaking from memory, the synthetic filesystem has a unique device path,
>> so the first step would be calling gBS->LocateDevicePath(), for finding
>> SimpleFs on the unique device path. Once you have the SimpleFs
>> interface, you can call OpenVolume, then open the "cmdline" file using
>> the EFI_FILE_PROTOCOL output by OpenVolume.
>>
>> Once we merge this series (basically just three patches), there is no
>> QemuFwCfgLib dependency left in either QemuLoadImageLib instance, I
>> reckon. Then you can post the second wave, in which:
>>
>> - a new "firmware config verifier" library class is introduced,
>>
>> - two library instances for that class are introduced (null, and the
>>   real thing),
>>
>> - the AmdSevX64.dsc platform resolves the new lib class to the "real"
>>   (hashing) instance,
>>
>> - all other platform DSCs using QemuKernelLoaderFsDxe resolve the new
>>   lib class to the null instance,
>>
>> - QemuKernelLoaderFsDxe is extended with a dependency on the new class,
>>   calling the proper APIs to (a) initialize the verifier, and (b) verify
>>   every fw_cfg blob that is about to be exposed as a synthetic file.
>>
>> Then QemuLoadImageLib needs no changes, as it will not depend on fw_cfg,
>> and every synthetic file it may want to access will have been verified
>> by QemuKernelLoaderFsDxe already, according to the verifier lib instance
>> that's used in the respective platform DSC file.
>>
>> I would recommend only posting the first patch set initially. It has a
>> very well defined goal (--> hide the fw_cfg dependency in both
>> QemuLoadImageLib instances behind the synthetic filesystem); we can
>> validate / review that regardless of the ultimate crypto / security
>> goal. Using the SimpleFs / FILE protocol APIs is not trivial IMO, so
>> it's possible that just the first wave will require a v2.
>>
> 
> OK, I'll try to follow this plan.
> 
>>>
>>>
>>>>
>>>> And then, we could eliminate the dynamic callback registration, plus
>>>> the separate SevFwCfgVerifier, SevHashFinderLib, and
>>>> SevQemuLoadImageLib stuff.
>>>>
>>>> We'd only need one new lib class, with *statically linked* hooks for
>>>> QemuKernelLoaderFsDxe, and two instances of this new class, a Null
>>>> one, and an actual (SEV hash verifier) one. The latter instance would
>>>> locate the hash values, calculate the fresh hashes, and perform the
>>>> comparisons. Only the AmdSevX64 platform would use the non-Null
>>>> instance of this library class.
>>>
>>> OK, I'll refactor to static linking with two BlobVerifierLib
>>> imlementations.
>>>
>>>
>>>>
>>>> (NB QemuKernelLoaderFsDxe is used by some ArmVirtPkg platforms, so
>>>> resolutions to the Null instance would be required there too.)
>>>
>>> I'll need to learn how to build edk2 for Arm to test this.  Thanks for
>>> the heads-up.
>>
>> With regard to QemuKernelLoaderFsDxe specifically:
>>
>>   build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemu.dsc               -a AARCH64
>>   build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemu.dsc       -a ARM
>>   build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemuKernel.dsc         -a AARCH64
>>   build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemuKernel.dsc -a ARM
>>
>> If you work on an x86_64 machine, you'll need cross-gcc and
>> cross-binutils for this. I have the following packages installed on my
>> laptop:
>>
>>   binutils-aarch64-linux-gnu-2.31.1-3.el7.x86_64
>>   binutils-arm-linux-gnu-2.31.1-3.el7.x86_64
>>   cross-binutils-common-2.31.1-3.el7.noarch
>>
>>   cross-gcc-common-9.2.1-3.el7.1.noarch
>>   gcc-aarch64-linux-gnu-9.2.1-3.el7.1.x86_64
>>   gcc-arm-linux-gnu-9.2.1-3.el7.1.x86_64
>>   gcc-c++-aarch64-linux-gnu-9.2.1-3.el7.1.x86_64
>>   gcc-c++-arm-linux-gnu-9.2.1-3.el7.1.x86_64
>>
>> (I don't remember why I have the c++ cross-compiler installed.)
>>
> 
> Thanks for the details; I'll try it.
> 
> -Dov
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76130): https://edk2.groups.io/g/devel/message/76130
Mute This Topic: https://groups.io/mt/83074450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by Dov Murik 2 years, 11 months ago


On 04/06/2021 14:26, Laszlo Ersek wrote:
> On 06/04/21 12:30, Dov Murik wrote:
> 

...

>>
>>> [Ard, please see this one question:]
>>>
>>> - A major complication for hashing all three of: kernel, initrd,
>>> cmdline, is that the *fetching* of this triplet is split between two
>>> places. (Well, it is split between *three* places in fact, but I'm
>>> going to ignore LinuxInitrdDynamicShellCommand for now, because the
>>> AmdSevX64 platform sets BUILD_SHELL to FALSE for production.)
>>>
>>> The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but
>>> the command line is fetched in (both) QemuLoadImageLib instances.
>>> This requires that all these modules be littered with hashing as
>>> well, which I find *really bad*. Even if we factor out the actual
>>> logic, I strongly dislike having *just hooks* for hashing in multiple
>>> modules.
>>>
>>> Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe:
>>> don't expose kernel command line", 2020-03-05). If we first
>>>
>>> (a) reverted that commit, and
>>>
>>> (b) modified *both* QemuLoadImageLib instances, to load the kernel
>>> command line from the *synthetic filesystem* (rather than directly
>>> from fw_cfg),
>>>
>>> then we could centralize the hashing to just QemuKernelLoaderFsDxe.
>>>
>>> Ard -- what's your thought on this?
>>>
>>
>> I understand there's agreement here, and that both this suggested
>> change (use the synthetic filesystem) and my patch series (add hash
>> verification) touch the same code areas.  How do you envision this
>> process in the mailing list?  Seperate patch serieses with dependency?
>> One long patch series with both changes?  What goes first?
> 
> Good point. I do have a kind of patch order laid out in my mind, but I
> didn't think of whether we should have the patches in one patch series,
> or in two "waves".
> 
> OK, let's go with two patch sets.
> 
> In the first set, we should just focus on the above steps (a) and (b).
> Step (a) shouldn't be too hard. In step (b), you'd modify both
> QemuLoadImageLib instances (two separate patches), replacing the
> QemuFwCfgLib APIs for fetching the cmdline with
> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL and EFI_FILE_PROTOCOL APIs.
> 
> Speaking from memory, the synthetic filesystem has a unique device path,
> so the first step would be calling gBS->LocateDevicePath(), for finding
> SimpleFs on the unique device path. Once you have the SimpleFs
> interface, you can call OpenVolume, then open the "cmdline" file using
> the EFI_FILE_PROTOCOL output by OpenVolume.
> 
> Once we merge this series (basically just three patches), there is no
> QemuFwCfgLib dependency left in either QemuLoadImageLib instance, I
> reckon. 

I started working on that, and managed to remove all QemuFwCfg* calls in
the main path of QemuLoadKernelImage (so far working on
X86QemuLoadImageLib.c).  That works fine: I read the content of the
"cmdline" synthetic file, and I check the size of the synthetic "initrd"
file.  I used Library/FileHandleLib.h; I hope that's fine.

However, there's another path (which I don't reach with my test setup),
which is the call to QemuLoadLegacyImage, which has a lot of calls to
QemuFwCfg* in its body.

Am I expected to change that legacy path as well?
Or is it in a "it's working don't touch" state?
If I modify this, how do I test it?


Thanks for the guidance,

-Dov



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76193): https://edk2.groups.io/g/devel/message/76193
Mute This Topic: https://groups.io/mt/83074450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by Laszlo Ersek 2 years, 11 months ago

Ard,

do you have any comments please, on the topic at the bottom?

My comments follow:

On 06/08/21 11:57, Dov Murik wrote:
>
>
> On 04/06/2021 14:26, Laszlo Ersek wrote:
>> On 06/04/21 12:30, Dov Murik wrote:
>>
>
> ...
>
>>>
>>>> [Ard, please see this one question:]
>>>>
>>>> - A major complication for hashing all three of: kernel, initrd,
>>>> cmdline, is that the *fetching* of this triplet is split between
>>>> two places. (Well, it is split between *three* places in fact, but
>>>> I'm going to ignore LinuxInitrdDynamicShellCommand for now, because
>>>> the AmdSevX64 platform sets BUILD_SHELL to FALSE for production.)
>>>>
>>>> The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but
>>>> the command line is fetched in (both) QemuLoadImageLib instances.
>>>> This requires that all these modules be littered with hashing as
>>>> well, which I find *really bad*. Even if we factor out the actual
>>>> logic, I strongly dislike having *just hooks* for hashing in
>>>> multiple modules.
>>>>
>>>> Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe:
>>>> don't expose kernel command line", 2020-03-05). If we first
>>>>
>>>> (a) reverted that commit, and
>>>>
>>>> (b) modified *both* QemuLoadImageLib instances, to load the kernel
>>>> command line from the *synthetic filesystem* (rather than directly
>>>> from fw_cfg),
>>>>
>>>> then we could centralize the hashing to just QemuKernelLoaderFsDxe.
>>>>
>>>> Ard -- what's your thought on this?
>>>>
>>>
>>> I understand there's agreement here, and that both this suggested
>>> change (use the synthetic filesystem) and my patch series (add hash
>>> verification) touch the same code areas.  How do you envision this
>>> process in the mailing list?  Seperate patch serieses with
>>> dependency? One long patch series with both changes?  What goes
>>> first?
>>
>> Good point. I do have a kind of patch order laid out in my mind, but
>> I didn't think of whether we should have the patches in one patch
>> series, or in two "waves".
>>
>> OK, let's go with two patch sets.
>>
>> In the first set, we should just focus on the above steps (a) and
>> (b). Step (a) shouldn't be too hard. In step (b), you'd modify both
>> QemuLoadImageLib instances (two separate patches), replacing the
>> QemuFwCfgLib APIs for fetching the cmdline with
>> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL and EFI_FILE_PROTOCOL APIs.
>>
>> Speaking from memory, the synthetic filesystem has a unique device
>> path, so the first step would be calling gBS->LocateDevicePath(), for
>> finding SimpleFs on the unique device path. Once you have the
>> SimpleFs interface, you can call OpenVolume, then open the "cmdline"
>> file using the EFI_FILE_PROTOCOL output by OpenVolume.
>>
>> Once we merge this series (basically just three patches), there is no
>> QemuFwCfgLib dependency left in either QemuLoadImageLib instance, I
>> reckon.
>
> I started working on that, and managed to remove all QemuFwCfg* calls
> in the main path of QemuLoadKernelImage (so far working on
> X86QemuLoadImageLib.c).  That works fine: I read the content of the
> "cmdline" synthetic file, and I check the size of the synthetic
> "initrd" file.  I used Library/FileHandleLib.h; I hope that's fine.

The lib class header says "Provides interface to EFI_FILE_HANDLE
functionality", which is not too bad; but I don't immediately see what
those APIs save us -- the APIs that I believe to be relevant to this use
case all seem to be thin wrappers around EFI_FILE_PROTOCOL. (The only
instance of FileHandleLib is
"MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf".) While not
necessarily a problem, it doesn't seem an obvious win (unless it saves
you much complexity and/or code in a way that I'm missing).

In OVMF, the following executables use UefiFileHandleLib at the moment:

- MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
- ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
- ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.inf
- OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
- ShellPkg/Application/Shell/Shell.inf

The last four are shell-related, so "prior art" is really just BdsDxe...

> However, there's another path (which I don't reach with my test
> setup), which is the call to QemuLoadLegacyImage, which has a lot of
> calls to QemuFwCfg* in its body.
>
> Am I expected to change that legacy path as well?
> Or is it in a "it's working don't touch" state?
> If I modify this, how do I test it?

The use case that you foresee for this feature is really important here.

When you say that you don't reach QemuLoadLegacyImage(), that means your
guest kernel is built with the UEFI stub.

(1) If you can make the Linux UEFI stub a *required* part of the use
case, then:

(1a) switch the QemuLoadImageLib class resolution from
"OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf" to
"OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf" in
"OvmfPkg/AmdSev/AmdSevX64.dsc",

(1b) modify "OvmfPkg/Library/GenericQemuLoadImageLib" only (your
modifications thus far should be easy to transplant to this lib
instance); ignore "OvmfPkg/Library/X86QemuLoadImageLib". There is no
QemuLoadLegacyImage() in GenericQemuLoadImageLib.

This makes sense especially because "OvmfPkg/AmdSev/AmdSevX64.dsc" does
not offer Secure Boot support, so there's not going to be a case when
gBS->LoadImage() rejects the UEFI stubbed Linux binary due to Secure
Boot verification failing (EFI_SECURITY_VIOLATION, EFI_ACCESS_DENIED).

(2) If you cannot make the Linux UEFI stub a required part of the use
case, then X86QemuLoadImageLib needs to be modified indeed.

(2a) Unfortunately, in this case we have to add a hack, because the
synthetic filesystem only exposes the concatenated "setup data + kernel
image" blob. The following would have to be preserved (as the only
remaining QemuFwCfgLib access):

  QemuFwCfgSelectItem (QemuFwCfgItemKernelSetupSize);
  SetupSize = (UINTN)QemuFwCfgRead32 ();

(2b) and then the kernel blob from the synthetic fs would have to be
split in two (= setup, kernel), within QemuLoadLegacyImage().

I'm sorry for missing this aspect previously! I really hope we can go
with (1)!

Thanks,
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76194): https://edk2.groups.io/g/devel/message/76194
Mute This Topic: https://groups.io/mt/83074450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by Dov Murik 2 years, 11 months ago


On 08/06/2021 13:59, Laszlo Ersek wrote:
> Ard,
> 
> do you have any comments please, on the topic at the bottom?
> 
> My comments follow:
> 
> On 06/08/21 11:57, Dov Murik wrote:
>>
>>
>> On 04/06/2021 14:26, Laszlo Ersek wrote:
>>> On 06/04/21 12:30, Dov Murik wrote:
>>>
>>
>> ...
>>
>>>>
>>>>> [Ard, please see this one question:]
>>>>>
>>>>> - A major complication for hashing all three of: kernel, initrd,
>>>>> cmdline, is that the *fetching* of this triplet is split between
>>>>> two places. (Well, it is split between *three* places in fact, but
>>>>> I'm going to ignore LinuxInitrdDynamicShellCommand for now, because
>>>>> the AmdSevX64 platform sets BUILD_SHELL to FALSE for production.)
>>>>>
>>>>> The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but
>>>>> the command line is fetched in (both) QemuLoadImageLib instances.
>>>>> This requires that all these modules be littered with hashing as
>>>>> well, which I find *really bad*. Even if we factor out the actual
>>>>> logic, I strongly dislike having *just hooks* for hashing in
>>>>> multiple modules.
>>>>>
>>>>> Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe:
>>>>> don't expose kernel command line", 2020-03-05). If we first
>>>>>
>>>>> (a) reverted that commit, and
>>>>>
>>>>> (b) modified *both* QemuLoadImageLib instances, to load the kernel
>>>>> command line from the *synthetic filesystem* (rather than directly
>>>>> from fw_cfg),
>>>>>
>>>>> then we could centralize the hashing to just QemuKernelLoaderFsDxe.
>>>>>
>>>>> Ard -- what's your thought on this?
>>>>>
>>>>
>>>> I understand there's agreement here, and that both this suggested
>>>> change (use the synthetic filesystem) and my patch series (add hash
>>>> verification) touch the same code areas.  How do you envision this
>>>> process in the mailing list?  Seperate patch serieses with
>>>> dependency? One long patch series with both changes?  What goes
>>>> first?
>>>
>>> Good point. I do have a kind of patch order laid out in my mind, but
>>> I didn't think of whether we should have the patches in one patch
>>> series, or in two "waves".
>>>
>>> OK, let's go with two patch sets.
>>>
>>> In the first set, we should just focus on the above steps (a) and
>>> (b). Step (a) shouldn't be too hard. In step (b), you'd modify both
>>> QemuLoadImageLib instances (two separate patches), replacing the
>>> QemuFwCfgLib APIs for fetching the cmdline with
>>> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL and EFI_FILE_PROTOCOL APIs.
>>>
>>> Speaking from memory, the synthetic filesystem has a unique device
>>> path, so the first step would be calling gBS->LocateDevicePath(), for
>>> finding SimpleFs on the unique device path. Once you have the
>>> SimpleFs interface, you can call OpenVolume, then open the "cmdline"
>>> file using the EFI_FILE_PROTOCOL output by OpenVolume.
>>>
>>> Once we merge this series (basically just three patches), there is no
>>> QemuFwCfgLib dependency left in either QemuLoadImageLib instance, I
>>> reckon.
>>
>> I started working on that, and managed to remove all QemuFwCfg* calls
>> in the main path of QemuLoadKernelImage (so far working on
>> X86QemuLoadImageLib.c).  That works fine: I read the content of the
>> "cmdline" synthetic file, and I check the size of the synthetic
>> "initrd" file.  I used Library/FileHandleLib.h; I hope that's fine.
> 
> The lib class header says "Provides interface to EFI_FILE_HANDLE
> functionality", which is not too bad; but I don't immediately see what
> those APIs save us -- the APIs that I believe to be relevant to this use
> case all seem to be thin wrappers around EFI_FILE_PROTOCOL. (The only
> instance of FileHandleLib is
> "MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf".) While not
> necessarily a problem, it doesn't seem an obvious win (unless it saves
> you much complexity and/or code in a way that I'm missing).


Using FileHandleGetSize() saves some handling of a EFI_FILE_INFO pointer
and freeing it etc (for getting the size of "cmdline" and "initrd").
But maybe it's better not to add another dependency.


> 
> In OVMF, the following executables use UefiFileHandleLib at the moment:
> 
> - MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> - ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
> - ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.inf
> - OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
> - ShellPkg/Application/Shell/Shell.inf
> 
> The last four are shell-related, so "prior art" is really just BdsDxe...
> 
>> However, there's another path (which I don't reach with my test
>> setup), which is the call to QemuLoadLegacyImage, which has a lot of
>> calls to QemuFwCfg* in its body.
>>
>> Am I expected to change that legacy path as well?
>> Or is it in a "it's working don't touch" state?
>> If I modify this, how do I test it?
> 
> The use case that you foresee for this feature is really important here.
> 
> When you say that you don't reach QemuLoadLegacyImage(), that means your
> guest kernel is built with the UEFI stub.
> 
> (1) If you can make the Linux UEFI stub a *required* part of the use
> case, then:
> 
> (1a) switch the QemuLoadImageLib class resolution from
> "OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf" to
> "OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf" in
> "OvmfPkg/AmdSev/AmdSevX64.dsc",
> 
> (1b) modify "OvmfPkg/Library/GenericQemuLoadImageLib" only (your
> modifications thus far should be easy to transplant to this lib
> instance); ignore "OvmfPkg/Library/X86QemuLoadImageLib". There is no
> QemuLoadLegacyImage() in GenericQemuLoadImageLib.
> 
> This makes sense especially because "OvmfPkg/AmdSev/AmdSevX64.dsc" does
> not offer Secure Boot support, so there's not going to be a case when
> gBS->LoadImage() rejects the UEFI stubbed Linux binary due to Secure
> Boot verification failing (EFI_SECURITY_VIOLATION, EFI_ACCESS_DENIED).
> 
> 
> (2) If you cannot make the Linux UEFI stub a required part of the use
> case, then X86QemuLoadImageLib needs to be modified indeed.
> 
> (2a) Unfortunately, in this case we have to add a hack, because the
> synthetic filesystem only exposes the concatenated "setup data + kernel
> image" blob. The following would have to be preserved (as the only
> remaining QemuFwCfgLib access):
> 
>   QemuFwCfgSelectItem (QemuFwCfgItemKernelSetupSize);
>   SetupSize = (UINTN)QemuFwCfgRead32 ();
> 
> (2b) and then the kernel blob from the synthetic fs would have to be
> split in two (= setup, kernel), within QemuLoadLegacyImage().
> 
> 
> I'm sorry for missing this aspect previously! I really hope we can go
> with (1)!
> 

I'll check.

But if we go with (1) -- do you (and Ard) prefer:

(a) leave X86QemuLoadImageLib as it is in master;

-or-

(b) modify X86QemuLoadImageLib the "main" path to use the
QemuKernelLoaderFs (what I started doing) and leave the "legacy" path
with QemuFwCfg

?


Or, in other words, is the refactoring to read the cmdline from
QemuKernelLoaderFs (across both QemuLoadImageLib implementations)
beneficial even if we don't add the verification "hooks"?


-Dov

> Thanks,
> Laszlo
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76197): https://edk2.groups.io/g/devel/message/76197
Mute This Topic: https://groups.io/mt/83074450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by Laszlo Ersek 2 years, 11 months ago

On 06/08/21 14:09, Dov Murik wrote:
> On 08/06/2021 13:59, Laszlo Ersek wrote:
>> On 06/08/21 11:57, Dov Murik wrote:

>>> I started working on that, and managed to remove all QemuFwCfg*
>>> calls in the main path of QemuLoadKernelImage (so far working on
>>> X86QemuLoadImageLib.c).  That works fine: I read the content of the
>>> "cmdline" synthetic file, and I check the size of the synthetic
>>> "initrd" file.  I used Library/FileHandleLib.h; I hope that's fine.
>>
>> The lib class header says "Provides interface to EFI_FILE_HANDLE
>> functionality", which is not too bad; but I don't immediately see
>> what those APIs save us -- the APIs that I believe to be relevant to
>> this use case all seem to be thin wrappers around EFI_FILE_PROTOCOL.
>> (The only instance of FileHandleLib is
>> "MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf".) While not
>> necessarily a problem, it doesn't seem an obvious win (unless it
>> saves you much complexity and/or code in a way that I'm missing).
>
>
> Using FileHandleGetSize() saves some handling of a EFI_FILE_INFO
> pointer and freeing it etc (for getting the size of "cmdline" and
> "initrd"). But maybe it's better not to add another dependency.

Hmm, no; you have convinced me. Please go ahead with FileHandleLib.
Thank you for taking the time to talk this through with me.

>>> Am I expected to change that legacy path as well?
>>> Or is it in a "it's working don't touch" state?
>>> If I modify this, how do I test it?
>>
>> The use case that you foresee for this feature is really important
>> here.
>>
>> When you say that you don't reach QemuLoadLegacyImage(), that means
>> your guest kernel is built with the UEFI stub.
>>
>> (1) If you can make the Linux UEFI stub a *required* part of the use
>> case, then:
>>
>> (1a) switch the QemuLoadImageLib class resolution from
>> "OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf" to
>> "OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf"
>> in "OvmfPkg/AmdSev/AmdSevX64.dsc",
>>
>> (1b) modify "OvmfPkg/Library/GenericQemuLoadImageLib" only (your
>> modifications thus far should be easy to transplant to this lib
>> instance); ignore "OvmfPkg/Library/X86QemuLoadImageLib". There is no
>> QemuLoadLegacyImage() in GenericQemuLoadImageLib.
>>
>> This makes sense especially because "OvmfPkg/AmdSev/AmdSevX64.dsc"
>> does not offer Secure Boot support, so there's not going to be a case
>> when gBS->LoadImage() rejects the UEFI stubbed Linux binary due to
>> Secure Boot verification failing (EFI_SECURITY_VIOLATION,
>> EFI_ACCESS_DENIED).
>>
>>
>> (2) If you cannot make the Linux UEFI stub a required part of the use
>> case, then X86QemuLoadImageLib needs to be modified indeed.
>>
>> (2a) Unfortunately, in this case we have to add a hack, because the
>> synthetic filesystem only exposes the concatenated "setup data +
>> kernel image" blob. The following would have to be preserved (as the
>> only remaining QemuFwCfgLib access):
>>
>>   QemuFwCfgSelectItem (QemuFwCfgItemKernelSetupSize);
>>   SetupSize = (UINTN)QemuFwCfgRead32 ();
>>
>> (2b) and then the kernel blob from the synthetic fs would have to be
>> split in two (= setup, kernel), within QemuLoadLegacyImage().
>>
>>
>> I'm sorry for missing this aspect previously! I really hope we can go
>> with (1)!
>>
>
> I'll check.
>
> But if we go with (1) -- do you (and Ard) prefer:
>
> (a) leave X86QemuLoadImageLib as it is in master;
>
> -or-
>
> (b) modify X86QemuLoadImageLib the "main" path to use the
> QemuKernelLoaderFs (what I started doing) and leave the "legacy" path
> with QemuFwCfg
>
> ?

I prefer option (a), with the extension that we need to update the
following file-top comment in the files under
"OvmfPkg/Library/X86QemuLoadImageLib":

  X86 specific implementation of QemuLoadImageLib library class interface
  with support for loading mixed mode images and non-EFI stub images

We should add a warning there that this library instance (a) depends on
fw_cfg directly, and (b) is therefore unsuitable for blob verification
purposes.

> Or, in other words, is the refactoring to read the cmdline from
> QemuKernelLoaderFs (across both QemuLoadImageLib implementations)
> beneficial even if we don't add the verification "hooks"?

My answer would be "no". IMO, the refactoring is only useful for
unifying the blob verifier in a single layer (the synthetic fs). Without
the blob verifier in the picture, I see nothing wrong with fetching
different fw_cfg blobs in different modules. The kernel command line
never needed to be available through the synthetic FS, for any
particular consumer, so removing that synthetic file was fine.

Now, we do have an internal consumer of sorts (the blob verifier). Given
that we don't want to add a whole new extra layer for it, and we also
don't want to scatter it over multiple modules, integrating it into the
synthetic FS make sense, hopefully.

I initially thought that the refactoring would benefit both
QemuLoadImageLib instances, but you've shown that I missed the
complications in the legacy path of X86QemuLoadImageLib. Namely, that
the synthetic filesystem abstraction (= the fused setup data + kernel
image blob) isn't a good match for the legacy path.

Library instances are permitted to have different features and different
limitations.

(If we *really* wanted to do refactor the legacy path cleanly, we could
further extend the QemuKernelLoaderFsDxe driver, to present -- under new
filenames -- the setup data blob isolation, and the bare kernel image
blob in isolation. But this would be a lot of wasted work, in practice,
provided that your use case requires a UEFI stub on the guest kernel at
all times (= the legacy path would never be taken). Also, let's not
forget, if we exposed such *new* synthetic files, the blob verifier
would have to verify those isolated blobs too (you'd have to provide
hashes for them from the outside); otherwise the whole exercise would be
moot, I think.)

Thanks,
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76226): https://edk2.groups.io/g/devel/message/76226
Mute This Topic: https://groups.io/mt/83074450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by Dov Murik 2 years, 11 months ago


On 08/06/2021 18:59, Laszlo Ersek wrote:
> On 06/08/21 14:09, Dov Murik wrote:
>> On 08/06/2021 13:59, Laszlo Ersek wrote:
>>> On 06/08/21 11:57, Dov Murik wrote:
> 

>>
>> But if we go with (1) -- do you (and Ard) prefer:
>>
>> (a) leave X86QemuLoadImageLib as it is in master;
>>
>> -or-
>>
>> (b) modify X86QemuLoadImageLib the "main" path to use the
>> QemuKernelLoaderFs (what I started doing) and leave the "legacy" path
>> with QemuFwCfg
>>
>> ?
> 
> I prefer option (a), with the extension that we need to update the
> following file-top comment in the files under
> "OvmfPkg/Library/X86QemuLoadImageLib":
> 
>   X86 specific implementation of QemuLoadImageLib library class interface
>   with support for loading mixed mode images and non-EFI stub images
> 

First attempt at this is submitted to the mailing list:
https://edk2.groups.io/g/devel/message/76265


> We should add a warning there that this library instance (a) depends on
> fw_cfg directly, and (b) is therefore unsuitable for blob verification
> purposes.

I'll add the warning (b) when I add the blob verification feature.

-Dov



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76270): https://edk2.groups.io/g/devel/message/76270
Mute This Topic: https://groups.io/mt/83074450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by Laszlo Ersek 2 years, 11 months ago

On 06/09/21 14:25, Dov Murik wrote:
> 
> 
> On 08/06/2021 18:59, Laszlo Ersek wrote:
>> On 06/08/21 14:09, Dov Murik wrote:
>>> On 08/06/2021 13:59, Laszlo Ersek wrote:
>>>> On 06/08/21 11:57, Dov Murik wrote:
>>
> 
>>>
>>> But if we go with (1) -- do you (and Ard) prefer:
>>>
>>> (a) leave X86QemuLoadImageLib as it is in master;
>>>
>>> -or-
>>>
>>> (b) modify X86QemuLoadImageLib the "main" path to use the
>>> QemuKernelLoaderFs (what I started doing) and leave the "legacy" path
>>> with QemuFwCfg
>>>
>>> ?
>>
>> I prefer option (a), with the extension that we need to update the
>> following file-top comment in the files under
>> "OvmfPkg/Library/X86QemuLoadImageLib":
>>
>>   X86 specific implementation of QemuLoadImageLib library class interface
>>   with support for loading mixed mode images and non-EFI stub images
>>
> 
> First attempt at this is submitted to the mailing list:
> https://edk2.groups.io/g/devel/message/76265
> 
> 
>> We should add a warning there that this library instance (a) depends on
>> fw_cfg directly, and (b) is therefore unsuitable for blob verification
>> purposes.
> 
> I'll add the warning (b) when I add the blob verification feature.

That makes sense to me, thanks.
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76276): https://edk2.groups.io/g/devel/message/76276
Mute This Topic: https://groups.io/mt/83074450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

回复: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by gaoliming 2 years, 11 months ago

Dov:
  Can you submit one BZ for this new feature? I will add it into edk2 202108 stable tag planning. 

Thanks
Liming
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Laszlo Ersek
> 发送时间: 2021年6月9日 21:54
> 收件人: Dov Murik <dovmurik@linux.ibm.com>; devel@edk2.groups.io; Ard
> Biesheuvel <ardb+tianocore@kernel.org>
> 抄送: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>; Tobin
> Feldman-Fitzthum <tobin@ibm.com>; Jim Cadden <jcadden@ibm.com>;
> James Bottomley <jejb@linux.ibm.com>; Hubertus Franke
> <frankeh@us.ibm.com>; Jordan Justen <jordan.l.justen@intel.com>; Ashish
> Kalra <ashish.kalra@amd.com>; Brijesh Singh <brijesh.singh@amd.com>;
> Erdem Aktas <erdemaktas@google.com>; Jiewen Yao
> <jiewen.yao@intel.com>; Min Xu <min.m.xu@intel.com>; Tom Lendacky
> <thomas.lendacky@amd.com>
> 主题: Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with
> kernel/initrd/cmdline
> 
> On 06/09/21 14:25, Dov Murik wrote:
> >
> >
> > On 08/06/2021 18:59, Laszlo Ersek wrote:
> >> On 06/08/21 14:09, Dov Murik wrote:
> >>> On 08/06/2021 13:59, Laszlo Ersek wrote:
> >>>> On 06/08/21 11:57, Dov Murik wrote:
> >>
> >
> >>>
> >>> But if we go with (1) -- do you (and Ard) prefer:
> >>>
> >>> (a) leave X86QemuLoadImageLib as it is in master;
> >>>
> >>> -or-
> >>>
> >>> (b) modify X86QemuLoadImageLib the "main" path to use the
> >>> QemuKernelLoaderFs (what I started doing) and leave the "legacy" path
> >>> with QemuFwCfg
> >>>
> >>> ?
> >>
> >> I prefer option (a), with the extension that we need to update the
> >> following file-top comment in the files under
> >> "OvmfPkg/Library/X86QemuLoadImageLib":
> >>
> >>   X86 specific implementation of QemuLoadImageLib library class
> interface
> >>   with support for loading mixed mode images and non-EFI stub images
> >>
> >
> > First attempt at this is submitted to the mailing list:
> > https://edk2.groups.io/g/devel/message/76265
> >
> >
> >> We should add a warning there that this library instance (a) depends on
> >> fw_cfg directly, and (b) is therefore unsuitable for blob verification
> >> purposes.
> >
> > I'll add the warning (b) when I add the blob verification feature.
> 
> That makes sense to me, thanks.
> Laszlo
> 
> 
> 
> 
> 





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76322): https://edk2.groups.io/g/devel/message/76322
Mute This Topic: https://groups.io/mt/83440567/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: 回复: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by Dov Murik 2 years, 10 months ago

On 10/06/2021 12:15, gaoliming wrote:
> Dov:
>   Can you submit one BZ for this new feature? I will add it into edk2 202108 stable tag planning. 

Submitted: https://bugzilla.tianocore.org/show_bug.cgi?id=3457

I'll add the BZ link to future versions of the patch series.

Thanks,
-Dov

> 
> Thanks
> Liming
>> -----邮件原件-----
>> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Laszlo Ersek
>> 发送时间: 2021年6月9日 21:54
>> 收件人: Dov Murik <dovmurik@linux.ibm.com>; devel@edk2.groups.io; Ard
>> Biesheuvel <ardb+tianocore@kernel.org>
>> 抄送: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>; Tobin
>> Feldman-Fitzthum <tobin@ibm.com>; Jim Cadden <jcadden@ibm.com>;
>> James Bottomley <jejb@linux.ibm.com>; Hubertus Franke
>> <frankeh@us.ibm.com>; Jordan Justen <jordan.l.justen@intel.com>; Ashish
>> Kalra <ashish.kalra@amd.com>; Brijesh Singh <brijesh.singh@amd.com>;
>> Erdem Aktas <erdemaktas@google.com>; Jiewen Yao
>> <jiewen.yao@intel.com>; Min Xu <min.m.xu@intel.com>; Tom Lendacky
>> <thomas.lendacky@amd.com>
>> 主题: Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with
>> kernel/initrd/cmdline
>>
>> On 06/09/21 14:25, Dov Murik wrote:
>>>
>>>
>>> On 08/06/2021 18:59, Laszlo Ersek wrote:
>>>> On 06/08/21 14:09, Dov Murik wrote:
>>>>> On 08/06/2021 13:59, Laszlo Ersek wrote:
>>>>>> On 06/08/21 11:57, Dov Murik wrote:
>>>>
>>>
>>>>>
>>>>> But if we go with (1) -- do you (and Ard) prefer:
>>>>>
>>>>> (a) leave X86QemuLoadImageLib as it is in master;
>>>>>
>>>>> -or-
>>>>>
>>>>> (b) modify X86QemuLoadImageLib the "main" path to use the
>>>>> QemuKernelLoaderFs (what I started doing) and leave the "legacy" path
>>>>> with QemuFwCfg
>>>>>
>>>>> ?
>>>>
>>>> I prefer option (a), with the extension that we need to update the
>>>> following file-top comment in the files under
>>>> "OvmfPkg/Library/X86QemuLoadImageLib":
>>>>
>>>>   X86 specific implementation of QemuLoadImageLib library class
>> interface
>>>>   with support for loading mixed mode images and non-EFI stub images
>>>>
>>>
>>> First attempt at this is submitted to the mailing list:
>>> https://edk2.groups.io/g/devel/message/76265
>>>
>>>
>>>> We should add a warning there that this library instance (a) depends on
>>>> fw_cfg directly, and (b) is therefore unsuitable for blob verification
>>>> purposes.
>>>
>>> I'll add the warning (b) when I add the blob verification feature.
>>
>> That makes sense to me, thanks.
>> Laszlo
>>
>>
>>
>> 
>>
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76452): https://edk2.groups.io/g/devel/message/76452
Mute This Topic: https://groups.io/mt/83440567/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by Ard Biesheuvel 2 years, 11 months ago

On Tue, 8 Jun 2021 at 12:59, Laszlo Ersek <lersek@redhat.com> wrote:
>
> Ard,
>
> do you have any comments please, on the topic at the bottom?
>
> My comments follow:
>
> On 06/08/21 11:57, Dov Murik wrote:
> >
> >
> > On 04/06/2021 14:26, Laszlo Ersek wrote:
> >> On 06/04/21 12:30, Dov Murik wrote:
> >>
> >
> > ...
> >
> >>>
> >>>> [Ard, please see this one question:]
> >>>>
> >>>> - A major complication for hashing all three of: kernel, initrd,
> >>>> cmdline, is that the *fetching* of this triplet is split between
> >>>> two places. (Well, it is split between *three* places in fact, but
> >>>> I'm going to ignore LinuxInitrdDynamicShellCommand for now, because
> >>>> the AmdSevX64 platform sets BUILD_SHELL to FALSE for production.)
> >>>>
> >>>> The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but
> >>>> the command line is fetched in (both) QemuLoadImageLib instances.
> >>>> This requires that all these modules be littered with hashing as
> >>>> well, which I find *really bad*. Even if we factor out the actual
> >>>> logic, I strongly dislike having *just hooks* for hashing in
> >>>> multiple modules.
> >>>>
> >>>> Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe:
> >>>> don't expose kernel command line", 2020-03-05). If we first
> >>>>
> >>>> (a) reverted that commit, and
> >>>>
> >>>> (b) modified *both* QemuLoadImageLib instances, to load the kernel
> >>>> command line from the *synthetic filesystem* (rather than directly
> >>>> from fw_cfg),
> >>>>
> >>>> then we could centralize the hashing to just QemuKernelLoaderFsDxe.
> >>>>
> >>>> Ard -- what's your thought on this?
> >>>>
> >>>
> >>> I understand there's agreement here, and that both this suggested
> >>> change (use the synthetic filesystem) and my patch series (add hash
> >>> verification) touch the same code areas.  How do you envision this
> >>> process in the mailing list?  Seperate patch serieses with
> >>> dependency? One long patch series with both changes?  What goes
> >>> first?
> >>
> >> Good point. I do have a kind of patch order laid out in my mind, but
> >> I didn't think of whether we should have the patches in one patch
> >> series, or in two "waves".
> >>
> >> OK, let's go with two patch sets.
> >>
> >> In the first set, we should just focus on the above steps (a) and
> >> (b). Step (a) shouldn't be too hard. In step (b), you'd modify both
> >> QemuLoadImageLib instances (two separate patches), replacing the
> >> QemuFwCfgLib APIs for fetching the cmdline with
> >> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL and EFI_FILE_PROTOCOL APIs.
> >>
> >> Speaking from memory, the synthetic filesystem has a unique device
> >> path, so the first step would be calling gBS->LocateDevicePath(), for
> >> finding SimpleFs on the unique device path. Once you have the
> >> SimpleFs interface, you can call OpenVolume, then open the "cmdline"
> >> file using the EFI_FILE_PROTOCOL output by OpenVolume.
> >>
> >> Once we merge this series (basically just three patches), there is no
> >> QemuFwCfgLib dependency left in either QemuLoadImageLib instance, I
> >> reckon.
> >
> > I started working on that, and managed to remove all QemuFwCfg* calls
> > in the main path of QemuLoadKernelImage (so far working on
> > X86QemuLoadImageLib.c).  That works fine: I read the content of the
> > "cmdline" synthetic file, and I check the size of the synthetic
> > "initrd" file.  I used Library/FileHandleLib.h; I hope that's fine.
>
> The lib class header says "Provides interface to EFI_FILE_HANDLE
> functionality", which is not too bad; but I don't immediately see what
> those APIs save us -- the APIs that I believe to be relevant to this use
> case all seem to be thin wrappers around EFI_FILE_PROTOCOL. (The only
> instance of FileHandleLib is
> "MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf".) While not
> necessarily a problem, it doesn't seem an obvious win (unless it saves
> you much complexity and/or code in a way that I'm missing).
>
> In OVMF, the following executables use UefiFileHandleLib at the moment:
>
> - MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> - ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
> - ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.inf
> - OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
> - ShellPkg/Application/Shell/Shell.inf
>
> The last four are shell-related, so "prior art" is really just BdsDxe...
>
> > However, there's another path (which I don't reach with my test
> > setup), which is the call to QemuLoadLegacyImage, which has a lot of
> > calls to QemuFwCfg* in its body.
> >
> > Am I expected to change that legacy path as well?
> > Or is it in a "it's working don't touch" state?
> > If I modify this, how do I test it?
>
> The use case that you foresee for this feature is really important here.
>
> When you say that you don't reach QemuLoadLegacyImage(), that means your
> guest kernel is built with the UEFI stub.
>
> (1) If you can make the Linux UEFI stub a *required* part of the use
> case, then:
>
> (1a) switch the QemuLoadImageLib class resolution from
> "OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf" to
> "OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf" in
> "OvmfPkg/AmdSev/AmdSevX64.dsc",
>
> (1b) modify "OvmfPkg/Library/GenericQemuLoadImageLib" only (your
> modifications thus far should be easy to transplant to this lib
> instance); ignore "OvmfPkg/Library/X86QemuLoadImageLib". There is no
> QemuLoadLegacyImage() in GenericQemuLoadImageLib.
>
> This makes sense especially because "OvmfPkg/AmdSev/AmdSevX64.dsc" does
> not offer Secure Boot support, so there's not going to be a case when
> gBS->LoadImage() rejects the UEFI stubbed Linux binary due to Secure
> Boot verification failing (EFI_SECURITY_VIOLATION, EFI_ACCESS_DENIED).
>
>
> (2) If you cannot make the Linux UEFI stub a required part of the use
> case, then X86QemuLoadImageLib needs to be modified indeed.
>
> (2a) Unfortunately, in this case we have to add a hack, because the
> synthetic filesystem only exposes the concatenated "setup data + kernel
> image" blob. The following would have to be preserved (as the only
> remaining QemuFwCfgLib access):
>
>   QemuFwCfgSelectItem (QemuFwCfgItemKernelSetupSize);
>   SetupSize = (UINTN)QemuFwCfgRead32 ();
>
> (2b) and then the kernel blob from the synthetic fs would have to be
> split in two (= setup, kernel), within QemuLoadLegacyImage().
>
>
> I'm sorry for missing this aspect previously! I really hope we can go
> with (1)!
>

The legacy x86 loader should only be kept if there is a strong need to
do so. Keeping it around for some theoretical compatibility concern is
simply not worth it, IMHO.

Having two boot paths to reason about in terms of security is not the
only problem: another concern is that the legacy fallback path is
strictly x86, and is tightly coupled with the kernel's struct
boot_params, which is only documented by the .h file that happens to
declare it (and some outdated prose under Documentation/ perhaps)

Also, the EFI stub does some non-trivial work at boot, and having this
uniform between architectures is an important goal, especially for
non-x86 folks like me. We have introduced an initrd loader mechanism
that is fully arch agnostic, and there are patches on the list to move
the measurement of the initrd into the EFI stub if it was loaded using
this mechanism.

In summary, please stick with the generic loader unless you *really* can't.

-- 
Ard.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76212): https://edk2.groups.io/g/devel/message/76212
Mute This Topic: https://groups.io/mt/83074450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by Laszlo Ersek 2 years, 11 months ago

On 06/08/21 14:49, Ard Biesheuvel wrote:
> On Tue, 8 Jun 2021 at 12:59, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> Ard,
>>
>> do you have any comments please, on the topic at the bottom?
>>
>> My comments follow:
>>
>> On 06/08/21 11:57, Dov Murik wrote:
>>>
>>>
>>> On 04/06/2021 14:26, Laszlo Ersek wrote:
>>>> On 06/04/21 12:30, Dov Murik wrote:
>>>>
>>>
>>> ...
>>>
>>>>>
>>>>>> [Ard, please see this one question:]
>>>>>>
>>>>>> - A major complication for hashing all three of: kernel, initrd,
>>>>>> cmdline, is that the *fetching* of this triplet is split between
>>>>>> two places. (Well, it is split between *three* places in fact, but
>>>>>> I'm going to ignore LinuxInitrdDynamicShellCommand for now, because
>>>>>> the AmdSevX64 platform sets BUILD_SHELL to FALSE for production.)
>>>>>>
>>>>>> The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but
>>>>>> the command line is fetched in (both) QemuLoadImageLib instances.
>>>>>> This requires that all these modules be littered with hashing as
>>>>>> well, which I find *really bad*. Even if we factor out the actual
>>>>>> logic, I strongly dislike having *just hooks* for hashing in
>>>>>> multiple modules.
>>>>>>
>>>>>> Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe:
>>>>>> don't expose kernel command line", 2020-03-05). If we first
>>>>>>
>>>>>> (a) reverted that commit, and
>>>>>>
>>>>>> (b) modified *both* QemuLoadImageLib instances, to load the kernel
>>>>>> command line from the *synthetic filesystem* (rather than directly
>>>>>> from fw_cfg),
>>>>>>
>>>>>> then we could centralize the hashing to just QemuKernelLoaderFsDxe.
>>>>>>
>>>>>> Ard -- what's your thought on this?
>>>>>>
>>>>>
>>>>> I understand there's agreement here, and that both this suggested
>>>>> change (use the synthetic filesystem) and my patch series (add hash
>>>>> verification) touch the same code areas.  How do you envision this
>>>>> process in the mailing list?  Seperate patch serieses with
>>>>> dependency? One long patch series with both changes?  What goes
>>>>> first?
>>>>
>>>> Good point. I do have a kind of patch order laid out in my mind, but
>>>> I didn't think of whether we should have the patches in one patch
>>>> series, or in two "waves".
>>>>
>>>> OK, let's go with two patch sets.
>>>>
>>>> In the first set, we should just focus on the above steps (a) and
>>>> (b). Step (a) shouldn't be too hard. In step (b), you'd modify both
>>>> QemuLoadImageLib instances (two separate patches), replacing the
>>>> QemuFwCfgLib APIs for fetching the cmdline with
>>>> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL and EFI_FILE_PROTOCOL APIs.
>>>>
>>>> Speaking from memory, the synthetic filesystem has a unique device
>>>> path, so the first step would be calling gBS->LocateDevicePath(), for
>>>> finding SimpleFs on the unique device path. Once you have the
>>>> SimpleFs interface, you can call OpenVolume, then open the "cmdline"
>>>> file using the EFI_FILE_PROTOCOL output by OpenVolume.
>>>>
>>>> Once we merge this series (basically just three patches), there is no
>>>> QemuFwCfgLib dependency left in either QemuLoadImageLib instance, I
>>>> reckon.
>>>
>>> I started working on that, and managed to remove all QemuFwCfg* calls
>>> in the main path of QemuLoadKernelImage (so far working on
>>> X86QemuLoadImageLib.c).  That works fine: I read the content of the
>>> "cmdline" synthetic file, and I check the size of the synthetic
>>> "initrd" file.  I used Library/FileHandleLib.h; I hope that's fine.
>>
>> The lib class header says "Provides interface to EFI_FILE_HANDLE
>> functionality", which is not too bad; but I don't immediately see what
>> those APIs save us -- the APIs that I believe to be relevant to this use
>> case all seem to be thin wrappers around EFI_FILE_PROTOCOL. (The only
>> instance of FileHandleLib is
>> "MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf".) While not
>> necessarily a problem, it doesn't seem an obvious win (unless it saves
>> you much complexity and/or code in a way that I'm missing).
>>
>> In OVMF, the following executables use UefiFileHandleLib at the moment:
>>
>> - MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
>> - ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
>> - ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.inf
>> - OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
>> - ShellPkg/Application/Shell/Shell.inf
>>
>> The last four are shell-related, so "prior art" is really just BdsDxe...
>>
>>> However, there's another path (which I don't reach with my test
>>> setup), which is the call to QemuLoadLegacyImage, which has a lot of
>>> calls to QemuFwCfg* in its body.
>>>
>>> Am I expected to change that legacy path as well?
>>> Or is it in a "it's working don't touch" state?
>>> If I modify this, how do I test it?
>>
>> The use case that you foresee for this feature is really important here.
>>
>> When you say that you don't reach QemuLoadLegacyImage(), that means your
>> guest kernel is built with the UEFI stub.
>>
>> (1) If you can make the Linux UEFI stub a *required* part of the use
>> case, then:
>>
>> (1a) switch the QemuLoadImageLib class resolution from
>> "OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf" to
>> "OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf" in
>> "OvmfPkg/AmdSev/AmdSevX64.dsc",
>>
>> (1b) modify "OvmfPkg/Library/GenericQemuLoadImageLib" only (your
>> modifications thus far should be easy to transplant to this lib
>> instance); ignore "OvmfPkg/Library/X86QemuLoadImageLib". There is no
>> QemuLoadLegacyImage() in GenericQemuLoadImageLib.
>>
>> This makes sense especially because "OvmfPkg/AmdSev/AmdSevX64.dsc" does
>> not offer Secure Boot support, so there's not going to be a case when
>> gBS->LoadImage() rejects the UEFI stubbed Linux binary due to Secure
>> Boot verification failing (EFI_SECURITY_VIOLATION, EFI_ACCESS_DENIED).
>>
>>
>> (2) If you cannot make the Linux UEFI stub a required part of the use
>> case, then X86QemuLoadImageLib needs to be modified indeed.
>>
>> (2a) Unfortunately, in this case we have to add a hack, because the
>> synthetic filesystem only exposes the concatenated "setup data + kernel
>> image" blob. The following would have to be preserved (as the only
>> remaining QemuFwCfgLib access):
>>
>>   QemuFwCfgSelectItem (QemuFwCfgItemKernelSetupSize);
>>   SetupSize = (UINTN)QemuFwCfgRead32 ();
>>
>> (2b) and then the kernel blob from the synthetic fs would have to be
>> split in two (= setup, kernel), within QemuLoadLegacyImage().
>>
>>
>> I'm sorry for missing this aspect previously! I really hope we can go
>> with (1)!
>>
> 
> The legacy x86 loader should only be kept if there is a strong need to
> do so. Keeping it around for some theoretical compatibility concern is
> simply not worth it, IMHO.
> 
> Having two boot paths to reason about in terms of security is not the
> only problem: another concern is that the legacy fallback path is
> strictly x86, and is tightly coupled with the kernel's struct
> boot_params, which is only documented by the .h file that happens to
> declare it (and some outdated prose under Documentation/ perhaps)
> 
> Also, the EFI stub does some non-trivial work at boot, and having this
> uniform between architectures is an important goal, especially for
> non-x86 folks like me. We have introduced an initrd loader mechanism
> that is fully arch agnostic, and there are patches on the list to move
> the measurement of the initrd into the EFI stub if it was loaded using
> this mechanism.
> 
> In summary, please stick with the generic loader unless you *really* can't.
> 

Awesome, thank you!
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76227): https://edk2.groups.io/g/devel/message/76227
Mute This Topic: https://groups.io/mt/83074450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Posted by Laszlo Ersek 2 years, 11 months ago

On 05/25/21 07:31, Dov Murik wrote:
> Booting with SEV prevented the loading of kernel, initrd, and kernel
> command-line via QEMU fw_cfg interface because they arrive from the VMM
> which is untrusted in SEV.
> 
> However, in some cases the kernel, initrd, and cmdline are not secret
> but should not be modified by the host.  In such a case, we want to
> verify inside the trusted VM that the kernel, initrd, and cmdline are
> indeed the ones expected by the Guest Owner, and only if that is the
> case go on and boot them up (removing the need for grub inside OVMF in
> that mode).
> 
> This patch series declares a new page in MEMFD which will contain the
> hashes of these three blobs (kernel, initrd, cmdline), each under its
> own GUID entry.  This tables of hashes is populated by QEMU before
> launch, and encrypted as part of the initial VM memory; this makes sure
> theses hashes are part of the SEV measurement (which has to be approved
> by the Guest Owner for secret injection, for example).  Note that this
> requires a new QEMU patch which will be submitted soon.
> 
> OVMF parses the table of hashes populated by QEMU (patch 5), and as it
> reads the fw_cfg blobs from QEMU, it will verify each one against the
> expected hash (kernel and initrd verifiers are introduced in patch 6,
> and command-line verifier is introduced in patches 7+8).  This is all
> done inside the trusted VM context.  If all the hashes are correct, boot
> of the kernel is allowed to continue.
> 
> Any attempt by QEMU to modify the kernel, initrd, cmdline (including
> dropping one of them), or to modify the OVMF code that verifies those
> hashes, will cause the initial SEV measurement to change and therefore
> will be detectable by the Guest Owner during launch before secret
> injection.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ashish Kalra <ashish.kalra@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> 
> James Bottomley (8):
>   OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming
>   OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg
>   OvmfPkg/AmdSev: add a page to the MEMFD for firmware config hashes
>   OvmfPkg/QemuKernelLoaderFsDxe: Add ability to verify loaded items
>   OvmfPkg/AmdSev: Add library to find encrypted hashes for the FwCfg
>     device
>   OvmfPkg/AmdSev: Add firmware file plugin to verifier
>   OvmfPkg: GenericQemuLoadImageLib: Allow verifying fw_cfg command line
>   OvmfPkg/AmdSev: add SevQemuLoadImageLib
> 
>  OvmfPkg/OvmfPkg.dec                                                       |  10 ++
>  OvmfPkg/AmdSev/AmdSevX64.dsc                                              |   9 +-
>  OvmfPkg/AmdSev/AmdSevX64.fdf                                              |   3 +
>  OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf              |  30 +++++
>  OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf              |  34 ++++++
>  OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf        |  30 +++++
>  OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf |   2 +
>  OvmfPkg/ResetVector/ResetVector.inf                                       |   2 +
>  OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h                         |  47 ++++++++
>  OvmfPkg/Include/Library/QemuFwCfgLib.h                                    |  35 ++++++
>  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h                  |  11 ++
>  OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c                |  60 ++++++++++
>  OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c                | 126 ++++++++++++++++++++
>  OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c          |  52 ++++++++
>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.c                                      |   2 +-
>  OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c         |  29 +++++
>  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c                  |   5 +
>  OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c                   |  50 ++++++++
>  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c                     |  31 +++++
>  OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm                              |  20 ++++
>  OvmfPkg/ResetVector/ResetVector.nasmb                                     |   2 +
>  21 files changed, 587 insertions(+), 3 deletions(-)
>  create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf
>  create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf
>  create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf
>  create mode 100644 OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h
>  create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c
>  create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c
>  create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c
>  create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c
> 

I'm confirming that this series is in my review queue.

However, I may need unusually long time to get to it. Thanks for your
patience.

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75792): https://edk2.groups.io/g/devel/message/75792
Mute This Topic: https://groups.io/mt/83074450/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-