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

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

[Dov Murik]

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@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>
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
Signed-off-by: Dov Murik <dovmurik@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/LinuxInitrdDynamicShellCommand.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
-- 
2.25.1

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

[James Bottomley]

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@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>
> Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
> Signed-off-by: Dov Murik <dovmurik@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.  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.

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?

James

Re: [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@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>
>> Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
>> Signed-off-by: Dov Murik <dovmurik@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