From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM12-DM6-obe.outbound.protection.outlook.com (NAM12-DM6-obe.outbound.protection.outlook.com [40.92.22.85]) by mx.groups.io with SMTP id smtpd.web11.1526.1687280821924679775 for ; Tue, 20 Jun 2023 10:07:02 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@outlook.com header.s=selector1 header.b=ayKM8m82; spf=pass (domain: outlook.com, ip: 40.92.22.85, mailfrom: spbrogan@outlook.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Q9fRAZx/7l7YOfOFqdkrW6E3qmUxwgyCddfAIYXjLz+gPFUi06XpnZUCiaq5L3A0Fgx/SGaIKxPNBijb8PptgpXguKiwSA3hf9b7isRmb5zReTPZJlJRwLFpxQATElObmHuUBteIkgC1VDlYbaNFyHQnyWD0J5r+m0XaMkS2Wwobck5aw0/YzSihQQYllSWYejSWlfnVswC6+ebHrQBDEU6BX9L/TpGY130KpriL4kCaMYJR0ib2ZcT6EYg/9VaOj8rRuj1szo1juk7KVtdDdsltER37Cc7nwTEPCoMgTn8XlKoUV302jLTKtXJ1rQmw9dCHjrK8XCPtNFdkDv8GKw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=upASJmL/ydtcRqF+OZ9uZwMTGFj7mwKGpC7Nu0XRAMk=; b=GK9asVMvocYh3TmxD0Az0UZ5L5YbNl4CdAlr55FmaNBiZQl4vd2pmlv0o1P7wkoFZnyfItSqL2M+sGgZ2svrqUb0j6HG0/xQ8P6dEp8nEfRHZaO48ERmHdRNahBA2g1Dw3SYXMUSvw9wrV9TViHDcP77lTU/zF7WfVjS++GN05B2Xg0ujRNFKxn8ks4WSK0D0jhCAi6g+HEtkAZaLYbbexINn6zHWWpnGGAoTG9eCYvqo/tOp8oIpiyvsvGBbKq9ZDFMko5DQV8PUiCPESYZOAYjlBPB5X4E6jKhqKnw4jO+5TQUDlPDQ1SK+/oVLy5NzPJX47fOkjFQ4CCNJc8KnA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=outlook.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=upASJmL/ydtcRqF+OZ9uZwMTGFj7mwKGpC7Nu0XRAMk=; b=ayKM8m82KCAXIT0SYDiFlihuy6V2evvhM7nyd4igtxB8YvumC/jzErHAPd9FtGVTXpaiRF7utfmPuHHDcADU+kvlyz1NWmKa4lWYJm9pd6D+kbdebWn/SMM8C4WLyf/qt37hnkvBMgHMpyLqEpoEg2cnrxX8BcR2l7BS2rdevYG0g27ceLnL++MI2yjk1xOAW+T9ErnLJx39m1Ajro+FRN5reI5adUwZM6PD1kW5NlD5Ha1FJscZKga1UTJeJd7w/66s/I/4mqkePDM0AzYU0U5WqlwnXMG6KuJ+a4cH2lZPi8bMkmaXxghf21yFr86AW1JTDqFEFsqljJKPzS+Rrw== Received: from BY3PR19MB4900.namprd19.prod.outlook.com (2603:10b6:a03:354::11) by CY8PR19MB6937.namprd19.prod.outlook.com (2603:10b6:930:62::5) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6500.14; Tue, 20 Jun 2023 17:06:58 +0000 Received: from BY3PR19MB4900.namprd19.prod.outlook.com ([fe80::3f34:d4b:3873:2cac]) by BY3PR19MB4900.namprd19.prod.outlook.com ([fe80::3f34:d4b:3873:2cac%5]) with mapi id 15.20.6521.020; Tue, 20 Jun 2023 17:06:58 +0000 Message-ID: Date: Tue, 20 Jun 2023 10:06:55 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.12.0 Subject: Re: [edk2-devel] [PATCH 1/1] ArmPkg: Add Pcd to disable EFI_MEMORY_ATTRIBUTE_PROTOCOL To: devel@edk2.groups.io, 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> From: "Sean" In-Reply-To: X-TMN: [bIh4zbukkc2J1cpoyvLM7Bn9W4Rrg58jVOf/7mZ9v2p06zdMAzR8qPI6q5GyPKXP] X-ClientProxiedBy: CY5P221CA0043.NAMP221.PROD.OUTLOOK.COM (2603:10b6:930:4::13) To BY3PR19MB4900.namprd19.prod.outlook.com (2603:10b6:a03:354::11) Return-Path: spbrogan@outlook.com X-Microsoft-Original-Message-ID: <48d361d1-7929-5a4a-675d-818dc0543008@outlook.com> MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: BY3PR19MB4900:EE_|CY8PR19MB6937:EE_ X-MS-Office365-Filtering-Correlation-Id: 59f2cd3a-dddb-42f3-6005-08db71b0c143 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: X8MU3bi7EMj67OytGy2iq7juIpeeFbdPD2zZHosoKay47DqNsswlH6sQa2fAoFpIG4aouMa0Qm9ZB8DBLuHxfuxmy3hMGCvHj7ic/09LKNBPoVK8/Pbhl4w0kFT1ZhfrluHUYPzSAlo3+9iXs2kOQuy6ZRjKUnb25P4Z8paHWdqdZ3FCSIxrzEyrggCP9IDxrw3WHraiOmhOOmGF70jJrLVR4LiVrix+ZpgwYo2spsJ5+zA1jgfiRCEKp23u2XRoA+bqOwRxHjRnjj5mkzs8mOFmPwfgaT6YudFaPDPcRNQ0z9rirbzDk4LH0nyGWbSswgiRJ5Ob1YIpt1A5Pk2cw7qRm4mNe6HBkof8gDZgLOIefZ8i51HaYBhK86wnrBwNpNH9Q2amGs46Wz0a7LOFKI/OzI2rL8BQp4q48URCr/tr4kum3uIBC7t0x2JWOlquKB+cp0hpk4EKSu2WLoZjYdjOmnsAHYUz1FttiEdUc3YSN1WY+4mqMSOTZBcRdIBtxfuc++iZRaDibqaMANEDwZmB7ZE+eSkExFk07W8McO1DzmsaDXjPMpN/bqhhNodWKUiZI9ObdZW5gLxUqIDMvjezr3lv84LxZa2TvCcE7BJUna3e7TSfrjijGAKRqp/nD/GkEMGWxQcjDPNRGgTeoR+Hyj7JZb3XRu2txgZZWpQ= X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?dlg0K2Nud2ZEd05GMlB0S3ZNcXg5RkxrNjlmZ1E3WkRsRDVrYkR2bmw1RkUz?= =?utf-8?B?aHRBWjgxdXIxSmxpekNGQjJrOFBiaG8vb1V6SXdSZjl5VHFPQ1k1NGJZM21E?= =?utf-8?B?VUE2cnVYR1IzeU5mZjFiQjF5Vk53N0ZVQXVpc0NHTHJIYURmK1lySzlDVGRo?= =?utf-8?B?WWJCK3JCREE3OGJwNEhwd09sSTdJMHVQeVpzUjR6M0o3YThFSGU5SlAvSDZp?= =?utf-8?B?ZE5GU1Z2ZDJCNm05Z01qaDNrOVhydWFhamQvU3pCMXQ5dWxDUlZtNWJOOUJx?= =?utf-8?B?ZGZwekYxcW5KQTRBSDU4VFBsWklPZG5PYVB6eHh0Um1WOVpmd0dKL2lhT0ha?= =?utf-8?B?aGpNb2h5QVpVaElJL2E0NDM5WDIwVWhJeXpObXhYUWhqSjFwNEtIeG1lQk51?= =?utf-8?B?UStMOUh0dXpqNllLTmx2clZldWF2dHJ2bU5UT2wra1ZRQitwNjlETGdvQitO?= =?utf-8?B?RWQyTnU1Q1gwVTZ6UlRVUzFDaTdPbTVYektwSmh4NGdaQURNWWlPTFlRU0tS?= =?utf-8?B?UkxZZmJSTjF2SHRhZGFVRjBDUlNPbU5hSjR2UkE0eVYzTHQ4TGhyVHFuMHhy?= =?utf-8?B?cVFqN2pSZ1FlYk50QWhIc3ZZTjhvR0Q0NHFlOWt0V2JXYTVkQ2tHWGRQRktV?= =?utf-8?B?RWNHazA5dXM4MDZuOHBiaFp3cFVmTlcrWm9LR2NQOXhtZzB5ditZOFFJTXls?= =?utf-8?B?V2t2ZDNUNUN6Z1Z1NUNydXg3VzExbC90RFdxVFpvVjVKNkM4NUh3allUNSsx?= =?utf-8?B?Y1F3bGhCTVNVRjRta3lrMEx5SWdNZUkvencra28wK25qaHR3L0plZGlZZVAx?= =?utf-8?B?UFJjRXVVQXhRZUI4aE5aYmtBQ3h5dlFqNU56clNlaVNoZ2FSN3QrbytScC9u?= =?utf-8?B?QUh3Sk4yRUVyZmFPOURJUmlMUG5BYTFMR2RqbEZjUnJXdHJzcUQ4bEgvRmZu?= =?utf-8?B?V2MzWGZUMmRhQ0JGM0YrYWxEVHBMa2t1WmcrdFdYK3k0a2FpeCtlSFJPem5Z?= =?utf-8?B?NXRKamlHcjNrQ1lTTW9SVm4zVXJicHpES1dhbXBsaGJyTGFqSlpXTW12Zktx?= =?utf-8?B?VGtWNE01MGx2NWJCR2FualJMVHE5a1RQUGRGSHFSc0xDYytrWkJGSmRtb2I2?= =?utf-8?B?ejBLVkk2TkFRRDF3WGlMNXgxaExsNXFCaVVpWmlHWWdNdk03V3h0U0dZL0lt?= =?utf-8?B?VDg5UmhySExtOE5HS0JHUzlLL2xueHl3S3NtbTFsbEFaUmZ6dzFpc0hNcDVZ?= =?utf-8?B?ajArY1NDK0xaUXltVFBSVHNMcGtuNGtSS1NhT3JlN1liN05qLy82K3gxcytO?= =?utf-8?B?cW5DNTdrYm5BUzdFTWFQdFhkaVlDdThxYVlpOXNnM015QTBsREc2MjhNUG16?= =?utf-8?B?YUs3dWN3N0RMbzdVS050WVUxU2lWdDJLdW5wWEp3NkI0VGNBTm91RUdpbE9z?= =?utf-8?B?azBONStGbzJtMDdGTXhEa1FWMHkxSnRGZXp6N214K1hDVittMk9pMlJ5TFVp?= =?utf-8?B?dml5MmdhdVlzNHN0SGtoQTVUVGRvcGZQSzJzb2NuQmRzMVdpeVpXUFROTHVB?= =?utf-8?B?cktuMEdVNUNLL3dndnB3UzlmNG80WGJzNU5wV2FLZi8rSEwzK0lmazBXNFZw?= =?utf-8?B?TFdyVFlVaFVYeDJWbkcxMlQ5UGpIZjV5VGIrM3dUMENPVEdTeUlRTzZqaU9x?= =?utf-8?B?ZmliSVM4NkZhenV0MjBkRjNzdEcvS2hSQldLM3dJcTlCWEdJTzI0YVNnPT0=?= X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 59f2cd3a-dddb-42f3-6005-08db71b0c143 X-MS-Exchange-CrossTenant-AuthSource: BY3PR19MB4900.namprd19.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Jun 2023 17:06:58.2557 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000 X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR19MB6937 Content-Type: multipart/alternative; boundary="------------gIvToSEjzdzgXcQmgGNv4OhR" Content-Language: en-US --------------gIvToSEjzdzgXcQmgGNv4OhR Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit I don't think a MemoryAttributes2Protocol with a single API would have avoided the errors. The programming pattern that triggered this would still need multiple calls to any API and in the future where all memory is allocated as NX this possibility would still exist. A short term effort to minimize the compatibility problem in the ecosystem is documented here Memory Protections: Document compatibility challenges · Issue #18 · tianocore/projects (github.com)   It does not address (and i don't see any reason to try to) a loader that uses the protocol incorrectly. We have provided virtual reference platforms with these features enabled (both arm and x86) and have been working with the relevant communities for multiple years now.  The UEFI CA for option roms already have compliance requirements (UPDATED: UEFI Signing Requirements - Microsoft Community Hub ).  But there are and will continue to be compatibility challenges when enabling a more restrictive execution environment in uefi and the uefi ecosystem.  The more things we make optional the longer this transition period will take.    "Memory Mitigations" were proposed and mostly coded over a decade ago.  The code changes are not that difficult. To change our vast and unwieldyecosystem is the hard part.   Please help to "stay the course" for this very necessary industry change.   If a production platform has requirements that force such a configuration option that is understandable but it is counter productive in open-source industry standard reference Edk2 code. Thanks Sean On 6/20/2023 9:03 AM, Gerd Hoffmann 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 > > > > > > --------------gIvToSEjzdzgXcQmgGNv4OhR Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

