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.10085.1687267012484964568 for ; Tue, 20 Jun 2023 06:16:52 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=rLkjg5jB; 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.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id E4AC161241 for ; Tue, 20 Jun 2023 13:16:51 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id AE2CDC433CB for ; Tue, 20 Jun 2023 13:16:51 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1687267011; bh=/rZR9leD/95HbI5p/hnRaknSnGmnUveOqvJQGe6tjcM=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=rLkjg5jBuuLiJSgpJWBKTBAce6yjfbYYGtdyGYghfDotncDVsMBc3qwxPHbNpWFsJ d/TBRG94vOb2qhorJHA5reo6XQTseU/kiUqIVMzUbmOVaB0o38AQKFjVPY09w7b3Cp eWo3x31prL1KaGZ2whGXh5j7N3xTAkCUF61PEnRu2uCRpcQIBB9d4P8ZKF4C/XkoXB ZyCauxrVNmgQYWiATaksOqPOGdENtB62aj1YR047p0w7Kf/DsxqxzlNLM6O3y/meOz PRINI3sVNIAmc9BC6MsFv5iKdkDiOEwBseC3sN+/Q12awuD04clTf6A6JzS5vXr4FR HNN7vQLdFUnKA== Received: by mail-oa1-f42.google.com with SMTP id 586e51a60fabf-1a9b0ec6f4cso4245956fac.0 for ; Tue, 20 Jun 2023 06:16:51 -0700 (PDT) X-Gm-Message-State: AC+VfDy3hNoIH5KNInge9et77OitdDpS8bFrcOEC1a//JX9gjQm7X7Kg EnruyT4f6ND8wPAqn6RgiBjOPhRaITESux9rB2s= X-Google-Smtp-Source: ACHHUZ46uoI3SA7z42jg0fSgHkwutZ8VgBF3Tv06tMaFsnzdLWpNnpbwp4iuJNL3Cd9O9XAu5jj1ZdCNPpRJ6Dn4hxI= X-Received: by 2002:a05:6870:7401:b0:19f:5c37:abb9 with SMTP id x1-20020a056870740100b0019f5c37abb9mr5656000oam.12.1687267010775; Tue, 20 Jun 2023 06:16:50 -0700 (PDT) MIME-Version: 1.0 References: <20230619203244.228933-1-osteffen@redhat.com> In-Reply-To: From: "Ard Biesheuvel" Date: Tue, 20 Jun 2023 16:16:40 +0300 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 1/1] ArmPkg: Add Pcd to disable EFI_MEMORY_ATTRIBUTE_PROTOCOL To: Gerd Hoffmann Cc: Oliver Steffen , edk2-devel-groups-io , Ard Biesheuvel , Daniel Schaefer , Eric Dong , Leif Lindholm , Liming Gao , Michael D Kinney , Rahul Kumar , Ray Ni , Sami Mujawar , Sunil V L , Zhiguang Liu , Taylor Beebe , Oliver Smith-Denny , Michael Kubacki Content-Type: multipart/alternative; boundary="000000000000d190df05fe8f769e" --000000000000d190df05fe8f769e Content-Type: text/plain; charset="UTF-8" On Tue, Jun 20, 2023, 12:33 Gerd Hoffmann wrote: > On Mon, Jun 19, 2023 at 10:32:25PM +0200, Oliver Steffen wrote: > > Recent versions of shim (15.6 and 15.7) crash when the newly added > > EFI_MEMORY_ATTRIBUTE_PROTOCOL is provided by the firmware. To allow > > existing installations to boot, provide a workaround in form of a Pcd > > that allows tuning it off at build time (defaults to 'enabled'). > > Background: We have untested + broken code for > EFI_MEMORY_ATTRIBUTE_PROTOCOL support in the listed shim releases. > > Now that firmware starts to actually provide that protocol the > time bomb explodes. > > > Fantastic. This is kind of a big deal, really, and just turning it off for ArmVirtQemu does not help at all with the fact that these shim builds will crash on any platform that implements the protocol. (Including x86) Given that secure boot is kind of pointless on this particular platform anyway, maybe this is a good opportunity to make shim optional in the boot chain? I understand that this does not fix existing builds but shim proves to be such a problematic component that you really should not be using it if there is no need. As for the protocol, this has its own set of problems, and the bug in question can partly be blamed on the misdesigned api, which has separate set and clear methods. Not only does this force the implementation to traverse the page tables twice for the common case of switching between RO and XP or vice versa, it also means we lose any transactional properties of a RO <-> XP switch. I.e., if we could make it the implementation's responsibility to ensure that such a transformation either completes successfully, or otherwise, doesn't make any modifications at all, the risk of ending up in a limbo state is reduced significantly. So maybe there is still opportunity for specifying a MemoryAttributes2 protocol with a single method for set and clear? We could just drop the current one in that case. In any case, while i can see how this patch helps make all your ci status icons turn green again, it does so by papering over the underlying issue so I'm not a fan. > --- a/ArmPkg/ArmPkg.dec > > +++ b/ArmPkg/ArmPkg.dec > > @@ -172,6 +172,9 @@ [PcdsFixedAtBuild.common] > > > gArmTokenSpaceGuid.PcdCpuVectorBaseAddress|0xffff0000|UINT64|0x00000004 > > gArmTokenSpaceGuid.PcdCpuResetAddress|0x00000000|UINT32|0x00000005 > > > > + # Enable/Disable EFI_MEMORY_ATTRIBUTE_PROTOCOL > > + > gArmTokenSpaceGuid.PcdEnableEfiMemoryAttributeProtocol|TRUE|BOOLEAN|0x000000EE > > + > > # > > # ARM Secure Firmware PCDs > > # > > Given that I expect we will run into the very same problem on x64 as > soon as EFI_MEMORY_ATTRIBUTE_PROTOCOL gets enabled there we should > probably define the PCD in MdePkg not ArmPkg (which implies splitting > the patch into a mini series with one MdePkg and one ArmPkg patch). > > take care, > Gerd > > --000000000000d190df05fe8f769e Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Tue, Jun 20, 2023, 12:33 Gerd Hoffmann <kraxel@redhat.com> wrote:
On Mon, Jun 19, 2023 at 10:32:25PM +0200, Olive= r Steffen wrote:
> Recent versions of shim (15.6 and 15.7) crash when the newly added
> EFI_MEMORY_ATTRIBUTE_PROTOCOL is provided by the firmware.=C2=A0 To al= low
> existing installations to boot, provide a workaround in form of a Pcd<= br> > that allows tuning it off at build time (defaults to 'enabled'= ).

