From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by mx.groups.io with SMTP id smtpd.web11.1647.1675364467735925743 for ; Thu, 02 Feb 2023 11:01:08 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=b1RoYwuO; spf=pass (domain: kernel.org, ip: 145.40.68.75, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 46D72B8279C for ; Thu, 2 Feb 2023 19:01:05 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id F02A8C4339B for ; Thu, 2 Feb 2023 19:01:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1675364464; bh=ia7EzL7IfsNrQl/OAG5gbDTG4xxixi4bqhHXmL/eLTg=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=b1RoYwuON6VhQER8TGdTMagfmqC9l0a9U7grVV64ZT+Jm3MdnQnrjqhi94awkZERW +ZkDL+mnvKblTVMcp8FluvVejSSbUrv5xh+8TubH132S1Ra+dYsNXAlpHr7r/SkWZV kqpKKZ+N5pwmzvBCDswxw2jk7uauNqaaFE2Om4NWl+cWlXnSOp5nQOomfysIbpF32i el4VIVgCWBqJ08pjJMpB/To21Xt/TMLO9O43kLmZVSRDlDrFDU3CXzypQL3UMTfmhM lLov7asVO5sI5wY3LPBL9/OC6m3i+imNlhmjv1sZhhoX3myvbyolXCtB1GjgzmLqHs 8GvDL8xxiUSpQ== Received: by mail-lf1-f50.google.com with SMTP id bi36so4393891lfb.8 for ; Thu, 02 Feb 2023 11:01:03 -0800 (PST) X-Gm-Message-State: AO0yUKWpdspx50R3AlVj1O2iDx8sI9FFJVekWHWteX8qN7KxCtfTEM3P jquoRUQ30kC/dEyTj1XA4iXvHNIXQg4CMwiL83Q= X-Google-Smtp-Source: AK7set/ejHn9Y/HdU6CUizcbIZr6MOTQRElrm4dUfR1ZrfR9fGZgJy8BwrD+L93OlhV4CEVER/y9sQ8CA4OkLKqudDw= X-Received: by 2002:a19:500b:0:b0:4cc:789a:dac8 with SMTP id e11-20020a19500b000000b004cc789adac8mr1334704lfb.198.1675364461973; Thu, 02 Feb 2023 11:01:01 -0800 (PST) MIME-Version: 1.0 References: <20230202180335.2256160-1-ardb@kernel.org> <20230202180335.2256160-3-ardb@kernel.org> In-Reply-To: From: "Ard Biesheuvel" Date: Thu, 2 Feb 2023 20:00:50 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem attributes table To: "Kinney, Michael D" Cc: "devel@edk2.groups.io" , "Gao, Liming" , "Yao, Jiewen" , "Kubacki, Michael" , Sean Brogan , Rebecca Cran , Leif Lindholm , Sami Mujawar , Taylor Beebe Content-Type: text/plain; charset="UTF-8" On Thu, 2 Feb 2023 at 19:49, Kinney, Michael D wrote: > > Hi Ard, > > Since the PE/COFF image does not contain this information, is there an option > to add the information to an FFS file. Either as a new bit in a standard header > or as a GUIDed section defined by EDK II? > > Since an FV may contain content build from source and additional content > Integrated as binaries from other vendors, having a PCD that scopes this > attribute to all FVs many only work for FW builds that are 100% from sources. > I didn't quite consider that scenario tbh. It would be nice if we could avoid another PI spec dance for this, and get some kind of IBT/BTI annotation added to the PE/COFF spec. That way, we cover this issue, as well as clear the way for the use if IBT/BTI when running under the firmware. Are TE images a concern here as well? I.e., are those likely to be used for runtime DXE images in firmware volumes? > > > -----Original Message----- > > From: Ard Biesheuvel > > Sent: Thursday, February 2, 2023 10:04 AM > > To: devel@edk2.groups.io > > Cc: Ard Biesheuvel ; Kinney, Michael D ; Gao, Liming ; Yao, > > Jiewen ; Kubacki, Michael ; Sean Brogan ; Rebecca > > Cran ; Leif Lindholm ; Sami Mujawar ; Taylor Beebe > > > > Subject: [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem attributes table > > > > The memory attributes table has been extended with a flag that indicates > > whether or not the OS is permitted to map the EFI runtime code regions > > with strict enforcement for IBT/BTI landing pad instructions. > > > > This is generally a property of the firmware build, and so we can permit > > a platform to set this property using a PCD, and put the burden on the > > platform definition to set the toolchain options accordingly. > > > > There is one snag, however: PE/COFF does not expose whether or not the > > code was generated with landing pads, so if any runtime DXE drivers were > > loaded from storage other than the firmware volumes, we must assume that > > setting the CFI flag in the memory attributes table is unsafe. > > > > Signed-off-by: Ard Biesheuvel > > --- > > MdeModulePkg/Core/Dxe/DxeMain.h | 2 ++ > > MdeModulePkg/Core/Dxe/DxeMain.inf | 1 + > > MdeModulePkg/Core/Dxe/Image/Image.c | 11 +++++++++++ > > MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c | 7 ++++++- > > MdeModulePkg/MdeModulePkg.dec | 8 ++++++++ > > 5 files changed, 28 insertions(+), 1 deletion(-) > > > > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h > > index 815a6b4bd844..427a5fc78f72 100644 > > --- a/MdeModulePkg/Core/Dxe/DxeMain.h > > +++ b/MdeModulePkg/Core/Dxe/DxeMain.h > > @@ -280,6 +280,8 @@ extern EFI_MEMORY_TYPE_INFORMATION gMemoryTypeInformation[EfiMaxMemoryType + 1] > > extern BOOLEAN gDispatcherRunning; > > > > extern EFI_RUNTIME_ARCH_PROTOCOL gRuntimeTemplate; > > > > > > > > +extern BOOLEAN gMemoryAttributesTableForwardCfi; > > > > + > > > > extern EFI_LOAD_FIXED_ADDRESS_CONFIGURATION_TABLE gLoadModuleAtFixAddressConfigurationTable; > > > > extern BOOLEAN gLoadFixedAddressCodeMemoryReady; > > > > // > > > > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf > > index 35d5bf0dee6f..e6ff67773a69 100644 > > --- a/MdeModulePkg/Core/Dxe/DxeMain.inf > > +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf > > @@ -187,6 +187,7 @@ [Pcd] > > gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask ## CONSUMES > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ## CONSUMES > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth ## CONSUMES > > > > + gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryAttributesTableForwardCfi ## CONSUMES > > > > > > > > # [Hob] > > > > # RESOURCE_DESCRIPTOR ## CONSUMES > > > > diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c > > index 06cc6744b8c6..181fefdb6657 100644 > > --- a/MdeModulePkg/Core/Dxe/Image/Image.c > > +++ b/MdeModulePkg/Core/Dxe/Image/Image.c > > @@ -1398,6 +1398,17 @@ CoreLoadImageCommon ( > > CoreNewDebugImageInfoEntry (EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL, &Image->Info, Image->Handle); > > > > } > > > > > > > > + // > > > > + // If we loaded a runtime DXE driver from something other than a FV, it > > > > + // was not built as part of the firmware image, and so we cannot assume > > > > + // that it was built with IBT/BTI landing pads for forward edge control > > > > + // flow integrity. > > > > + // > > > > + if (!ImageIsFromFv && > > > > + (Image->ImageContext.ImageCodeMemoryType == EfiRuntimeServicesCode)) { > > > > + gMemoryAttributesTableForwardCfi = FALSE; > > > > + } > > > > + > > > > // > > > > // Reinstall loaded image protocol to fire any notifications > > > > // > > > > diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c > > index e07921371187..cdd35ade0a8a 100644 > > --- a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c > > +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c > > @@ -89,6 +89,7 @@ BOOLEAN mMemoryAttributesTableEnable = TRUE; > > BOOLEAN mMemoryAttributesTableEndOfDxe = FALSE; > > > > EFI_MEMORY_ATTRIBUTES_TABLE *mMemoryAttributesTable = NULL; > > > > BOOLEAN mMemoryAttributesTableReadyToBoot = FALSE; > > > > +BOOLEAN gMemoryAttributesTableForwardCfi = FixedPcdGetBool (PcdMemoryAttributesTableForwardCfi); > > > > > > > > /** > > > > Install MemoryAttributesTable. > > > > @@ -182,7 +183,11 @@ InstallMemoryAttributesTable ( > > MemoryAttributesTable->Version = EFI_MEMORY_ATTRIBUTES_TABLE_VERSION; > > > > MemoryAttributesTable->NumberOfEntries = RuntimeEntryCount; > > > > MemoryAttributesTable->DescriptorSize = (UINT32)DescriptorSize; > > > > - MemoryAttributesTable->Reserved = 0; > > > > + if (gMemoryAttributesTableForwardCfi) { > > > > + MemoryAttributesTable->Flags = EFI_MEMORY_ATTRIBUTES_FLAGS_RT_FORWARD_CONTROL_FLOW_GUARD; > > > > + } else { > > > > + MemoryAttributesTable->Flags = 0; > > > > + } > > > > DEBUG ((DEBUG_VERBOSE, "MemoryAttributesTable:\n")); > > > > DEBUG ((DEBUG_VERBOSE, " Version - 0x%08x\n", MemoryAttributesTable->Version)); > > > > DEBUG ((DEBUG_VERBOSE, " NumberOfEntries - 0x%08x\n", MemoryAttributesTable->NumberOfEntries)); > > > > diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec > > index 9605c617b7a8..d336a38655a3 100644 > > --- a/MdeModulePkg/MdeModulePkg.dec > > +++ b/MdeModulePkg/MdeModulePkg.dec > > @@ -1093,6 +1093,14 @@ [PcdsFixedAtBuild] > > # @Prompt Enable UEFI Stack Guard. > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0x30001055 > > > > > > > > + ## Indicates whether the EFI memory attributes table will inform the OS that > > > > + # forward edge control flow guards have been inserted into the runtime services > > > > + # code regions. > > > > + # TRUE - Runtime code has forward control flow guards.
> > > > + # FALSE - Runtime code does not have forward control flow guards.
> > > > + # @Prompt Enable forward control flow guards in EFI memory attributes table > > > > + gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryAttributesTableForwardCfi|FALSE|BOOLEAN|0x30001056 > > > > + > > > > [PcdsFixedAtBuild, PcdsPatchableInModule] > > > > ## Dynamic type PCD can be registered callback function for Pcd setting action. > > > > # PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number of callback function > > > > -- > > 2.39.1 >