From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web10.104003.1680620417834027061 for ; Tue, 04 Apr 2023 08:00:17 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=R2NiSZ1c; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: osde@linux.microsoft.com) Received: from [10.137.194.171] (unknown [131.107.159.171]) by linux.microsoft.com (Postfix) with ESMTPSA id 3B347210DD98; Tue, 4 Apr 2023 08:00:15 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 3B347210DD98 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1680620415; bh=55WO3iHmuqGtUe/lx8DS7jL+KnCBa1tjhaTZotX5o3g=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=R2NiSZ1cVNjkCPFJqCL9ls82xM2X3/SQ4+fPNA4Mgmmf0cBj1oVwWBivfgPwSMk1W o/jNu1f0yDmonWhMC5du9gZtcCYPIrfufq6LkP/S6D0d8n/Cvf0Ik/bnV91C0/Lacr ECHp3zErl8dnJ31vwEVFPLJXg2OIdE/Fan9BAEuc= Message-ID: <496938cd-0e7d-d719-a097-d17f113f6adf@linux.microsoft.com> Date: Tue, 4 Apr 2023 08:00:14 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.9.1 Subject: Re: [edk2-devel] [PATCH v2 17/17] MdeModulePkg: Enable forward edge CFI in mem attributes table To: devel@edk2.groups.io, ardb@kernel.org Cc: Michael Kinney , Liming Gao , Jiewen Yao , Michael Kubacki , Sean Brogan , Rebecca Cran , Leif Lindholm , Sami Mujawar , Taylor Beebe , =?UTF-8?Q?Marvin_H=c3=a4user?= , Bob Feng References: <20230327110112.262503-1-ardb@kernel.org> <20230327110112.262503-18-ardb@kernel.org> From: "Oliver Smith-Denny" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 4/4/2023 3:41 AM, Ard Biesheuvel wrote: > On Mon, 3 Apr 2023 at 17:48, Oliver Smith-Denny > wrote: >> >> Turns out my old email was getting sent to a lot of folks spam, so >> resending with hopefully a better email... >> >> On 3/27/2023 4:01 AM, Ard Biesheuvel wrote: >>> 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. >>> >>> Given that the PE/COFF spec now defines a DllCharacteristicsEx flag that >>> indicates whether or not a loaded image is compatible with this, we can >>> wire this up to the flag in the memory attributes table, and set it if >>> all loaded runtime image are compatible with it. >>> >>> Signed-off-by: Ard Biesheuvel >>> --- >>> MdeModulePkg/Core/Dxe/DxeMain.h | 2 ++ >>> MdeModulePkg/Core/Dxe/Image/Image.c | 10 ++++++++++ >>> MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c | 8 +++++++- >>> 3 files changed, 19 insertions(+), 1 deletion(-) >>> >>> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h >>> index 815a6b4bd844a452..43daa037be441150 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/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c >>> index 8704ebea9a7c88c0..9dbfb2a1fad22ced 100644 >>> --- a/MdeModulePkg/Core/Dxe/Image/Image.c >>> +++ b/MdeModulePkg/Core/Dxe/Image/Image.c >>> @@ -1399,6 +1399,16 @@ CoreLoadImageCommon ( >>> CoreNewDebugImageInfoEntry (EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL, &Image->Info, Image->Handle); >>> >>> } >>> >>> >>> >>> + // >>> >>> + // Check whether we are loading a runtime image that lacks support for >>> >>> + // IBT/BTI landing pads. >>> >>> + // >>> >>> + if ((Image->ImageContext.ImageCodeMemoryType == EfiRuntimeServicesCode) && >>> >>> + ((Image->ImageContext.DllCharacteristicsEx & EFI_IMAGE_DLLCHARACTERISTICS_EX_FORWARD_CFI_COMPAT) == 0)) >>> >>> + { >>> >>> + gMemoryAttributesTableForwardCfi = FALSE; >>> >>> + } >> >> If I understand this correctly, we are disabling Forward CFI if we >> attempt to load any runtime images that don't support it. Would it make >> sense to have a PCD to determine whether we strictly enforce >> Forward CFI (i.e. don't load this incompatible image) in such a case? We >> have a similar option for non-NX_COMPAT images. >> > > These changes only affect what the OS sees, and if the OS wants to > implement a certain policy around this, it is free to do so. I don't > think this belongs in the firmware though, > > *However*, if/when we wire up forward CFI enforcement at boot time, it > would be appropriate to have a configurable policy around this, and > reject 3rd party images that do not implement forward CFI if the > firmware is configured for strict enforcement. > > I intend to look into that next, but given how tedious and painful it > is to get changes reviewed, I'm not sure this will be anytime soon. I see, thanks for the explanation, makes sense to me. For the patchset (sorry, with the email change I don't have the top level entry): Reviewed-by: Oliver Smith-Denny > > > >