Re: [edk2-devel] [PATCH 1/1] ArmPkg: Add Pcd to disable EFI_MEMORY_ATTRIBUTE_PROTOCOL

["Nhi Pham via groups.io" <nhi=os.amperecomputing.com@groups.io>]

Hi Ard, Oliver,

I'm investigating the crash on grub2/shim loader due to the added 
EFI_MEMORY_ATTRIBUTE_PROTOCOL when rebasing. I found this interesting 
patch and went through on the discussion, I am still not sure the 
conclusion on this patch.

This issue impacts many platforms, and any downstream edk2 has to clone 
this patch to disable the EFI_MEMORY_ATTRIBUTE_PROTOCOL until we have 
the loader fixed, maybe years. So, I wonder whether we can merge this 
patch with changing PcdEnableEfiMemoryAttributeProtocol to be disabled 
by default in DEC? This provides downstream platforms with the 
flexibility to enable/disable it as per their preference, rather than 
having to clone this path to their local repository. Furthermore, it 
does not impact the default installation of the 
EFI_MEMORY_ATTRIBUTE_PROTOCOL in the mainline.

Thanks,
Nhi

On 6/20/2023 11:03 PM, Gerd Hoffmann via groups.io wrote:
> On Tue, Jun 20, 2023 at 04:16:40PM +0300, Ard Biesheuvel wrote:
>> On Tue, Jun 20, 2023, 12:33 Gerd Hoffmann <kraxel@redhat.com> 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)
> 
> Sure.  This hits VM firmware first because we quickly rebase our builds
> to new edk2 stable tags.  But yes, this is not limited to VMs and
> likewise not limited to arm.
> 
>> 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.
> 
> I'd love to ditch shim.efi, even with secure boot.  For VMs one can
> just enroll the distro signing certificate to 'db' and be done with
> it.
> 
> Unfortunately shim has a solid position being *the* entry point for
> linux efi systems due to being the only piece of software carrying a
> microsoft signature.  Especially on install media you can't really have
> more than one (such as different binaries depending on whenever secure
> boot is on or off).  For installed systems and cloud images shim also
> creates/restores BootNNNN entries.
> 
> Additional problem is that shim is the piece of software which handles
> sbat revocations, so even in case the distro cert is enrolled in 'db' so
> the certificate handling implemented by shim is not needed I can't just
> ignore shim.efi.
> 
>> 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.
> 
> Sounds reasonable to me.
> 
>> 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.
> 
> Yes.  It's not a solution, it's a workaround which we could use to turn
> off EFI_MEMORY_ATTRIBUTE_PROTOCOL for a year or two, depending on how
> quickly the shim / distro folks get their act together and updates
> rolled out.
> 
> I'm not a fan either, but we need some temporary stopgap, and given that
> others likely meet the very same problem too we figured sending it to
> the list is a good idea, and here we are ;)
> 
> take care,
>    Gerd
> 
> 
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109343): https://edk2.groups.io/g/devel/message/109343
Mute This Topic: https://groups.io/mt/99631663/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-