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


Laszlo Ersek
 

On 11/12/20 01:13, 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 preven secret theft.

Signed-off-by: James Bottomley <jejb@...>
---
OvmfPkg/OvmfPkg.dec | 1 +
OvmfPkg/AmdSev/AmdSevX64.dsc | 14 +-
OvmfPkg/AmdSev/AmdSevX64.fdf | 5 +-
OvmfPkg/AmdSev/Grub/Grub.inf | 37 +
.../PlatformBootManagerLibGrub.inf | 84 +
.../PlatformBootManagerLibGrub/BdsPlatform.h | 179 ++
.../PlatformBootManagerLibGrub/BdsPlatform.c | 1538 +++++++++++++++++
.../PlatformBootManagerLibGrub/PlatformData.c | 213 +++
OvmfPkg/AmdSev/Grub/.gitignore | 1 +
OvmfPkg/AmdSev/Grub/grub.cfg | 35 +
OvmfPkg/AmdSev/Grub/grub.sh | 54 +
11 files changed, 2157 insertions(+), 4 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
diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index d1dfb8742f..7d3663150e 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -23,6 +23,7 @@
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.
@@ -33,6 +34,7 @@
DEFINE SOURCE_DEBUG_ENABLE = FALSE
DEFINE TPM_ENABLE = FALSE
DEFINE TPM_CONFIG_ENABLE = FALSE
+ DEFINE BUILD_SHELL = FALSE
(1) Please add a comment that this is strictly for debugging (not
production).


diff --git a/OvmfPkg/AmdSev/Grub/Grub.inf b/OvmfPkg/AmdSev/Grub/Grub.inf
new file mode 100644
index 0000000000..a12428668b
--- /dev/null
+++ b/OvmfPkg/AmdSev/Grub/Grub.inf
@@ -0,0 +1,37 @@
+## @file
+# Create a Firmware Volume based Grub Bootloaded
+#
+# 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 = IA32 X64 EBC
(2) We can likely restrict this comment to X64 only.


+#
+
+##
+# 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)
(3) Can you elaborate how to skip PREBUILD?


