Re: [PATCH v2 2/6] OvmfPkg/AmdSev: add Grub Firmware Volume Package

["Laszlo Ersek" <lersek@redhat.com>]

Hi James,

On 11/20/20 19:45, James Bottomley wrote:
> This is used to package up the grub bootloader into a firmware volume
> where it can be executed as a shell like the UEFI Shell.  Grub itself
> is built as a minimal entity into a Fv and then added as a boot
> option.  By default the UEFI shell isn't built but for debugging
> purposes it can be enabled and will then be presented as a boot option
> (This should never be allowed for secure boot in an external data
> centre but may be useful for local debugging).  Finally all other boot
> options except grub and possibly the shell are stripped and the boot
> timeout forced to 0 so the system will not enter a setup menu and will
> only boot to grub.  This is done by copying the
> Library/PlatformBootManagerLib into Library/PlatformBootManagerLibGrub
> and then customizing it.
>
> Boot failure is fatal to try to prevent secret theft.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>
> ---
>
> v2: strip out s3 and qemu boot contacts make grub script robust and
>     don't build grub.efi each time
> ---
>  OvmfPkg/OvmfPkg.dec                           |    1 +
>  OvmfPkg/AmdSev/AmdSevX64.dsc                  |   21 +-
>  OvmfPkg/AmdSev/AmdSevX64.fdf                  |    7 +-
>  OvmfPkg/AmdSev/Grub/Grub.inf                  |   37 +
>  .../PlatformBootManagerLibGrub.inf            |   79 +
>  .../PlatformBootManagerLibGrub/BdsPlatform.h  |  175 ++
>  .../PlatformBootManagerLibGrub/BdsPlatform.c  | 1483 +++++++++++++++++
>  .../PlatformBootManagerLibGrub/PlatformData.c |  213 +++
>  OvmfPkg/AmdSev/Grub/.gitignore                |    1 +
>  OvmfPkg/AmdSev/Grub/grub.cfg                  |   46 +
>  OvmfPkg/AmdSev/Grub/grub.sh                   |   92 +
>  11 files changed, 2146 insertions(+), 9 deletions(-)
>  create mode 100644 OvmfPkg/AmdSev/Grub/Grub.inf
>  create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf
>  create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h
>  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

In advance, a shortcut: my previous review was at [a].

[a] https://edk2.groups.io/g/devel/message/67618
    https://www.redhat.com/archives/edk2-devel-archive/2020-November/msg00778.html


> diff --git a/OvmfPkg/AmdSev/Grub/Grub.inf b/OvmfPkg/AmdSev/Grub/Grub.inf
> new file mode 100644
> index 000000000000..211e7b8b2be6
> --- /dev/null
> +++ b/OvmfPkg/AmdSev/Grub/Grub.inf
> @@ -0,0 +1,37 @@
> +##  @file
> +#  Create a Firmware Volume based Grub Bootloaded

(1) typo -- should be "Bootloader", I believe (apologies for missing it
in v1)


> +#
> +#  Copyright (C) 2020 James Bottomley, IBM Corporation.
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010006
> +  BASE_NAME                      = Grub
> +  # This is gGrubFileGuid
> +  FILE_GUID                      = b5ae312c-bc8a-43b1-9c62-ebb826dd5d07
> +  MODULE_TYPE                    = UEFI_APPLICATION
> +  VERSION_STRING                 = 1.0
> +  ENTRY_POINT                    = UefiMain
> +
> +[Packages]
> +  OvmfPkg/OvmfPkg.dec
> +
> +#
> +# The following information is for reference only and not required by
> +# the build tools.
> +#
> +#  VALID_ARCHITECTURES           = X64
> +#
> +
> +##
> +# Note: The version of grub.efi this picks up can be generated by
> +# grub.sh which must be specified as a PREBUILD in the .dsc file or
> +# you can simply move a precompiled grub into here and not do the
> +# PREBUILD)

