v3:

- More grub and boot stripping (I think I got everything out, but
  there may be something that strayed in the boot panic resolution).
- grub.sh tidy up with tabs->spaces.
- Move the reset vector GUIDisation patch to the front so it can be
  applied independently
- Update the .dsc and .fdf files for variable policy

v2:

- Strip more out of AmdSev image (networking, secure boot, smm)
- give sev reset block a generic table guid and use it for boot secret area
- separate secret patches and make grub script more robust
- Add copyrights and fix formatting issues

v1:

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077

This patch series is modelled on the structure of the Bhyve patches
for Ovmf, since it does somewhat similar things.  This patch series
creates a separate build for an AmdSev OVMF.fd that does nothing
except combine with grub and boot straight through the internal grub
to try to mount an encrypted volume.

Concept: SEV Secure Encrypted Images
====================================

The SEV patches in Linux and OVMF allow for the booting of SEV VMs in
an encrypted state, but don't really show how this could be done with
an encrypted image.  Since the key used to decrypt the image must be
maintained within the SEV encryption envelope, encrypted QCOW is not
an option because the key would then have to be known to QEMU which is
outside the encryption envelope.  The proposal here is that an
encrypted image should be a QCOW image consisting of two partitions,
the normal unencrypted EFI partition (Identifying it as an OVMF
bootable image) and a luks encrypted root partition.  The kernel would
be inside the encrypted root in the /boot directory.  The secret
injected securely through QEMU is extracted by OVMF and passed to grub
which uses it to mount the encrypted root and boot the kernel
normally.  The creator of the secret bundle must be satisfied with the
SEV attestation before the secret is constructed.  Unfortunately, the
SEV attestation can only be on the first QEMU firmware volume and
nothing else, so this patch series builds grub itself into a firmware
volume and places it inside OVMF so that the entire boot system can be
attested.  In a normal OVMF KVM system, the variable store is on the
second flash volume (which is read/write).  Unfortunately, this
mutable configuration provided by the variables is outside the
attestation envelope and can significantly alter the boot path,
possibly leading to secret leak, so encrypted image boot should only
be done with the OVMF.fd that combines both the code and variables.
the OVMF.fd is constructed so that it becomes impossible to interrupt
the boot sequence after attestation and the system will either boot
the image or fail. The boot sequence runs the grub.efi embedded in the
OVMF firmware volume so the encrypted image owner knows their own
version of grub is the only one that will boot before injecting the
secret.  Note this boot path actually ignores the unencrypted EFI
partition.  However, as part of this design, the encrypted image may be
booted by a standard OVMF KVM boot and in that case, the user will
have to type the encryption password.  This standard boot will be
insecure but it might be used by the constructor of the encrypted
images on their own private laptop, for instance.  The standard boot
path will use the unencrypted EFI partition.

Patches Required Outside of OVMF
================================

There is a patch set to grub which allows it to extract the SEV secret
area from the configuration table and use the secret as a password to
do a luks crypto mount of root (this is the sevsecret grub module):

https://lists.gnu.org/archive/html/grub-devel/2020-11/msg00078.html

There is also a patch to qemu which allows it to search through the
OVMF.fd and find the SEV secret area which is now described inside the
Reset Vector using the existing SEV_ES reset block.  This area is the
place QEMU will inject the encrypted SEV secret bundle.

Security of the System
======================

Since Grub is now part of the attested OVMF.fd bundle, the VM owner
knows absolutely that it will proceed straight to partition decryption
inside the attested code and boot the kernel off the encrypted
partition.  Even if a different QCOW image is substituted, the boot
will fail without revealing the secret because the system is designed
to fail hard in that case and because the secret is always contained
within the encrypted envelope it should be impossible for the cloud
operator to obtain it even if they can pause the boot and examine the
machine memory.

Putting it All Together
=======================

This is somewhat hard.  You must first understand how to boot a QEMU
system so as to have the VM pause after firmware loading (-S option)
and use the qmp port to request an attestation.  Only if the
attestation corresponds to the expected sha256sum of OVMF.fd should
the secret bundle be constructed and injected using qmp.  The tools
for constructing the secret bundle are in

https://github.com/AMDESE/sev-tool/

James

---

James Bottomley (6):
  OvmfPkg/ResetVector: convert SEV-ES Reset Block structure to be GUIDed
  OvmfPkg/Amdsev: Base commit to build encrypted boot specific OVMF
  OvmfPkg/AmdSev: add Grub Firmware Volume Package
  OvmfPkg: create a SEV secret area in the AmdSev memfd
  OvmfPkg/AmdSev: assign and protect the Sev Secret area
  OvmfPkg/AmdSev: Expose the Sev Secret area using a configuration table

 OvmfPkg/OvmfPkg.dec                           |    8 +
 OvmfPkg/AmdSev/AmdSevX64.dsc                  |  844 ++++++++++
 OvmfPkg/AmdSev/AmdSevX64.fdf                  |  456 +++++
 OvmfPkg/AmdSev/Grub/Grub.inf                  |   39 +
 OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf        |   37 +
 OvmfPkg/AmdSev/SecretPei/SecretPei.inf        |   35 +
 .../PlatformBootManagerLibGrub.inf            |   71 +
 OvmfPkg/ResetVector/ResetVector.inf           |    4 +
 OvmfPkg/Include/Guid/SevLaunchSecret.h        |   28 +
 .../PlatformBootManagerLibGrub/BdsPlatform.h  |  175 ++
 OvmfPkg/AmdSev/SecretDxe/SecretDxe.c          |   26 +
 OvmfPkg/AmdSev/SecretPei/SecretPei.c          |   25 +
 .../PlatformBootManagerLibGrub/BdsPlatform.c  | 1482 +++++++++++++++++
 .../PlatformBootManagerLibGrub/PlatformData.c |  214 +++
 OvmfPkg/AmdSev/Grub/.gitignore                |    1 +
 OvmfPkg/AmdSev/Grub/grub.cfg                  |   46 +
 OvmfPkg/AmdSev/Grub/grub.sh                   |   93 ++
 OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm  |   70 +-
 OvmfPkg/ResetVector/ResetVector.nasmb         |    2 +
 19 files changed, 3645 insertions(+), 11 deletions(-)
 create mode 100644 OvmfPkg/AmdSev/AmdSevX64.dsc
 create mode 100644 OvmfPkg/AmdSev/AmdSevX64.fdf
 create mode 100644 OvmfPkg/AmdSev/Grub/Grub.inf
 create mode 100644 OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
 create mode 100644 OvmfPkg/AmdSev/SecretPei/SecretPei.inf
 create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf
 create mode 100644 OvmfPkg/Include/Guid/SevLaunchSecret.h
 create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h
 create mode 100644 OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
 create mode 100644 OvmfPkg/AmdSev/SecretPei/SecretPei.c
 create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c
 create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformData.c
 create mode 100644 OvmfPkg/AmdSev/Grub/.gitignore
 create mode 100644 OvmfPkg/AmdSev/Grub/grub.cfg
 create mode 100644 OvmfPkg/AmdSev/Grub/grub.sh

