From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 2837B7803CE for ; Thu, 5 Oct 2023 08:23:37 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=WgNziSmBAa/9P9YLgTjd+hp2BK6cRhtzjNAJ6rS2PZY=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1696494215; v=1; b=uRjzlOvIGSlHU09o5RYjTGzCkNKXIKd5wD9GMDR4I8h7uzP7C7h/RrZXicaXgPLOdHdmLnFI 1GmrFSyPpoRxFAZmRuoqUhlxcdUCDVhdAEJC64KdzlLXdNSoVZnqbcIGzk3kpDWuEMCnaQTaU6E 7LD7rpOkOb6ZGzE49Y9q2DK8= X-Received: by 127.0.0.2 with SMTP id UFjYYY7687511xxbK0pRqm9Z; Thu, 05 Oct 2023 01:23:35 -0700 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web10.10637.1696494215138006840 for ; Thu, 05 Oct 2023 01:23:35 -0700 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-193-eh0bymP6NJ-UjU7nGENQTw-1; Thu, 05 Oct 2023 04:23:30 -0400 X-MC-Unique: eh0bymP6NJ-UjU7nGENQTw-1 X-Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id D75DA101A53B; Thu, 5 Oct 2023 08:23:29 +0000 (UTC) X-Received: from [10.39.193.139] (unknown [10.39.193.139]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A13A42026D68; Thu, 5 Oct 2023 08:23:26 +0000 (UTC) Message-ID: <254cfe1b-3b28-419e-c5ef-9907938536c5@redhat.com> Date: Thu, 5 Oct 2023 10:23:25 +0200 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 1/1] ArmPkg: Add Pcd to disable EFI_MEMORY_ATTRIBUTE_PROTOCOL To: devel@edk2.groups.io, nhi@os.amperecomputing.com, kraxel@redhat.com, Ard Biesheuvel Cc: Oliver Steffen , 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 References: <20230619203244.228933-1-osteffen@redhat.com> <4f7bcd27-35a6-33bf-61b4-4cafc6d23d5c@os.amperecomputing.com> From: "Laszlo Ersek" In-Reply-To: <4f7bcd27-35a6-33bf-61b4-4cafc6d23d5c@os.amperecomputing.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.4 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: eQQ6M93fVz58Q19cr0bK6DCDx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=uRjzlOvI; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io 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 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/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-