On 10/5/23 08:31, Nhi Pham via groups.io wrote: > 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.
I think a more general approach is being discussed in the "MdeModulePkg: Add Additional Profiles to SetMemoryProtectionsLib" thread. I do agree the "--pcd" build flag would be best to configure a default platform profile. Laszlo > 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 <kra...@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 (#109345): https://edk2.groups.io/g/devel/message/109345 Mute This Topic: https://groups.io/mt/99631663/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