-- 
2.26.2



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

On 11/30/20 9:28 PM, James Bottomley wrote:
> v3:
> 
> - More grub and boot stripping (I think I got everything out, but
>    there may be something that strayed in the boot panic resolution).
> - grub.sh tidy up with tabs->spaces.
> - Move the reset vector GUIDisation patch to the front so it can be
>    applied independently
> - Update the .dsc and .fdf files for variable policy
> 
> v2:
> 
> - Strip more out of AmdSev image (networking, secure boot, smm)
> - give sev reset block a generic table guid and use it for boot secret area
> - separate secret patches and make grub script more robust
> - Add copyrights and fix formatting issues
> 
> v1:
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077
> 
> This patch series is modelled on the structure of the Bhyve patches
> for Ovmf, since it does somewhat similar things.  This patch series
> creates a separate build for an AmdSev OVMF.fd that does nothing
> except combine with grub and boot straight through the internal grub
> to try to mount an encrypted volume.
> 

This all looks reasonable to me, although I defer to Laszlo when it 
comes to assessing the impact on maintainability of other platforms 
under OvmfPkg.

Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>

Is there any point to keeping the TPM bits in the AmdSev platform? Or 
are these completely orthogonal? If there is no meaningful way [yet] to 
plumb these together, it might be better to just rip that out entirely 
so people don't make assumptions.



> Concept: SEV Secure Encrypted Images
> ====================================
> 
> The SEV patches in Linux and OVMF allow for the booting of SEV VMs in
> an encrypted state, but don't really show how this could be done with
> an encrypted image.  Since the key used to decrypt the image must be
> maintained within the SEV encryption envelope, encrypted QCOW is not
> an option because the key would then have to be known to QEMU which is
> outside the encryption envelope.  The proposal here is that an
> encrypted image should be a QCOW image consisting of two partitions,
> the normal unencrypted EFI partition (Identifying it as an OVMF
> bootable image) and a luks encrypted root partition.  The kernel would
> be inside the encrypted root in the /boot directory.  The secret
> injected securely through QEMU is extracted by OVMF and passed to grub
> which uses it to mount the encrypted root and boot the kernel
> normally.  The creator of the secret bundle must be satisfied with the
> SEV attestation before the secret is constructed.  Unfortunately, the
> SEV attestation can only be on the first QEMU firmware volume and
> nothing else, so this patch series builds grub itself into a firmware
> volume and places it inside OVMF so that the entire boot system can be
> attested.  In a normal OVMF KVM system, the variable store is on the
> second flash volume (which is read/write).  Unfortunately, this
> mutable configuration provided by the variables is outside the
> attestation envelope and can significantly alter the boot path,
> possibly leading to secret leak, so encrypted image boot should only
> be done with the OVMF.fd that combines both the code and variables.
> the OVMF.fd is constructed so that it becomes impossible to interrupt
> the boot sequence after attestation and the system will either boot
> the image or fail. The boot sequence runs the grub.efi embedded in the
> OVMF firmware volume so the encrypted image owner knows their own
> version of grub is the only one that will boot before injecting the
> secret.  Note this boot path actually ignores the unencrypted EFI
> partition.  However, as part of this design, the encrypted image may be
> booted by a standard OVMF KVM boot and in that case, the user will
> have to type the encryption password.  This standard boot will be
> insecure but it might be used by the constructor of the encrypted
> images on their own private laptop, for instance.  The standard boot
> path will use the unencrypted EFI partition.
> 
> Patches Required Outside of OVMF
> ================================
> 
> There is a patch set to grub which allows it to extract the SEV secret
> area from the configuration table and use the secret as a password to
> do a luks crypto mount of root (this is the sevsecret grub module):
> 
> https://lists.gnu.org/archive/html/grub-devel/2020-11/msg00078.html
> 
> There is also a patch to qemu which allows it to search through the
> OVMF.fd and find the SEV secret area which is now described inside the
> Reset Vector using the existing SEV_ES reset block.  This area is the
> place QEMU will inject the encrypted SEV secret bundle.
> 
> Security of the System
> ======================
> 
> Since Grub is now part of the attested OVMF.fd bundle, the VM owner
> knows absolutely that it will proceed straight to partition decryption
> inside the attested code and boot the kernel off the encrypted
> partition.  Even if a different QCOW image is substituted, the boot
> will fail without revealing the secret because the system is designed
> to fail hard in that case and because the secret is always contained
> within the encrypted envelope it should be impossible for the cloud
> operator to obtain it even if they can pause the boot and examine the
> machine memory.
> 
> Putting it All Together
> =======================
> 
> This is somewhat hard.  You must first understand how to boot a QEMU
> system so as to have the VM pause after firmware loading (-S option)
> and use the qmp port to request an attestation.  Only if the
> attestation corresponds to the expected sha256sum of OVMF.fd should
> the secret bundle be constructed and injected using qmp.  The tools
> for constructing the secret bundle are in
> 
> https://github.com/AMDESE/sev-tool/
> 
> James
> 
> ---
> 
> James Bottomley (6):
>    OvmfPkg/ResetVector: convert SEV-ES Reset Block structure to be GUIDed
>    OvmfPkg/Amdsev: Base commit to build encrypted boot specific OVMF
>    OvmfPkg/AmdSev: add Grub Firmware Volume Package
>    OvmfPkg: create a SEV secret area in the AmdSev memfd
>    OvmfPkg/AmdSev: assign and protect the Sev Secret area
>    OvmfPkg/AmdSev: Expose the Sev Secret area using a configuration table
> 
>   OvmfPkg/OvmfPkg.dec                           |    8 +
>   OvmfPkg/AmdSev/AmdSevX64.dsc                  |  844 ++++++++++
>   OvmfPkg/AmdSev/AmdSevX64.fdf                  |  456 +++++
>   OvmfPkg/AmdSev/Grub/Grub.inf                  |   39 +
>   OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf        |   37 +
>   OvmfPkg/AmdSev/SecretPei/SecretPei.inf        |   35 +
>   .../PlatformBootManagerLibGrub.inf            |   71 +
>   OvmfPkg/ResetVector/ResetVector.inf           |    4 +
>   OvmfPkg/Include/Guid/SevLaunchSecret.h        |   28 +
>   .../PlatformBootManagerLibGrub/BdsPlatform.h  |  175 ++
>   OvmfPkg/AmdSev/SecretDxe/SecretDxe.c          |   26 +
>   OvmfPkg/AmdSev/SecretPei/SecretPei.c          |   25 +
>   .../PlatformBootManagerLibGrub/BdsPlatform.c  | 1482 +++++++++++++++++
>   .../PlatformBootManagerLibGrub/PlatformData.c |  214 +++
>   OvmfPkg/AmdSev/Grub/.gitignore                |    1 +
>   OvmfPkg/AmdSev/Grub/grub.cfg                  |   46 +
>   OvmfPkg/AmdSev/Grub/grub.sh                   |   93 ++
>   OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm  |   70 +-
>   OvmfPkg/ResetVector/ResetVector.nasmb         |    2 +
>   19 files changed, 3645 insertions(+), 11 deletions(-)
>   create mode 100644 OvmfPkg/AmdSev/AmdSevX64.dsc
>   create mode 100644 OvmfPkg/AmdSev/AmdSevX64.fdf
>   create mode 100644 OvmfPkg/AmdSev/Grub/Grub.inf
>   create mode 100644 OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
>   create mode 100644 OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>   create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf
>   create mode 100644 OvmfPkg/Include/Guid/SevLaunchSecret.h
>   create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h
>   create mode 100644 OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
>   create mode 100644 OvmfPkg/AmdSev/SecretPei/SecretPei.c
>   create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c
>   create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformData.c
>   create mode 100644 OvmfPkg/AmdSev/Grub/.gitignore
>   create mode 100644 OvmfPkg/AmdSev/Grub/grub.cfg
>   create mode 100644 OvmfPkg/AmdSev/Grub/grub.sh
> 



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