Background:=C2=A0 We have untested + broken code for
EFI_MEMORY_ATTRIBUTE_PROTOCOL support in the listed shim releases.

Now that firmware starts to actually provide that protocol the
time bomb explodes.

=C2=A0
=C2=A0

Fantastic.=

This is kind of a big d= eal, really, and just turning it off for ArmVirtQemu does not help at all w= ith the fact that these shim builds will crash on any platform that impleme= nts the protocol. (Including x86)

Given that secure boot is kind of pointless on this particular pl= atform anyway, maybe this is a good opportunity to make shim optional in th= e boot chain? I understand that this does not fix existing builds but shim = proves to be such a problematic component that you really should not be usi= ng it if there is no need.

As for the protocol, this has its own set of problems, and the bug in qu= estion can partly be blamed on the misdesigned api, which has separate set = and clear methods. Not only does this force the implementation to traverse = the page tables twice for the common case of switching between RO and XP or= vice versa, it also means we lose any transactional properties of a RO <= ;-> XP switch. I.e., if we could make it the implementation's respon= sibility to ensure that such a transformation either completes successfully= , or otherwise, doesn't make any modifications at all, the risk of endi= ng up in a limbo state is reduced significantly.
So maybe there is still opportunity for specifying= a MemoryAttributes2 protocol with a single method for set and clear? We co= uld just drop the current one in that case.

In any case, while i can see how this patch helps make = all your ci status icons turn green again, it does so by papering over the = underlying issue so I'm not a fan.




=

> --- a/ArmPkg/ArmPkg.dec
> +++ b/ArmPkg/ArmPkg.dec
> @@ -172,6 +172,9 @@ [PcdsFixedAtBuild.common]
>=C2=A0 =C2=A0 gArmTokenSpaceGuid.PcdCpuVectorBaseAddress|0xffff0000|UIN= T64|0x00000004
>=C2=A0 =C2=A0 gArmTokenSpaceGuid.PcdCpuResetAddress|0x00000000|UINT32|0= x00000005
>=C2=A0
> +=C2=A0 # Enable/Disable EFI_MEMORY_ATTRIBUTE_PROTOCOL
> +=C2=A0 gArmTokenSpaceGuid.PcdEnableEfiMemoryAttributeProtocol|TRUE|BO= OLEAN|0x000000EE
> +
>=C2=A0 =C2=A0 #
>=C2=A0 =C2=A0 # ARM Secure Firmware PCDs
>=C2=A0 =C2=A0 #

Given that I expect we will run into the very same problem on x64 as
soon as EFI_MEMORY_ATTRIBUTE_PROTOCOL gets enabled there we should
probably define the PCD in MdePkg not ArmPkg (which implies splitting
the patch into a mini series with one MdePkg and one ArmPkg patch).

take care,
=C2=A0 Gerd

--000000000000d190df05fe8f769e--