[edk2-devel] [PATCH 1/1] OvmfPkg/AmdSev: introduce EMBED_GRUB=FALSE to skip including Grub image

[Dov Murik]

On 07/07/2021 13:51, James Bottomley wrote:
> On Wed, 2021-07-07 at 10:42 +0000, Dov Murik wrote:
>> The AmdSevX64 target includes an embedded Grub image to support
>> secure
>> (measured) boot of confidential guests with encrypted root images.
>>
>> However, it is sometimes convenient to build this target without an
>> embedded Grub.  We introduce the EMBED_GRUB setting (defaults to
>> TRUE),
>> which conditions the generation (grub.sh) and inclusion of the Grub
>> image.  Now building AmdSevX64 with -DEMBED_GRUB=FALSE allows it.
>>
>> Cc: Laszlo Ersek <lersek at redhat.com>
>> Cc: Ard Biesheuvel <ardb+tianocore at kernel.org>
>> Cc: Jordan Justen <jordan.l.justen at intel.com>
>> Cc: Ashish Kalra <ashish.kalra at amd.com>
>> Cc: Brijesh Singh <brijesh.singh at amd.com>
>> Cc: Erdem Aktas <erdemaktas at google.com>
>> Cc: James Bottomley <jejb at linux.ibm.com>
>> Cc: Jiewen Yao <jiewen.yao at intel.com>
>> Cc: Min Xu <min.m.xu at intel.com>
>> Cc: Tom Lendacky <thomas.lendacky at amd.com>
>> Cc: Tobin Feldman-Fitzthum <tobin at linux.ibm.com>
>> Signed-off-by: Dov Murik <dovmurik at linux.ibm.com>
>> ---
>>  OvmfPkg/AmdSev/AmdSevX64.dsc | 16 +++++++++++++++-
>>  OvmfPkg/AmdSev/AmdSevX64.fdf |  2 ++
>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc
>> b/OvmfPkg/AmdSev/AmdSevX64.dsc
>> index 1d487befae08..ba7d6fe6b749 100644
>> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
>> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
>> @@ -25,7 +25,6 @@ [Defines]
>>    BUILD_TARGETS                  = NOOPT|DEBUG|RELEASE
>>    SKUID_IDENTIFIER               = DEFAULT
>>    FLASH_DEFINITION               = OvmfPkg/AmdSev/AmdSevX64.fdf
>> -  PREBUILD                       = sh OvmfPkg/AmdSev/Grub/grub.sh
>>  
>>    #
>>    # Defines for default states.  These can be changed on the command
>> line.
>> @@ -40,6 +39,19 @@ [Defines]
>>    #
>>    DEFINE BUILD_SHELL             = FALSE
>>  
>> +  #
>> +  # Embed Grub into the OVMF image so they are measured together
>> when launching
>> +  # confidential guest
>> +  #
>> +  DEFINE EMBED_GRUB              = TRUE
>> +
>> +!if $(EMBED_GRUB) == TRUE
>> +  #
>> +  # This step builds the grub.efi binary image if needed
>> +  #
>> +  PREBUILD                       = sh OvmfPkg/AmdSev/Grub/grub.sh
>> +!endif
>> +
>>    #
>>    # Device drivers
>>    #
>> @@ -784,7 +796,9 @@ [Components]
>>    }
>>  !endif
>>    OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
>> +!if $(EMBED_GRUB) == TRUE
>>    OvmfPkg/AmdSev/Grub/Grub.inf
>> +!endif
>>  !if $(BUILD_SHELL) == TRUE
>>    ShellPkg/Application/Shell/Shell.inf {
>>      <LibraryClasses>
>> diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf
>> b/OvmfPkg/AmdSev/AmdSevX64.fdf
>> index 9977b0f00a18..ee3d96bb813f 100644
>> --- a/OvmfPkg/AmdSev/AmdSevX64.fdf
>> +++ b/OvmfPkg/AmdSev/AmdSevX64.fdf
>> @@ -270,7 +270,9 @@ [FV.DXEFV]
>>  INF  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellC
>> ommand.inf
>>  !endif
>>  INF OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
>> +!if $(EMBED_GRUB) == TRUE
>>  INF  OvmfPkg/AmdSev/Grub/Grub.inf
>> +!endif
>>  !if $(BUILD_SHELL) == TRUE
>>  INF  ShellPkg/Application/Shell/Shell.inf
>>  !endif
> 
> This likely isn't enough: the boot pathway of the AmdSev package is
> stripped down and designed to fail if grub won't boot, so if you set
> EMBED_GRUB = false, you'll likely build a system that won't boot.  This
> would still work for the Kata use case, if the kernel and initrd are
> plumbed back in, but it won't work for the generic use case. 

You're right.  That's why I left the default as EMBED_GRUB=TRUE.  And
for Kata (that is, with the edk2 patches from the "Measured SEV boot
with kernel/initrd/cmdline​" series) we'll build with EMBED_GRUB=FALSE
because we don't have an encrypted disk there.


Note that currently it's a bit difficult to build with EMBED_GRUB=TRUE
because it expects to run grub-mkimage and include modules which are not
upstream; so for most cases user needs to prepare a special grub dir
with modules etc and also make sure that that is the one used in
grub.sh.  (To ease that pain maybe we can include grub as a git
submodule or something like that, and point it to a revision with the
modules we require.  But this might cause other dependency pain which I
might not think of.)



> I think
> the change log needs to describe the use cases so we don't end up
> getting a load of annoyed people building systems that won't work for
> them.

Yes.  Is there documentation for the different -D settings somewhere, or
should I add it to the .dsc file (and the commit log) ?



> 
> There's also the broader question of whether this should all be
> integrated back into OvmfX64Pkg with more determination done at
> runtime, so we can build fewer separate binaries?
> 

I'm not sure about merging in OvmfX64Pkg, but for AmdSevX64 if we build
with embedded grub then determination *is* done at runtime: if there's a
-kernel fw_cfg entry and all the measured hashes are OK, then it boots
via -kernel/-initrd; otherwise, it'll go to grub and try to boot from
encrypted disk.

-Dov


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77556): https://edk2.groups.io/g/devel/message/77556
Mute This Topic: https://groups.io/mt/84041501/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-