On 12/01/20 09:05, Ard Biesheuvel wrote:
> On 11/30/20 9:28 PM, James Bottomley wrote:
>> v3:
>>
>> - More grub and boot stripping (I think I got everything out, but
>>    there may be something that strayed in the boot panic resolution).
>> - grub.sh tidy up with tabs->spaces.
>> - Move the reset vector GUIDisation patch to the front so it can be
>>    applied independently
>> - Update the .dsc and .fdf files for variable policy
>>
>> v2:
>>
>> - Strip more out of AmdSev image (networking, secure boot, smm)
>> - give sev reset block a generic table guid and use it for boot secret
>> area
>> - separate secret patches and make grub script more robust
>> - Add copyrights and fix formatting issues
>>
>> v1:
>>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077
>>
>> This patch series is modelled on the structure of the Bhyve patches
>> for Ovmf, since it does somewhat similar things.  This patch series
>> creates a separate build for an AmdSev OVMF.fd that does nothing
>> except combine with grub and boot straight through the internal grub
>> to try to mount an encrypted volume.
>>
> 
> This all looks reasonable to me, although I defer to Laszlo when it
> comes to assessing the impact on maintainability of other platforms
> under OvmfPkg.
> 
> Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>

Thanks for reviewing this! I'll go through v3 later.

And, indeed, it was my request / suggestion (off-list, earlier) that the
feature please be implemented as a separate platform under OvmfPkg. This
new platform has very different goals from the earlier ones; in
particular their attitude about integration with the host side is
entirely different.

> 
> Is there any point to keeping the TPM bits in the AmdSev platform?

I wondered that myself, when I was suggesting the removal of multiple
feature flags (such as SMM_REQUIRE, SECURE_BOOT_ENABLE, etc). TPM_ENABLE
didn't look immediately wrong or unsupportable in the new platform, so I
didn't suggest removing it.

> Or
> are these completely orthogonal? If there is no meaningful way [yet] to
> plumb these together, it might be better to just rip that out entirely
> so people don't make assumptions.

It's certainly good to trim this platform to the bare minimum, I'm just
generally unsure about TPM (swtpm / vTPM) use cases with OVMF (I never
use that feature, personally). I wouldn't want to regress an otherwise
valid use case.

Thanks
Laszlo



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

On Tue, 2020-12-01 at 09:05 +0100, Ard Biesheuvel wrote:
> On 11/30/20 9:28 PM, James Bottomley wrote:
> > v3:
> > 
> > - More grub and boot stripping (I think I got everything out, but
> >    there may be something that strayed in the boot panic
> > resolution).
> > - grub.sh tidy up with tabs->spaces.
> > - Move the reset vector GUIDisation patch to the front so it can be
> >    applied independently
> > - Update the .dsc and .fdf files for variable policy
> > 
> > v2:
> > 
> > - Strip more out of AmdSev image (networking, secure boot, smm)
> > - give sev reset block a generic table guid and use it for boot
> > secret area
> > - separate secret patches and make grub script more robust
> > - Add copyrights and fix formatting issues
> > 
> > v1:
> > 
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077
> > 
> > This patch series is modelled on the structure of the Bhyve patches
> > for Ovmf, since it does somewhat similar things.  This patch series
> > creates a separate build for an AmdSev OVMF.fd that does nothing
> > except combine with grub and boot straight through the internal
> > grub to try to mount an encrypted volume.
> > 
> 
> This all looks reasonable to me, although I defer to Laszlo when it 
> comes to assessing the impact on maintainability of other platforms 
> under OvmfPkg.
> 
> Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> 
> Is there any point to keeping the TPM bits in the AmdSev platform?
> Or are these completely orthogonal? If there is no meaningful way
> [yet] to plumb these together, it might be better to just rip that
> out entirely so people don't make assumptions.

There is definitely a use case in the cloud for an attested VM boot and
run time whether that VM is encrypted or not.  Although the SEV
attestation covers OVMF/grub it would still be useful to get the
regular boot time attestation from them as well, so I think OVMF will
require a TPM device even in the SEV use case.