I don't think a MemoryAttributes2Protocol with a single API would have avoided the errors. 

The programming pattern that triggered this would still need multiple calls to any API and in the future where all memory is allocated as NX this possibility would still exist. 

A short term effort to minimize the compatibility problem in the ecosystem is documented here Memory Protections: Document compatibility challenges · Issue #18 · tianocore/projects (github.com)  It does not address (and i don't see any reason to try to) a loader that uses the protocol incorrectly. 

We have provided virtual reference platforms with these features enabled (both arm and x86) and have been working with the relevant communities for multiple years now.  The UEFI CA for option roms already have compliance requirements (UPDATED: UEFI Signing Requirements - Microsoft Community Hub).  But there are and will continue to be compatibility challenges when enabling a more restrictive execution environment in uefi and the uefi ecosystem.  The more things we make optional the longer this transition period will take.    "Memory Mitigations" were proposed and mostly coded over a decade ago.  The code changes are not that difficult. To change our vast and unwieldy ecosystem is the hard part.   Please help to "stay the course" for this very necessary industry change.   If a production platform has requirements that force such a configuration option that is understandable but it is counter productive in open-source industry standard reference Edk2 code. 

Thanks

Sean




On 6/20/2023 9:03 AM, Gerd Hoffmann 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
--------------gIvToSEjzdzgXcQmgGNv4OhR--