From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web08.41146.1605559381526870043 for ; Mon, 16 Nov 2020 12:43:01 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Geegffmx; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1605559380; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=f8qYUCV7vQEqFoCC+9Z+B43BFjvs+wO0P8PMBhCDi2g=; b=Geegffmx/C4ZXSImT0sUdP/coulbYr7rtTVmJlMgJL49rwpsOVovgL45kcvr/BzHR/hl7q H66cZizLPZhi/Fa+2a9wJYWy2XTQfT0oytdRkrhrgefMb8v3n43se1WkdLQjNbw5Rfs6eT 61Q8m8c3mrE3mO+WAq1++HtX4QlwA7M= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-470-FrPy5mq3PaKo70gd8yuUTw-1; Mon, 16 Nov 2020 15:42:56 -0500 X-MC-Unique: FrPy5mq3PaKo70gd8yuUTw-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id B331489CCC0; Mon, 16 Nov 2020 20:42:54 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-190.ams2.redhat.com [10.36.112.190]) by smtp.corp.redhat.com (Postfix) with ESMTP id CEE4660C05; Mon, 16 Nov 2020 20:42:51 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 2/4] OvmfPkg/AmdSev: add Grub Firmware Volume Package To: devel@edk2.groups.io, jejb@linux.ibm.com Cc: dovmurik@linux.vnet.ibm.com, Dov.Murik1@il.ibm.com, ashish.kalra@amd.com, brijesh.singh@amd.com, tobin@ibm.com, david.kaplan@amd.com, jon.grimm@amd.com, thomas.lendacky@amd.com, frankeh@us.ibm.com, "Dr . David Alan Gilbert" References: <20201112001316.11341-1-jejb@linux.ibm.com> <20201112001316.11341-3-jejb@linux.ibm.com> From: "Laszlo Ersek" Message-ID: <64377f9f-da65-d0ca-e473-c14a69e16a66@redhat.com> Date: Mon, 16 Nov 2020 21:42:50 +0100 MIME-Version: 1.0 In-Reply-To: <20201112001316.11341-3-jejb@linux.ibm.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > --- > 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.
> +# 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 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