(2) This part of the patch looks unchanged, and I asked for more
explanation in [a](3) -- "Can you elaborate how to skip PREBUILD"?

The comment in the patch remains the same, and (AFAICT) I have not
received a response within the v1 thread either. (Instead you stated "I
did everything except [...].)

So now I'm not sure... Did you miss [a](3), or did you decide it was not
important enough to discuss / address?


> +##
> +[Binaries]
> +   PE32|grub.efi|*
> +
> diff --git a/OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf b/OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf
> new file mode 100644
> index 000000000000..a997d7586e6a
> --- /dev/null
> +++ b/OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf
> @@ -0,0 +1,79 @@
> +## @file
> +#  Platform BDS customizations library.
> +#
> +#  Copyright (c) 2007 - 2019, Intel Corporation. All rights reserved.<BR>
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##

(3) In the v1 review thread, under patch #1, I requested: "In every new
file created in this series, please prepend an IBM Copyright Notice, to
the original (C) notices (if any)."

  https://edk2.groups.io/g/devel/message/67615
  https://www.redhat.com/archives/edk2-devel-archive/2020-November/msg00775.html

This is a new file, but it still has no IBM (C) notice.


> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = PlatformBootManagerLibGrub
> +  FILE_GUID                      = 3a8f8431-f0c9-4c95-8a1d-04445c582d4e
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = PlatformBootManagerLib|DXE_DRIVER
> +
> +#
> +# The following information is for reference only and not required by the build tools.
> +#
> +#  VALID_ARCHITECTURES           = X64
> +#
> +
> +[Sources]
> +  BdsPlatform.c
> +  PlatformData.c
> +  BdsPlatform.h
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  SourceLevelDebugPkg/SourceLevelDebugPkg.dec
> +  OvmfPkg/OvmfPkg.dec
> +  SecurityPkg/SecurityPkg.dec
> +  ShellPkg/ShellPkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  MemoryAllocationLib
> +  UefiBootServicesTableLib
> +  UefiRuntimeServicesTableLib
> +  BaseMemoryLib
> +  DebugLib
> +  PcdLib
> +  UefiBootManagerLib
> +  BootLogoLib
> +  DevicePathLib
> +  PciLib
> +  ReportStatusCodeLib
> +  UefiLib
> +  PlatformBmPrintScLib
> +  Tcg2PhysicalPresenceLib
> +  XenPlatformLib

(4) I suggested removing XenPlatformLib too, in [a](4).

Did you run into build problems with that?

Replacing the XenDetected() calls in "BdsPlatform.c" with constant
FALSE, and simplifying the resultant code, should suffice.


> +
> +[Pcd]
> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable

(5) Thanks for removing PcdEmuVariableEvent.

But, PcdOvmfFlashVariablesEnable is just as superfluous; the INF file
now lists it without any references to the PCD in the code.


In [a](5) I requested the removal of everything in this INF file that
was not required for the desired semantics (i.e., for unconditionally
booting the builtin GRUB binary).