diff --git a/OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf b/OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf
new file mode 100644
index 0000000000..62707b0bdd
--- /dev/null
+++ b/OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf
@@ -0,0 +1,84 @@
+## @file
+# Platform BDS customizations library.
+#
+# Copyright (c) 2007 - 2019, Intel Corporation. All rights reserved.<BR>
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[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 = IA32 X64 EBC
+#
+
+[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
+ QemuFwCfgLib
+ QemuFwCfgS3Lib
+ QemuLoadImageLib
+ QemuBootOrderLib
+ ReportStatusCodeLib
+ UefiLib
+ PlatformBmPrintScLib
+ Tcg2PhysicalPresenceLib
+ XenPlatformLib
(4) I recommend removing the following lib classes:

- QemuFwCfgLib
- QemuFwCfgS3Lib
- QemuLoadImageLib
- QemuBootOrderLib
- XenPlatformLib

The parallel

#include <Library/...>

directives should be removed from the
"OvmfPkg/Library/PlatformBootManagerLibGrub/" source code.

This will cause a bunch of compilation errors. The code should then be
simplified by either just removing function calls, or assuming suitable
constant return values from those function calls.


+
+[Pcd]
+ gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent
(5) This PCD not needed.

In general, please remove *everything* from this INF file that is not
strictly necessary for the desired semantics.

While I *am* a neat freak, eliminating dependencies on hypervisor
information channels at this granularity helps reasoning about security,
in this particular case (in my opinion).

If there is build breakage that's not obvious to fix, I'm happy to
advise to the best of my knowledge.


diff --git a/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c
new file mode 100644
index 0000000000..24c37068a2
--- /dev/null
+++ b/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c
+/**
+ Remove all MemoryMapped(...)/FvFile(...) and Fv(...)/FvFile(...) boot options
+ whose device paths do not resolve exactly to an FvFile in the system.
+
+ Also strip out every boot option that is not an FvFile, meaning the system
+ can only boot either the Grub or (if built) the shell.
+
+ This removes any boot options that point to binaries built into the firmware
+ and have become stale due to any of the following:
+ - DXEFV's base address or size changed (historical),
+ - DXEFV's FvNameGuid changed,
+ - the FILE_GUID of the pointed-to binary changed,
+ - the referenced binary is no longer built into the firmware.
+
+ EfiBootManagerFindLoadOption() used in PlatformRegisterFvBootOption() only
+ avoids exact duplicates.
+**/
+VOID
+RemoveStaleFvFileOptions (
+ VOID
+ )
(6) Do we need this function at all?

My understanding is:

- platform reboot is not supported,

- the original platform boot occurs with the varstore having been
verified by the remote guest owner

That seems to imply that we don't need to dynamically prune the boot
variables.

I don't insist of course, just looking for further simplifications. If
you think keeping this function (with the updates) is necessary or just
prudent, I'm OK with that.

... Ah wait, I see that the shell is added and *then* removed
(optionally). OK.


+/**
+ 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
+ //
+ ASSERT (FALSE);
+}
(7) I suggest adding an explicit CpuDeadLoop() here -- RELEASE builds do
not include ASSERT()s.


diff --git a/OvmfPkg/AmdSev/Grub/grub.cfg b/OvmfPkg/AmdSev/Grub/grub.cfg
new file mode 100644
index 0000000000..5c8fd1e547
--- /dev/null
+++ b/OvmfPkg/AmdSev/Grub/grub.cfg
@@ -0,0 +1,35 @@
(8) This file lacks a (C) Notice and an SPDX-License-Identifier.


+echo "Entering grub config"
+sevsecret
+if [ $? -ne 0 ]; then
+ echo "Failed to locate anything in the SEV secret area, prompting for password"
+ cryptomount -a
+else
+ cryptomount -s
+ if [ $? -ne 0 ]; then
+ echo "Failed to mount root securely, retrying with password prompt"
+ cryptomount -a
+ fi
+fi
+set root=
+for f in (crypto*); do
+ if [ -e $f/boot/grub/grub.cfg ]; then
+ set root=$f
+ set prefix=($root)/boot/grub
+ break;
+ fi
+done
+if [ x$root = x ]; then
+ echo "Failed to find any grub configuration on the encrypted volume"
+ sleep 5
+ reboot
+fi
+# rest of modules to get boot to work
+set modules="
+ boot
+ loadenv
+ "
+for f in $modules; do
+ insmod $f
+done
+echo "Transferring to ${prefix}/grub.cfg"
+source $prefix/grub.cfg
I'm not familiar with the grub config file syntax, so I'm not going to
comment on the (potential) lack of quoting.


diff --git a/OvmfPkg/AmdSev/Grub/grub.sh b/OvmfPkg/AmdSev/Grub/grub.sh
new file mode 100644
index 0000000000..91fac11ac9
--- /dev/null
+++ b/OvmfPkg/AmdSev/Grub/grub.sh
@@ -0,0 +1,54 @@
(9) This file lacks a (C) Notice and an SPDX-License-Identifier.


+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`
(10) I'm going to put on my "pedant hat" for this. Given that this
script runs every time (or "almost every time") the platform is built,
I'd like the script to be more robust.

I suggest:

basedir=$(dirname -- "$0")


+##
+# 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
(11) s/which/command -v/


+ mkimage=$b
(12) add a break here?


+ fi
+done
+if [ -z "$mkimage" ]; then
+ echo "Can't find grub mkimage"
(13) Please write this to stderr: >&2


(14) Please store "" (the empty string) to "mkimage" before the loop.


+ exit 1
+fi
+
+# 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
(15) rm -f -- "${basedir}/disk.fat"


+mkfs.msdos -C ${basedir}/disk.fat 64 || exit 1
(16) Please quote ${basedir}/disk.fat, and (if mkfs.msdos doesn't choke
on it) please place a -- separator after "-C".


(17) Please remove the explicit '|| exit 1' commands from the script,
and instead add "set -e" at the top.


(18) Please consider adding an EXIT trap.

The EXIT trap should call a function. The function should remove (a) all
temporaries, (b) the final output.

Step (b) -- removal of the final output -- should depend on a global
variable. If we are about to exit successfully, this global variable
should be set accordingly, so that the subsequent EXIT handler omit step
(b).

If you think this is overkill, that's OK; I don't insist.


+mcopy -i ${basedir}/disk.fat ${basedir}/grub.cfg ::grub.cfg || exit 1
(19) Please improve quoting, operand-from-option separation, and
exit-on-error (see above).


+
+
+${mkimage} -O x86_64-efi -p '(crypto0)' -c ${basedir}/grub-bootstrap.cfg -m ${basedir}/disk.fat -o ${basedir}/grub.efi ${GRUB_MODULES} || exit 1
+
(20) Same as (19).


(21) Please keep the line length under 80 characters; something like:

${mkimage} -O x86_64-efi \
-p '(crypto0)' \
-c ${basedir}/grub-bootstrap.cfg \
...


+# remove the intermediates
+for f in disk.fat grub-bootstrap.cfg; do
+ rm -f ${basedir}/$f
+done
+echo "grub.efi generated in ${basedir}"
(22) I'd prefer improving this as described in (18).

--*--

The general idea of my review is that I'll let you do what you need to
do security-wise -- at best I can suggest high-level ideas regarding
that, such as point (5) --, whereas on the build front and the
look-and-feel front, I'd like this to be reasonably polished and
consistent with the edk2 style. (So if my review feels superficial, it's
not random.)

... Which reminds me: please check if the patch series passes the "ECC"
(Edk2 C Checker) part of the edk2 build CI. For that, please just push
your topic branch that you're about to post to the list to your personal
github repo, and submit a pull request against the main edk2 repo
(master branch). Your merge request will be auto-rejected in the end
(actual merging is only available to maintainers), but if there's a
problem with CI, you'll learn about it.

Now, "ECC" is by no means bug-free, so in some cases workarounds (or
even error suppressions) are necessary. Either way, we'll have to be
ECC-clean in the end, so it's best if you check that as well (I'd run
into the same problems with the real merge anyway).

CI can be run locally too, but setting it up is not trivial. If you
prefer not submitting ad-hoc PRs just for kicking of CI, then please see
this thread:

[edk2-devel] running CI locally
https://edk2.groups.io/g/devel/message/64428
https://www.redhat.com/archives/edk2-devel-archive/2020-August/msg00823.html

Thanks!
Laszlo

Join devel@edk2.groups.io to automatically receive all group messages.