Obviously there is still a question of how you run a vtpm in this use
case, since it would have to be part of the trusted envelope as well,
so it can't simply run in the host like vtpms usually do.  However,
SEV-SNP is moving towards the concept of a trusted management VM, so I
do envisage that the vtpm would eventually be part of this or another
set up like it so it would be separate from the encrypted VM that's
running but part of the trusted envelope.

James




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

Hi James,

On 11/30/20 21:28, James Bottomley wrote:
> v3:
> 
> - More grub and boot stripping (I think I got everything out, but
>   there may be something that strayed in the boot panic resolution).
> - grub.sh tidy up with tabs->spaces.
> - Move the reset vector GUIDisation patch to the front so it can be
>   applied independently
> - Update the .dsc and .fdf files for variable policy

In preparation for submitting the github PR for merging this series, I
first ran PatchCheck.py locally.

It doesn't like that I converted "OvmfPkg/AmdSev/Grub/grub.cfg" to LF,
in addition to "OvmfPkg/AmdSev/Grub/grub.sh".

PatchCheck.py recognizes the ".sh" suffix, so it's not complaining about
"OvmfPkg/AmdSev/Grub/grub.sh". But, the config file is a problem.

Can you please confirm that grub works fine if the config file has CRLF
line terminators? Because then I'll just convert the config file back to
CRLF, and merge the series.

Otherwise, we'll need a patch for "BaseTools/Scripts/PatchCheck.py", to
treat "grub.cfg" as another exception.

Thanks!
Laszlo



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

On Thu, 2020-12-03 at 13:26 +0100, Laszlo Ersek wrote:
> Hi James,
> 
> On 11/30/20 21:28, James Bottomley wrote:
> > v3:
> > 
> > - More grub and boot stripping (I think I got everything out, but
> >   there may be something that strayed in the boot panic
> > resolution).
> > - grub.sh tidy up with tabs->spaces.
> > - Move the reset vector GUIDisation patch to the front so it can be
> >   applied independently
> > - Update the .dsc and .fdf files for variable policy
> 
> In preparation for submitting the github PR for merging this series,
> I first ran PatchCheck.py locally.
> 
> It doesn't like that I converted "OvmfPkg/AmdSev/Grub/grub.cfg" to
> LF, in addition to "OvmfPkg/AmdSev/Grub/grub.sh".
> 
> PatchCheck.py recognizes the ".sh" suffix, so it's not complaining
> about "OvmfPkg/AmdSev/Grub/grub.sh". But, the config file is a
> problem.
> 
> Can you please confirm that grub works fine if the config file has
> CRLF line terminators? Because then I'll just convert the config file
> back to CRLF, and merge the series.

I converted the entire file with unix2dos and grub does seem to be fine
with the CRLF line endings (probably because it wants to be a boot
loader beyond linux), so I think converting the file to CRLF is the
right way to go.

James






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

On 12/03/20 15:27, James Bottomley wrote:
> On Thu, 2020-12-03 at 13:26 +0100, Laszlo Ersek wrote:
>> Hi James,
>>
>> On 11/30/20 21:28, James Bottomley wrote:
>>> v3:
>>>
>>> - More grub and boot stripping (I think I got everything out, but
>>>   there may be something that strayed in the boot panic
>>> resolution).
>>> - grub.sh tidy up with tabs->spaces.
>>> - Move the reset vector GUIDisation patch to the front so it can be
>>>   applied independently
>>> - Update the .dsc and .fdf files for variable policy
>>
>> In preparation for submitting the github PR for merging this series,
>> I first ran PatchCheck.py locally.
>>
>> It doesn't like that I converted "OvmfPkg/AmdSev/Grub/grub.cfg" to
>> LF, in addition to "OvmfPkg/AmdSev/Grub/grub.sh".
>>
>> PatchCheck.py recognizes the ".sh" suffix, so it's not complaining
>> about "OvmfPkg/AmdSev/Grub/grub.sh". But, the config file is a
>> problem.
>>
>> Can you please confirm that grub works fine if the config file has
>> CRLF line terminators? Because then I'll just convert the config file
>> back to CRLF, and merge the series.
> 
> I converted the entire file with unix2dos and grub does seem to be fine
> with the CRLF line endings (probably because it wants to be a boot
> loader beyond linux), so I think converting the file to CRLF is the
> right way to go.

I submitted PR <https://github.com/tianocore/edk2/pull/1175>, but it was
rejected due to ECC failures.

I think we all need to have a serious talk about ECC. I'll send a
separate email.

I'm really sorry.
Laszlo



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

On Fri, 2020-12-04 at 01:46 +0100, Laszlo Ersek wrote:
> On 12/03/20 15:27, James Bottomley wrote:
> > On Thu, 2020-12-03 at 13:26 +0100, Laszlo Ersek wrote:
> > > Hi James,
> > > 
> > > On 11/30/20 21:28, James Bottomley wrote:
> > > > v3:
> > > > 
> > > > - More grub and boot stripping (I think I got everything out,
> > > > but there may be something that strayed in the boot panic
> > > > resolution).
> > > > - grub.sh tidy up with tabs->spaces.
> > > > - Move the reset vector GUIDisation patch to the front so it
> > > > can be applied independently
> > > > - Update the .dsc and .fdf files for variable policy
> > > 
> > > In preparation for submitting the github PR for merging this
> > > series, I first ran PatchCheck.py locally.
> > > 
> > > It doesn't like that I converted "OvmfPkg/AmdSev/Grub/grub.cfg"
> > > to LF, in addition to "OvmfPkg/AmdSev/Grub/grub.sh".
> > > 
> > > PatchCheck.py recognizes the ".sh" suffix, so it's not
> > > complaining about "OvmfPkg/AmdSev/Grub/grub.sh". But, the config
> > > file is a problem.
> > > 
> > > Can you please confirm that grub works fine if the config file
> > > has CRLF line terminators? Because then I'll just convert the
> > > config file back to CRLF, and merge the series.
> > 
> > I converted the entire file with unix2dos and grub does seem to be
> > fine with the CRLF line endings (probably because it wants to be a
> > boot loader beyond linux), so I think converting the file to CRLF
> > is the right way to go.
> 
> I submitted PR <https://github.com/tianocore/edk2/pull/1175>;, but it
> was rejected due to ECC failures.
> 
> I think we all need to have a serious talk about ECC. I'll send a
> separate email.
> 
> I'm really sorry.

