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 <devel@edk2.groups.io>;
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 <devel@edk2.groups.io>; Tue, 4 Apr 2023 15:30:54 +0000 (UTC)
Received: by smtp.kernel.org (Postfix) with ESMTPSA id A0F74C4339C
for <devel@edk2.groups.io>; 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 <devel@edk2.groups.io>; 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>
<cad03d93-0d71-8277-fa3c-89e17ab9c475@linux.microsoft.com>
<CAMj1kXEK6q4-v84kh=QSaUmG1miGi3vFbQPiF=tkSwRd55iBEw@mail.gmail.com> <496938cd-0e7d-d719-a097-d17f113f6adf@linux.microsoft.com>
In-Reply-To: <496938cd-0e7d-d719-a097-d17f113f6adf@linux.microsoft.com>
From: "Ard Biesheuvel" <ardb@kernel.org>
Date: Tue, 4 Apr 2023 17:30:40 +0200
X-Gmail-Original-Message-ID: <CAMj1kXGRCeNnBdKK4FR-vcfY+rWmhFBnjHOV9z9UYObxeDO1Qw@mail.gmail.com>
Message-ID: <CAMj1kXGRCeNnBdKK4FR-vcfY+rWmhFBnjHOV9z9UYObxeDO1Qw@mail.gmail.com>
Subject: Re: [edk2-devel] [PATCH v2 17/17] MdeModulePkg: Enable forward edge CFI in mem attributes table
To: Oliver Smith-Denny <osde@linux.microsoft.com>
Cc: devel@edk2.groups.io, Michael Kinney <michael.d.kinney@intel.com>,
Liming Gao <gaoliming@byosoft.com.cn>, Jiewen Yao <jiewen.yao@intel.com>,
Michael Kubacki <michael.kubacki@microsoft.com>, Sean Brogan <sean.brogan@microsoft.com>,
Rebecca Cran <quic_rcran@quicinc.com>, Leif Lindholm <quic_llindhol@quicinc.com>,
Sami Mujawar <sami.mujawar@arm.com>, Taylor Beebe <t@taylorbeebe.com>,
=?UTF-8?Q?Marvin_H=C3=A4user?= <mhaeuser@posteo.de>,
Bob Feng <bob.c.feng@intel.com>
Content-Type: text/plain; charset="UTF-8"
On Tue, 4 Apr 2023 at 17:00, Oliver Smith-Denny
<osde@linux.microsoft.com> wrote:
>
> On 4/4/2023 3:41 AM, Ard Biesheuvel wrote:
> > On Mon, 3 Apr 2023 at 17:48, Oliver Smith-Denny
> > <osde@linux.microsoft.com> 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 <ardb@kernel.org>
> >>> ---
> >>> 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 <osde@linux.microsoft.com>
>
Thanks!