From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout01.posteo.de (mout01.posteo.de [185.67.36.65]) by mx.groups.io with SMTP id smtpd.web10.80111.1675764827492235771 for ; Tue, 07 Feb 2023 02:13:47 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=GgthhaoN; spf=pass (domain: posteo.de, ip: 185.67.36.65, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id BF233240447 for ; Tue, 7 Feb 2023 11:13:45 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1675764825; bh=Htj9cxt6jw7xG/2fU5lZs4qco2N8j84basQoq1YZzLo=; h=From:Subject:Date:Cc:To:From; b=GgthhaoN4udVrMJEwvAHRMvwrXHLkBtsgwqADud1v8qFfwhRUwkQCH0R3VBmgHD68 2b4bUAbot/RSbU0CApee5Zo5d40DLWKzPWsMrZ83RQDH8Pk01Ois7qe2s3clVcDlv2 +5H3sYTielKVc/U/HuTqMPCMzIOhxWlLCZlEmhERdO6xmplgjo4NZlamHrFcaNQVL/ brI0zW6YYkD8OL56D3LHqcYv3zD6AZI0I/RttK1IAO06xmHHc5ajXl/tdvWlZ/i3Rr GQekfT8/eATp272Fj6z/kGkfSbYYSJhHNJVwDvyuf0DwXTbwHuwOLuauLJA5eAt3ZT ePDhUW/fSFWeA== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4P9zVc51sZz6tmK; Tue, 7 Feb 2023 11:13:44 +0100 (CET) From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Mime-Version: 1.0 (1.0) Subject: Re: [edk2-devel] [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol Date: Tue, 7 Feb 2023 10:13:44 +0000 Message-Id: References: Cc: devel@edk2.groups.io, t@taylorbeebe.com In-Reply-To: To: Ard Biesheuvel Content-Type: multipart/alternative; boundary=Apple-Mail-A97EA42A-B787-4532-BCC6-A9B90DB1F692 Content-Transfer-Encoding: 7bit --Apple-Mail-A97EA42A-B787-4532-BCC6-A9B90DB1F692 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable > On 7. Feb 2023, at 11:01, Ard Biesheuvel wrote: >=20 > Actually, it seems UnprotectUefiImage () is corrent under the > assumption that all code regions have EFI_MEMORY_XP cleared by > default. >=20 > However, if you redefine the policy to set EFI_MEMORY_XP on code > regions by default, and only permit execution after remapping the code > read-only explicitly, and only then clearing EFI_MEMORY_XP, that > routine should revert the region to EFI_MEMORY_XP. But given the > existing ASSERT()s on having EFI_MEMORY_XP cleared for all code > regions, the code as it is currently is not incorrect. Right. My main issue is, it=E2=80=99s nowhere documented that manually chang= ed permissions must be restored to their default before freeing. Within DxeC= ore, this is easily done using the PCDs, but outside (say you allocate a tra= mpoline buffer and then free it), you would need to manually query the permi= ssions, store them, and restore later. I did *not* look into the implementation code in detail, but does the new me= mory permission protocol impose the same constraint implementation-wise and i= f so, is this documented anywhere? PS: Fetched the wrong link in my last mail: https://lkml.org/lkml/2022/12/15= /352 Best regards, Marvin= --Apple-Mail-A97EA42A-B787-4532-BCC6-A9B90DB1F692 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable
On 7. Feb 2023, at 11:01, A= rd Biesheuvel <ardb@kernel.org> wrote:

Actually, it seems UnprotectUefiImage () is corrent unde= r the
assumption that all code regions have EFI_MEMORY_XP cl= eared by
default.

However, i= f you redefine the policy to set EFI_MEMORY_XP on code
regio= ns by default, and only permit execution after remapping the code
= read-only explicitly, and only then clearing EFI_MEMORY_XP, that
routine should revert the region to EFI_MEMORY_XP. But given the<= /span>
existing ASSERT()s on having EFI_MEMORY_XP cleared for all c= ode
regions, the code as it is currently is not incorrect.

Right. My main issue is, it=E2=80=99s n= owhere documented that manually changed permissions must be restored to thei= r default before freeing. Within DxeCore, this is easily done using the PCDs= , but outside (say you allocate a trampoline buffer and then free it), you w= ould need to manually query the permissions, store them, and restore later.<= /div>

I did *not* look into the implementation code in de= tail, but does the new memory permission protocol impose the same constraint= implementation-wise and if so, is this documented anywhere?

<= /div>
PS: Fetched the wrong link in my last mail: https://lkml.org/lkml/2022/12/15/352

Best regards,
Marvin
= --Apple-Mail-A97EA42A-B787-4532-BCC6-A9B90DB1F692--