Is it just complaining that gGrubFileGuid is the same as the FILE_GUID
in Grub.inf (as the comment in Grub.inf says) but it shouldn't be, so
the fix is to remove the comment and generate a new FILE_GUID for
Grub.inf?

James




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

On 12/04/20 02:05, James Bottomley wrote:
> On Fri, 2020-12-04 at 01:46 +0100, Laszlo Ersek wrote:
>> On 12/03/20 15:27, James Bottomley wrote:
>>> On Thu, 2020-12-03 at 13:26 +0100, Laszlo Ersek wrote:
>>>> Hi James,
>>>>
>>>> On 11/30/20 21:28, James Bottomley wrote:
>>>>> v3:
>>>>>
>>>>> - More grub and boot stripping (I think I got everything out,
>>>>> but there may be something that strayed in the boot panic
>>>>> resolution).
>>>>> - grub.sh tidy up with tabs->spaces.
>>>>> - Move the reset vector GUIDisation patch to the front so it
>>>>> can be applied independently
>>>>> - Update the .dsc and .fdf files for variable policy
>>>>
>>>> In preparation for submitting the github PR for merging this
>>>> series, I first ran PatchCheck.py locally.
>>>>
>>>> It doesn't like that I converted "OvmfPkg/AmdSev/Grub/grub.cfg"
>>>> to LF, in addition to "OvmfPkg/AmdSev/Grub/grub.sh".
>>>>
>>>> PatchCheck.py recognizes the ".sh" suffix, so it's not
>>>> complaining about "OvmfPkg/AmdSev/Grub/grub.sh". But, the config
>>>> file is a problem.
>>>>
>>>> Can you please confirm that grub works fine if the config file
>>>> has CRLF line terminators? Because then I'll just convert the
>>>> config file back to CRLF, and merge the series.
>>>
>>> I converted the entire file with unix2dos and grub does seem to be
>>> fine with the CRLF line endings (probably because it wants to be a
>>> boot loader beyond linux), so I think converting the file to CRLF
>>> is the right way to go.
>>
>> I submitted PR <https://github.com/tianocore/edk2/pull/1175>;, but it
>> was rejected due to ECC failures.
>>
>> I think we all need to have a serious talk about ECC. I'll send a
>> separate email.
>>
>> I'm really sorry.
>
> Is it just complaining that gGrubFileGuid is the same as the FILE_GUID
> in Grub.inf (as the comment in Grub.inf says) but it shouldn't be, so
> the fix is to remove the comment and generate a new FILE_GUID for
> Grub.inf?
>
> James
>
>

Log files (same content, modulo timestamps etc):

  https://dev.azure.com/tianocore/11ea4a10-ac9f-4e5f-8b13-7def1f19d478/_apis/build/builds/16071/logs/115
  https://dev.azure.com/tianocore/11ea4a10-ac9f-4e5f-8b13-7def1f19d478/_apis/build/builds/16072/logs/113

Two plugins failed (search either log for the string "Test Failed" --
there's going to be two instances).

(1) The GUID check plugin is one of both failures.

It's unjustified of course, because you *need* to know the FILE_GUID of
Grub.inf -- that's how you can generate boot option to it in the
Platform BDS library. Making gGrubFileGuid different from what's in
FILE_GUID would *break the code*.

One hack for this is to change the FILE_GUID in Grub.inf (regenerate it
with uuidgen), but then use a DSC-level FILE_GUID override, to make it
match gGrubFileGuid from the DEC file again:

> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
> index bb7697eb324b..1593856faca1 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
> @@ -779,7 +779,10 @@ [Components]
>    }
>  !endif
>    OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
> -  OvmfPkg/AmdSev/Grub/Grub.inf
> +  OvmfPkg/AmdSev/Grub/Grub.inf {
> +    <Defines>
> +      FILE_GUID = ...
> +  }
>  !if $(BUILD_SHELL) == TRUE
>    ShellPkg/Application/Shell/Shell.inf {
>      <LibraryClasses>

But it's terrible for code that's actually valid.

So instead, we should add an exception to "OvmfPkg/OvmfPkg.ci.yaml",
near "GuidCheck".

(2) The other failure is from ECC.

I've evaluated the errors it has reported now, and all of them are
unjustifiedly reported.

Again, more exceptions are needed in "OvmfPkg/OvmfPkg.ci.yaml", this
time near "EccCheck".

I will send a short patch series to add the exceptions, and once that's
upstream, we *will* merge this (v3) series.

Thanks
Laszlo



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

On 12/04/20 02:55, Laszlo Ersek wrote:

> I will send a short patch series to add the exceptions, and once
> that's upstream, we *will* merge this (v3) series.

BTW the tweaks I added on top of your v3, in
<https://github.com/tianocore/edk2/pull/1175>, are as follows (git
range-diff output):

