From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web10.104917.1680622254900555704 for ; Tue, 04 Apr 2023 08:30:55 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=J9Wtyme9; spf=pass (domain: kernel.org, ip: 139.178.84.217, 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 dfw.source.kernel.org (Postfix) with ESMTPS id 3CB2F6360F for ; Tue, 4 Apr 2023 15:30:54 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id A0F74C4339C for ; Tue, 4 Apr 2023 15:30:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1680622253; bh=P/ph31Iqh+V+8GTPYdGqZooj6Nz6t8dDqIENLssfV/A=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=J9Wtyme9oLVvQfg9aw/O044PhuV4KQPVuxWX1azu1yXCoNYpW02lWyzsecpP+bcYx p/I9JzqgCRqF3RnZ91sSO75TPXjZrMEVMxPY/9hPbWR5zgSLW51DcOxrCRirgULQ9D xQqC5UTAZMwCC/WkIGpbqL/4vtIuSHJUNGUYv3A86hQT5hEvgKftbvEKlbQae4ooag xAN8c9kAjSyXgFIxmyAsZUDb+FnHtGWTVIFtFaDC7cHJXnJO6UrEfxqoW8l1+BYGB6 ThqW9hriMEvVm8NTxxk9PTAy0WMm7Gg2H39BtIv9he5098Id4ytdUcR79e7pFXC14n T3sNC9Lg/ayVA== Received: by mail-lj1-f179.google.com with SMTP id z42so34270769ljq.13 for ; Tue, 04 Apr 2023 08:30:53 -0700 (PDT) X-Gm-Message-State: AAQBX9e3TSObCXKZTon/ZomcrXTM93jfrQx8QPjod1bB28ljJ2aU7UFJ A3Xy9hd9iq2+kIDtkr9VzLY6qJQwu+Rb0vaZl8g= X-Google-Smtp-Source: AKy350a8YZKL7aEBknQHyrFr3nAQw68pr+M+fdN9YSEpM5JMJoJeyaeOG0EW2sV7yej9DJYcfWZ65vlixt76DCV9LE4= X-Received: by 2002:a2e:9c58:0:b0:298:b375:acfc with SMTP id t24-20020a2e9c58000000b00298b375acfcmr1185039ljj.2.1680622251641; Tue, 04 Apr 2023 08:30:51 -0700 (PDT) MIME-Version: 1.0 References: <20230327110112.262503-1-ardb@kernel.org> <20230327110112.262503-18-ardb@kernel.org> <496938cd-0e7d-d719-a097-d17f113f6adf@linux.microsoft.com> In-Reply-To: <496938cd-0e7d-d719-a097-d17f113f6adf@linux.microsoft.com> From: "Ard Biesheuvel" Date: Tue, 4 Apr 2023 17:30:40 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH v2 17/17] MdeModulePkg: Enable forward edge CFI in mem attributes table To: Oliver Smith-Denny Cc: devel@edk2.groups.io, 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 Content-Type: text/plain; charset="UTF-8" On Tue, 4 Apr 2023 at 17:00, Oliver Smith-Denny wrote: > > 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 > Thanks!