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.web12.42256.1606165708298824163 for ; Mon, 23 Nov 2020 13:08:28 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=WARISOWY; 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=1606165707; 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=2jAFB5N1bwzqHjGLv7kgoRIkvymx3QWAG5uadQFGVbU=; b=WARISOWY/67308aR8Mxv/E9f+gXAyuJOw83Oc6BufBMTu1IRasAgy2JN38CqrpqO0FoF4B Y8RUy976CsTRi05zzWtdZev1y457TVzxctH0tY3LQ5DwKBJ6GHKxNZjertMddeGKqFKbIw Mtmjvi00bC05hyoBTHt1GK+E+Y6QYI0= 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-150-JK5drhubN2yrUWKbRe5y-w-1; Mon, 23 Nov 2020 16:08:23 -0500 X-MC-Unique: JK5drhubN2yrUWKbRe5y-w-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 148B38030A8; Mon, 23 Nov 2020 21:08:21 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-230.ams2.redhat.com [10.36.112.230]) by smtp.corp.redhat.com (Postfix) with ESMTP id 15C2A5D6DC; Mon, 23 Nov 2020 21:08:17 +0000 (UTC) Subject: Re: [PATCH v2 2/6] OvmfPkg/AmdSev: add Grub Firmware Volume Package To: James Bottomley , devel@edk2.groups.io 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: <20201120184521.19437-1-jejb@linux.ibm.com> <20201120184521.19437-3-jejb@linux.ibm.com> From: "Laszlo Ersek" Message-ID: <28e99174-79b3-e805-b977-5fed0071a702@redhat.com> Date: Mon, 23 Nov 2020 22:08:17 +0100 MIME-Version: 1.0 In-Reply-To: <20201120184521.19437-3-jejb@linux.ibm.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 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 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 > > --- > > 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.
> +# 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.
> + 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.
> + 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.
> + 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 . 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