> 1:  4020c20b2342 ! 1:  b96494ad75db OvmfPkg/ResetVector: convert SEV-ES Reset Block structure to be GUIDed
>     @@ -8,8 +8,9 @@
>
>          Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077
>          Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>     -
>          Message-Id: <20201130202819.3910-2-jejb@linux.ibm.com>
>     +    Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>     +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
>      diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
>      --- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
> 2:  488fbdbe7689 ! 2:  acc8cb13da8d OvmfPkg/Amdsev: Base commit to build encrypted boot specific OVMF
>     @@ -11,8 +11,9 @@
>
>          Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077
>          Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>     -
>          Message-Id: <20201130202819.3910-3-jejb@linux.ibm.com>
>     +    Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>     +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
>      diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
>      new file mode 100644
> 3:  796ec96e3414 ! 3:  b80ce0838781 OvmfPkg/AmdSev: add Grub Firmware Volume Package
>     @@ -19,8 +19,10 @@
>
>          Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077
>          Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>     -
>          Message-Id: <20201130202819.3910-4-jejb@linux.ibm.com>
>     +    Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>     +    [lersek@redhat.com: replace local variable initialization with assignment]
>     +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
>      diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
>      --- a/OvmfPkg/OvmfPkg.dec
>     @@ -779,7 +781,9 @@
>      +{
>      +  EFI_HANDLE    Handle;
>      +  EFI_STATUS    Status;
>     -+  UINT16 FrontPageTimeout = 0;
>     ++  UINT16        FrontPageTimeout;
>     ++
>     ++  FrontPageTimeout = 0;
>      +
>      +  DEBUG ((DEBUG_INFO, "PlatformBootManagerBeforeConsole\n"));
>      +  InstallDevicePathCallback ();
> 4:  d954947f8d14 ! 4:  f3cda3cadde4 OvmfPkg: create a SEV secret area in the AmdSev memfd
>     @@ -10,8 +10,9 @@
>          Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077
>          Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>          Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>     -
>          Message-Id: <20201130202819.3910-5-jejb@linux.ibm.com>
>     +    Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>     +    [lersek@redhat.com: fix typo in "ResetVectorVtf0.asm" comments]
>
>      diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
>      --- a/OvmfPkg/OvmfPkg.dec
>     @@ -52,7 +53,7 @@
>      +;
>      +; SEV Secret block
>      +;
>     -+; This describes the guest ram area where the hypervisor may should
>     ++; This describes the guest ram area where the hypervisor should
>      +; inject the secret.  The data format is:
>      +;
>      +; base physical address (32 bit word)
> 5:  1a18c4921cdf ! 5:  c38b3caf22ad OvmfPkg/AmdSev: assign and protect the Sev Secret area
>     @@ -1,14 +1,17 @@
>      Author: James Bottomley <jejb@linux.ibm.com>
>
>     -    OvmfPkg/AmdSev: assign and protect the Sev Secret area
>     +    OvmfPkg/AmdSev: assign and reserve the Sev Secret area
>
>     -    Create a one page secret area in the MEMFD and protect the area with a
>     +    Create a one page secret area in the MEMFD and reserve the area with a
>          boot time HOB.
>
>          Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077
>          Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>          Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>          Message-Id: <20201130202819.3910-6-jejb@linux.ibm.com>
>     +    Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>     +    [lersek@redhat.com: s/protect/reserve/g in the commit message, at Ard's
>     +     and James's suggestion]
>
>      diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
>      --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
> 6:  6970b9413c93 ! 6:  ea823d078162 OvmfPkg/AmdSev: Expose the Sev Secret area using a configuration table
>     @@ -11,6 +11,8 @@
>          Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>          Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>          Message-Id: <20201130202819.3910-7-jejb@linux.ibm.com>
>     +    Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>     +    [lersek@redhat.com: fix indentation of InstallConfigurationTable() args]
>
>      diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
>      --- a/OvmfPkg/OvmfPkg.dec
>     @@ -152,7 +154,8 @@
>      +  IN EFI_SYSTEM_TABLE     *SystemTable
>      +  )
>      +{
>     -+  return gBS->InstallConfigurationTable (&gSevLaunchSecretGuid,
>     -+                                         &mSecretDxeTable
>     -+                                         );
>     ++  return gBS->InstallConfigurationTable (
>     ++                &gSevLaunchSecretGuid,
>     ++                &mSecretDxeTable
>     ++                );
>      +}

I meant to include this range-diff in the email where I'd confirm the
merge and the commit range; too bad I got distracted with this ECC mess.

Laszlo



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

Hi James,

On 12/04/20 03:01, Laszlo Ersek wrote:
> On 12/04/20 02:55, Laszlo Ersek wrote:
>
>> I will send a short patch series to add the exceptions, and once
>> that's upstream, we *will* merge this (v3) series.
>
> BTW the tweaks I added on top of your v3, in
> <https://github.com/tianocore/edk2/pull/1175>, are as follows (git
> range-diff output):
>
>> 1:  4020c20b2342 ! 1:  b96494ad75db OvmfPkg/ResetVector: convert SEV-ES Reset Block structure to be GUIDed
>>     @@ -8,8 +8,9 @@
>>
>>          Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077
>>          Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>>     -
>>          Message-Id: <20201130202819.3910-2-jejb@linux.ibm.com>
>>     +    Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>     +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>>      diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
>>      --- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
>> 2:  488fbdbe7689 ! 2:  acc8cb13da8d OvmfPkg/Amdsev: Base commit to build encrypted boot specific OVMF
>>     @@ -11,8 +11,9 @@
>>
>>          Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077
>>          Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>>     -
>>          Message-Id: <20201130202819.3910-3-jejb@linux.ibm.com>
>>     +    Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>     +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>>      diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
>>      new file mode 100644
>> 3:  796ec96e3414 ! 3:  b80ce0838781 OvmfPkg/AmdSev: add Grub Firmware Volume Package
>>     @@ -19,8 +19,10 @@
>>
>>          Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077
>>          Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>>     -
>>          Message-Id: <20201130202819.3910-4-jejb@linux.ibm.com>
>>     +    Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>     +    [lersek@redhat.com: replace local variable initialization with assignment]
>>     +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>>      diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
>>      --- a/OvmfPkg/OvmfPkg.dec
>>     @@ -779,7 +781,9 @@
>>      +{
>>      +  EFI_HANDLE    Handle;
>>      +  EFI_STATUS    Status;
>>     -+  UINT16 FrontPageTimeout = 0;
>>     ++  UINT16        FrontPageTimeout;
>>     ++
>>     ++  FrontPageTimeout = 0;
>>      +
>>      +  DEBUG ((DEBUG_INFO, "PlatformBootManagerBeforeConsole\n"));
>>      +  InstallDevicePathCallback ();
>> 4:  d954947f8d14 ! 4:  f3cda3cadde4 OvmfPkg: create a SEV secret area in the AmdSev memfd
>>     @@ -10,8 +10,9 @@
>>          Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077
>>          Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>>          Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>     -
>>          Message-Id: <20201130202819.3910-5-jejb@linux.ibm.com>
>>     +    Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>     +    [lersek@redhat.com: fix typo in "ResetVectorVtf0.asm" comments]
>>
>>      diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
>>      --- a/OvmfPkg/OvmfPkg.dec
>>     @@ -52,7 +53,7 @@
>>      +;
>>      +; SEV Secret block
>>      +;
>>     -+; This describes the guest ram area where the hypervisor may should
>>     ++; This describes the guest ram area where the hypervisor should
>>      +; inject the secret.  The data format is:
>>      +;
>>      +; base physical address (32 bit word)
>> 5:  1a18c4921cdf ! 5:  c38b3caf22ad OvmfPkg/AmdSev: assign and protect the Sev Secret area
>>     @@ -1,14 +1,17 @@
>>      Author: James Bottomley <jejb@linux.ibm.com>
>>
>>     -    OvmfPkg/AmdSev: assign and protect the Sev Secret area
>>     +    OvmfPkg/AmdSev: assign and reserve the Sev Secret area
>>
>>     -    Create a one page secret area in the MEMFD and protect the area with a
>>     +    Create a one page secret area in the MEMFD and reserve the area with a
>>          boot time HOB.
>>
>>          Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077
>>          Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>>          Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>          Message-Id: <20201130202819.3910-6-jejb@linux.ibm.com>
>>     +    Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>     +    [lersek@redhat.com: s/protect/reserve/g in the commit message, at Ard's
>>     +     and James's suggestion]
>>
>>      diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
>>      --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
>> 6:  6970b9413c93 ! 6:  ea823d078162 OvmfPkg/AmdSev: Expose the Sev Secret area using a configuration table
>>     @@ -11,6 +11,8 @@
>>          Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>>          Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>          Message-Id: <20201130202819.3910-7-jejb@linux.ibm.com>
>>     +    Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>     +    [lersek@redhat.com: fix indentation of InstallConfigurationTable() args]
>>
>>      diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
>>      --- a/OvmfPkg/OvmfPkg.dec
>>     @@ -152,7 +154,8 @@
>>      +  IN EFI_SYSTEM_TABLE     *SystemTable
>>      +  )
>>      +{
>>     -+  return gBS->InstallConfigurationTable (&gSevLaunchSecretGuid,
>>     -+                                         &mSecretDxeTable
>>     -+                                         );
>>     ++  return gBS->InstallConfigurationTable (
>>     ++                &gSevLaunchSecretGuid,
>>     ++                &mSecretDxeTable
>>     ++                );
>>      +}
>
> I meant to include this range-diff in the email where I'd confirm the
> merge and the commit range; too bad I got distracted with this ECC mess.

Additional updates (expressed incrementally), per prior discussion:

> 1:  b96494ad75db = 1:  11f014f9a5a5 OvmfPkg/ResetVector: convert SEV-ES Reset Block structure to be GUIDed
> 2:  acc8cb13da8d = 2:  ac3e7f9e93ab OvmfPkg/Amdsev: Base commit to build encrypted boot specific OVMF
> 3:  b80ce0838781 ! 3:  da5e1715a902 OvmfPkg/AmdSev: add Grub Firmware Volume Package
>     @@ -23,6 +23,10 @@
>          Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>          [lersek@redhat.com: replace local variable initialization with assignment]
>          Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>     +    [lersek@redhat.com: squash 'OvmfPkg: add "gGrubFileGuid=Grub" to
>     +     GuidCheck.IgnoreDuplicates', reviewed stand-alone by Phil (msgid
>     +     <e6eae551-8563-ccfb-5547-7a97da6d46e5@redhat.com>) and Ard (msgid
>     +     <10aeda37-def6-d9a4-6e02-4c66c1492f57@arm.com>)]
>
>      diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
>      --- a/OvmfPkg/OvmfPkg.dec
>     @@ -2327,3 +2331,16 @@
>      +
>      +remove_efi=0
>      +echo "grub.efi generated in ${basedir}"
>     +
>     +diff --git a/OvmfPkg/OvmfPkg.ci.yaml b/OvmfPkg/OvmfPkg.ci.yaml
>     +--- a/OvmfPkg/OvmfPkg.ci.yaml
>     ++++ b/OvmfPkg/OvmfPkg.ci.yaml
>     +@@
>     +         "IgnoreGuidName": ["ResetVector", "XenResetVector"], # Expected duplication for gEfiFirmwareVolumeTopFileGuid
>     +         "IgnoreGuidValue": [],
>     +         "IgnoreFoldersAndFiles": [],
>     +-        "IgnoreDuplicates": [],
>     ++        "IgnoreDuplicates": ["gGrubFileGuid=Grub"],
>     +     },
>     +
>     +     ## options defined .pytool/Plugin/LibraryClassCheck
> 4:  f3cda3cadde4 = 4:  9caed44db39b OvmfPkg: create a SEV secret area in the AmdSev memfd
> 5:  c38b3caf22ad = 5:  dbba0abc831f OvmfPkg/AmdSev: assign and reserve the Sev Secret area
> 6:  ea823d078162 = 6:  0612c2ecdc77 OvmfPkg/AmdSev: Expose the Sev Secret area using a configuration table