So again I'm unsure if you missed my feedback, or thought it was not
important. (I didn't get a request for clarification either.)

Keeping INF files minimal is relevant for future contributions. We
frequently need to determine the set of modules that depend on a
particular PCD. If some INF files list PCDs unjustifiedly, then the
affected module set may appear larger than it really is.

This applies to all sections of the INF file, not just [Pcd].


> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
> +  gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut
> +  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate         ## CONSUMES
> +  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits         ## CONSUMES
> +  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity           ## CONSUMES
> +  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits         ## CONSUMES
> +
> +[Pcd.IA32, Pcd.X64]
> +  gEfiMdePkgTokenSpaceGuid.PcdFSBClock
> +
> +[Protocols]
> +  gEfiDecompressProtocolGuid
> +  gEfiPciRootBridgeIoProtocolGuid
> +  gEfiS3SaveStateProtocolGuid                   # PROTOCOL SOMETIMES_CONSUMED
> +  gEfiDxeSmmReadyToLockProtocolGuid             # PROTOCOL SOMETIMES_PRODUCED
> +  gEfiLoadedImageProtocolGuid                   # PROTOCOL SOMETIMES_PRODUCED
> +  gEfiFirmwareVolume2ProtocolGuid               # PROTOCOL SOMETIMES_CONSUMED
> +
> +[Guids]
> +  gEfiEndOfDxeEventGroupGuid
> +  gEfiGlobalVariableGuid
> +  gRootBridgesConnectedEventGroupGuid
> +  gUefiShellFileGuid
> +  gGrubFileGuid
> diff --git a/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h b/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h
> new file mode 100644
> index 000000000000..a7fc4dbc3c1f
> --- /dev/null
> +++ b/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h
> @@ -0,0 +1,175 @@
> +/** @file
> +  Platform BDS customizations include file.
> +
> +  Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent

(6) Same as (3) -- missing IBM (C) on new file


> diff --git a/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c
> new file mode 100644
> index 000000000000..4fb2f904a10e
> --- /dev/null
> +++ b/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c
> @@ -0,0 +1,1483 @@
> +/** @file
> +  Platform BDS customizations.
> +
> +  Copyright (c) 2004 - 2019, Intel Corporation. All rights reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent

(7) same as (3) -- missing IBM (C) on new file


> +/**
> +  The function is called when no boot option could be launched,
> +  including platform recovery options and options pointing to applications
> +  built into firmware volumes.
> +
> +  If this function returns, BDS attempts to enter an infinite loop.
> +**/
> +VOID
> +EFIAPI
> +PlatformBootManagerUnableToBoot (
> +  VOID
> +  )
> +{
> +  //
> +  // If we get here something failed about the grub boot but since
> +  // We're privy to the secret we must panic and not retry or loop
> +  //
> +  CpuDeadLoop ();
> +}

(8) In [a](7) I meant that both ASSERT() and CpuDeadLoop() should be
called. In DEBUG and NOOPT builds, the ASSERT() fires, produces a
somewhat helpful debug message, and the CpuDeadLoop() is not reached
(ASSERT() may call CpuDeadLoop() internally, or raise an exception). In
RELEASE builds, the message is not produced, but we still won't proceed
past the CpuDeadLoop().

Sorry about not being clear enough.


> diff --git a/OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformData.c b/OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformData.c
> new file mode 100644
> index 000000000000..2858c3dfd5ca
> --- /dev/null
> +++ b/OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformData.c
> @@ -0,0 +1,213 @@
> +/** @file
> +  Defined the platform specific device path which will be used by
> +  platform Bbd to perform the platform policy connect.
> +
> +  Copyright (c) 2004 - 2017, Intel Corporation. All rights reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent

(9) same as (3) -- missing IBM (C) on new file


> diff --git a/OvmfPkg/AmdSev/Grub/grub.sh b/OvmfPkg/AmdSev/Grub/grub.sh
> new file mode 100644
> index 000000000000..fd28dc6ab274
> --- /dev/null
> +++ b/OvmfPkg/AmdSev/Grub/grub.sh
> @@ -0,0 +1,92 @@
> +##  @file
> +#  Build a version of grub capable of decrypting a luks volume with a SEV
> +#  Supplied secret
> +#
> +#  Copyright (C) 2020 James Bottomley, IBM Corporation.
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +set -e
> +remove_efi=1
> +
> +cleanup()  {
> +    # remove the intermediates
> +    for f in disk.fat grub-bootstrap.cfg; do
> +	rm -f "${basedir}/$f"
> +    done
> +    if [ $remove_efi -eq 1 ]; then
> +	rm -f "${basedir}/grub.efi"
> +    fi
> +}

(10) These two "rm -f" commands don't use the separator "--", unlike the
"rm -f" command below (which has been updated, thanks for that).


> +
> +trap cleanup EXIT
> +
> +GRUB_MODULES="
> +	    part_msdos
> +	    part_gpt
> +	    cryptodisk
> +	    luks
> +	    gcry_rijndael
> +	    gcry_sha256
> +	    ext2
> +	    btrfs
> +	    xfs
> +	    fat
> +	    configfile
> +	    memdisk
> +	    sleep
> +	    normal
> +	    echo
> +	    test
> +	    regexp
> +	    linux
> +	    linuxefi
> +	    reboot
> +	    sevsecret
> +	    "
> +basedir=$(dirname -- "$0")
> +
> +# don't run a build if grub.efi exists and is newer than the config files
> +if [ -e "${basedir}/grub.efi" -a \
> +     "${basedir}/grub.efi" -nt "${basedir}/grub.cfg" -a \
> +     "${basedir}/grub.efi" -nt "${basedir}/grub.sh" ]; then
> +    remove_efi=0
> +    echo "preserving existing grub.efi"

arguably this should go to stderr as well, but I don't insist

> +    exit 0
> +fi
>

Ah, OK. This sort of addresses my question (2). Not entirely -- the
comment at (2) implies the user is somehow responsible for skipping
PREBUILD. While in reality, PREBUILD will be launched, it will just exit
early.

(11) Independently, "-a" is considered obsolescent, per
<https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html#tag_20_128_16>.

Anyway, feel free to ignore.


> +##
> +# different distributions have different names for grub-mkimage, so
> +# search all the known ones
> +##
> +for b in grub2-mkimage grub-mkimage; do
> +    if which "$b" > /dev/null 2>&1; then
> +	mkimage="$b"
> +	break;

(12) "muscle memory" semicolon after "break" :)


(13) the indentation seems strange (Linux kernel style?); please don't
use hardware tabs. (Hmmm, applies to other parts of this script too.)


> +    fi
> +done
> +if [ -z "$mkimage" ]; then
> +    echo "Can't find grub mkimage" >&2
> +    exit 1
> +fi

(14) The variable "mkimage" has not been emptied before the loop, like I
asked in [a](14).

The reason I had asked for emptying "mkimage" was two-fold: (i) I
considered that maybe you'd add "set -u" at the top of the script as
well, in which case the above "if" could fail ungracefully with "unbound
variable", (ii) "mkimage" is not a totally uncommon variable name, and
it could be inherited from the calling shell environment -- in case the
loop never matched.

Now, I may have been wrong in that reasoning. But please, if you think
I'm wrong, say so. I think it's quite possible to convince me. If you
simply ignore a request from me, that's not a good use of my time.

Whenever I review the next version of a series, I go over every single
request I made under the last one, and verify whether it has been
addressed, or explicitly refuted in the discussion. If neither happens,
then I just don't know what to do, as I obviously have to make the exact
same request in the next round of review. It's disappointing, and
discourages me from continuing the review.


> +
> +# GRUB's rescue parser doesn't understand 'if'.
> +echo 'normal (memdisk)/grub.cfg' > "${basedir}/grub-bootstrap.cfg"
> +
> +# Now build a memdisk with the correct grub.cfg
> +rm -f -- "${basedir}/disk.fat"
> +mkfs.msdos -C -- "${basedir}/disk.fat" 64
> +mcopy -i "${basedir}/disk.fat" -- "${basedir}/grub.cfg" ::grub.cfg
> +
> +
> +${mkimage} -O x86_64-efi \
> +	   -p '(crypto0)' \
> +	   -c "${basedir}/grub-bootstrap.cfg" \
> +	   -m "${basedir}/disk.fat" \
> +	   -o "${basedir}/grub.efi" \
> +	   ${GRUB_MODULES}
> +
> +remove_efi=0
> +echo "grub.efi generated in ${basedir}"
>

Thanks
Laszlo