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.web11.98026.1680604900651268657 for ; Tue, 04 Apr 2023 03:41:40 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=WiGubFqo; 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 EAD2E6183B for ; Tue, 4 Apr 2023 10:41:39 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 6531AC433A7 for ; Tue, 4 Apr 2023 10:41:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1680604899; bh=eBladBIrvoAM53Q0rk4X6xq4z2t59mViNG+t89m8bwQ=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=WiGubFqoRHz8s3LnWtDxoldp7TTHddiT42SBAlOh/auYVz0f+cFNvomFT/nrjgZIV cL3gIykMpjvuhn+RA4av20o275N8GZChK/oq/Mtwjo9khOxNYl00xs7XUIbdQ+kUvg j4bh+EdVWcsvRfSjLaqq4swsAl+tc9xVZ6sbf9VePw18s+2asoRuU5utWRHbfuqBqE a3Qw+g3V6pBUIlH4JjBX5nZpGS9amzIfl/mu9adNVh4u8I4h3WEZZZbnrmkPN1bXe4 wGkFYJ/WMVdYGtd+hZWmbUzTHBWGXEBRwK5XAtaU1QhK+rIqMz3C5CwUy2TdyoGFf7 959yiosCkKOKg== Received: by mail-lj1-f181.google.com with SMTP id a11so33307934lji.6 for ; Tue, 04 Apr 2023 03:41:39 -0700 (PDT) X-Gm-Message-State: AAQBX9f/g7IP8SXyRc8XcYdwAgZ8CRk4VI8zMgZspL3++SiZ7R9YkFQ/ JPP52FrIetascFgyw09tCTBVQlnhlUyaJyazbbQ= X-Google-Smtp-Source: AKy350ZOZoleaxmDk3hPau1xTuIfKg3oJ1N1uLfw9nDq8z9676dL9G2hCf+VSvHfFzzww+tt2vOuobiXC/DS1FJfE6g= X-Received: by 2002:a2e:93c3:0:b0:298:bddc:dbbf with SMTP id p3-20020a2e93c3000000b00298bddcdbbfmr853842ljh.2.1680604897155; Tue, 04 Apr 2023 03:41:37 -0700 (PDT) MIME-Version: 1.0 References: <20230327110112.262503-1-ardb@kernel.org> <20230327110112.262503-18-ardb@kernel.org> In-Reply-To: From: "Ard Biesheuvel" Date: Tue, 4 Apr 2023 12:41:26 +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 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.