Merged as commit range ef3e73c6a0c0..01726b6d23d4, via the same PR:
<https://github.com/tianocore/edk2/pull/1175>.

Please proceed with addressing Jiewen's feedback, with further patches.

Thanks!
Laszlo



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

On 12/14/20 20:57, Laszlo Ersek wrote:

> Merged as commit range ef3e73c6a0c0..01726b6d23d4, via the same PR:
> <https://github.com/tianocore/edk2/pull/1175>.

I've listed <https://bugzilla.tianocore.org/show_bug.cgi?id=3077> under
<https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning#edk2-stable202102-tag-planning>.

Thanks
Laszlo



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

On 11/30/20 21:28, James Bottomley wrote:
> v3:
> 
> - More grub and boot stripping (I think I got everything out, but
>   there may be something that strayed in the boot panic resolution).
> - grub.sh tidy up with tabs->spaces.
> - Move the reset vector GUIDisation patch to the front so it can be
>   applied independently
> - Update the .dsc and .fdf files for variable policy
> 
> v2:
> 
> - Strip more out of AmdSev image (networking, secure boot, smm)
> - give sev reset block a generic table guid and use it for boot secret area
> - separate secret patches and make grub script more robust
> - Add copyrights and fix formatting issues
> 
> v1:
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077
> 
> This patch series is modelled on the structure of the Bhyve patches
> for Ovmf, since it does somewhat similar things.  This patch series
> creates a separate build for an AmdSev OVMF.fd that does nothing
> except combine with grub and boot straight through the internal grub
> to try to mount an encrypted volume.
> 
> Concept: SEV Secure Encrypted Images
> ====================================
> 
> The SEV patches in Linux and OVMF allow for the booting of SEV VMs in
> an encrypted state, but don't really show how this could be done with
> an encrypted image.  Since the key used to decrypt the image must be
> maintained within the SEV encryption envelope, encrypted QCOW is not
> an option because the key would then have to be known to QEMU which is
> outside the encryption envelope.  The proposal here is that an
> encrypted image should be a QCOW image consisting of two partitions,
> the normal unencrypted EFI partition (Identifying it as an OVMF
> bootable image) and a luks encrypted root partition.  The kernel would
> be inside the encrypted root in the /boot directory.  The secret
> injected securely through QEMU is extracted by OVMF and passed to grub
> which uses it to mount the encrypted root and boot the kernel
> normally.  The creator of the secret bundle must be satisfied with the
> SEV attestation before the secret is constructed.  Unfortunately, the
> SEV attestation can only be on the first QEMU firmware volume and
> nothing else, so this patch series builds grub itself into a firmware
> volume and places it inside OVMF so that the entire boot system can be
> attested.  In a normal OVMF KVM system, the variable store is on the
> second flash volume (which is read/write).  Unfortunately, this
> mutable configuration provided by the variables is outside the
> attestation envelope and can significantly alter the boot path,
> possibly leading to secret leak, so encrypted image boot should only
> be done with the OVMF.fd that combines both the code and variables.
> the OVMF.fd is constructed so that it becomes impossible to interrupt
> the boot sequence after attestation and the system will either boot
> the image or fail. The boot sequence runs the grub.efi embedded in the
> OVMF firmware volume so the encrypted image owner knows their own
> version of grub is the only one that will boot before injecting the
> secret.  Note this boot path actually ignores the unencrypted EFI
> partition.  However, as part of this design, the encrypted image may be
> booted by a standard OVMF KVM boot and in that case, the user will
> have to type the encryption password.  This standard boot will be
> insecure but it might be used by the constructor of the encrypted
> images on their own private laptop, for instance.  The standard boot
> path will use the unencrypted EFI partition.
> 
> Patches Required Outside of OVMF
> ================================
> 
> There is a patch set to grub which allows it to extract the SEV secret
> area from the configuration table and use the secret as a password to
> do a luks crypto mount of root (this is the sevsecret grub module):
> 
> https://lists.gnu.org/archive/html/grub-devel/2020-11/msg00078.html
> 
> There is also a patch to qemu which allows it to search through the
> OVMF.fd and find the SEV secret area which is now described inside the
> Reset Vector using the existing SEV_ES reset block.  This area is the
> place QEMU will inject the encrypted SEV secret bundle.
> 
> Security of the System
> ======================
> 
> Since Grub is now part of the attested OVMF.fd bundle, the VM owner
> knows absolutely that it will proceed straight to partition decryption
> inside the attested code and boot the kernel off the encrypted
> partition.  Even if a different QCOW image is substituted, the boot
> will fail without revealing the secret because the system is designed
> to fail hard in that case and because the secret is always contained
> within the encrypted envelope it should be impossible for the cloud
> operator to obtain it even if they can pause the boot and examine the
> machine memory.
> 
> Putting it All Together
> =======================
> 
> This is somewhat hard.  You must first understand how to boot a QEMU
> system so as to have the VM pause after firmware loading (-S option)
> and use the qmp port to request an attestation.  Only if the
> attestation corresponds to the expected sha256sum of OVMF.fd should
> the secret bundle be constructed and injected using qmp.  The tools
> for constructing the secret bundle are in
> 
> https://github.com/AMDESE/sev-tool/
> 
> James
> 
> ---
> 
> James Bottomley (6):
>   OvmfPkg/ResetVector: convert SEV-ES Reset Block structure to be GUIDed
>   OvmfPkg/Amdsev: Base commit to build encrypted boot specific OVMF
>   OvmfPkg/AmdSev: add Grub Firmware Volume Package
>   OvmfPkg: create a SEV secret area in the AmdSev memfd
>   OvmfPkg/AmdSev: assign and protect the Sev Secret area
>   OvmfPkg/AmdSev: Expose the Sev Secret area using a configuration table
> 
>  OvmfPkg/OvmfPkg.dec                           |    8 +
>  OvmfPkg/AmdSev/AmdSevX64.dsc                  |  844 ++++++++++
>  OvmfPkg/AmdSev/AmdSevX64.fdf                  |  456 +++++
>  OvmfPkg/AmdSev/Grub/Grub.inf                  |   39 +
>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf        |   37 +
>  OvmfPkg/AmdSev/SecretPei/SecretPei.inf        |   35 +
>  .../PlatformBootManagerLibGrub.inf            |   71 +
>  OvmfPkg/ResetVector/ResetVector.inf           |    4 +
>  OvmfPkg/Include/Guid/SevLaunchSecret.h        |   28 +
>  .../PlatformBootManagerLibGrub/BdsPlatform.h  |  175 ++
>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.c          |   26 +
>  OvmfPkg/AmdSev/SecretPei/SecretPei.c          |   25 +
>  .../PlatformBootManagerLibGrub/BdsPlatform.c  | 1482 +++++++++++++++++
>  .../PlatformBootManagerLibGrub/PlatformData.c |  214 +++
>  OvmfPkg/AmdSev/Grub/.gitignore                |    1 +
>  OvmfPkg/AmdSev/Grub/grub.cfg                  |   46 +
>  OvmfPkg/AmdSev/Grub/grub.sh                   |   93 ++
>  OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm  |   70 +-
>  OvmfPkg/ResetVector/ResetVector.nasmb         |    2 +
>  19 files changed, 3645 insertions(+), 11 deletions(-)
>  create mode 100644 OvmfPkg/AmdSev/AmdSevX64.dsc
>  create mode 100644 OvmfPkg/AmdSev/AmdSevX64.fdf
>  create mode 100644 OvmfPkg/AmdSev/Grub/Grub.inf
>  create mode 100644 OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
>  create mode 100644 OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>  create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf
>  create mode 100644 OvmfPkg/Include/Guid/SevLaunchSecret.h
>  create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h
>  create mode 100644 OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
>  create mode 100644 OvmfPkg/AmdSev/SecretPei/SecretPei.c
>  create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c
>  create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformData.c
>  create mode 100644 OvmfPkg/AmdSev/Grub/.gitignore
>  create mode 100644 OvmfPkg/AmdSev/Grub/grub.cfg
>  create mode 100644 OvmfPkg/AmdSev/Grub/grub.sh
> 

Confirming that I got this; I'll need some time to get to it.

Thanks
Laszlo



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