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

2023-10-05 Thread Gerd Hoffmann 
On Thu, Oct 05, 2023 at 10:23:25AM +0200, Laszlo Ersek wrote:
> 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.

I think the memory protection profiles do not configure whenever
EFI_MEMORY_ATTRIBUTE_PROTOCOL is exposed or not.  Adding a switch
there makes sense to me though.

I do not expect fixing shim will take years.  Right now shim updates are
blocked by microsoft being strict on w^x when it comes to secure boot
signing and the x86 linux kernels not being w^x clean yet.  Fixes are
underway (thanks Ard!) and should land in the next (6.7) merge window.
shim updates should follow shortly thereafter.  New distro releases and
boot media updates for LTS distros are the final steps in fixing the
current linux boot loader mess.  I expect the need for these tweaks
goes away for supported linux distros in the first half of next year.

Of course there are use cases where you want boot older (buggy) distro
boot media, so having a runtime switch for this would be nice.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109348): https://edk2.groups.io/g/devel/message/109348
Mute This Topic: https://groups.io/mt/99631663/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2023-10-05 Thread Laszlo Ersek 
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 Boot 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

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

2023-10-05 Thread Nhi Pham via 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  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 Boot 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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2023-06-23 Thread Sean 
I think that is an interesting idea but I would expect some push back 
from OS loader maintainers. I would expect they don't want to be 
constrained by the lowest common capabilities of the platforms they 
still support/run on in the ecosystem.   Not to mention the challenges 
around servicing and/or updating for bugs or features.


Example: Shim would not have been able to implement their version of 
SBAT in said scenario.


I know the Windows Boot team has been cautious about taking a dependency 
on the platform's UEFI and I would expect strong push back on them 
moving to using the platform's provided UEFI loader.


But I do agree with your goals.  Is there a better way using open 
source?  Could the PE loader/authenticode be a library managed as it's 
own project and be integrated into other pre-boot applications?   Would 
that help to eliminate bugs like this one and provide a better 
infrastructure to build on?


Thanks

Sean



On 6/23/2023 9:26 AM, Ard Biesheuvel wrote:

On Tue, 20 Jun 2023 at 19:07, Sean Brogan  wrote:

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.


Fair enough.

But I will note that the only reason we are in this situation in the
first place is because shim has to re-implement the PE loader, cert
handling and all related crypto, and needs the memory attributes
protocol to manipulate the RO/NX permissions.

Now that we are taking these things seriously, wouldn't it be *much*
better to get rid of all this cruft, and specify a method by which a
loader can provide an ephemeral DB that the system firmware will
authenticate against? That way, we can reduce shim to a single
SetVariable() call that creates the ephemeral DB (and perhaps a call
into the TPM code to measure it), which is arguably a lot easier to
audit than the code we have today.



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106310): https://edk2.groups.io/g/devel/message/106310
Mute This Topic: https://groups.io/mt/99631663/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2023-06-23 Thread Ard Biesheuvel 
On Tue, 20 Jun 2023 at 19:07, Sean Brogan  wrote:
>
> 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.
>

Fair enough.

But I will note that the only reason we are in this situation in the
first place is because shim has to re-implement the PE loader, cert
handling and all related crypto, and needs the memory attributes
protocol to manipulate the RO/NX permissions.

Now that we are taking these things seriously, wouldn't it be *much*
better to get rid of all this cruft, and specify a method by which a
loader can provide an ephemeral DB that the system firmware will
authenticate against? That way, we can reduce shim to a single
SetVariable() call that creates the ephemeral DB (and perhaps a call
into the TPM code to measure it), which is arguably a lot easier to
audit than the code we have today.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106299): https://edk2.groups.io/g/devel/message/106299
Mute This Topic: https://groups.io/mt/99631663/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2023-06-20 Thread Sean 
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 Boot 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

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

2023-06-20 Thread Gerd Hoffmann 
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 Boot 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 (#106220): https://edk2.groups.io/g/devel/message/106220
Mute This Topic: https://groups.io/mt/99631663/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2023-06-20 Thread Michael Kubacki 

We recently encountered this issue as well.

For reference, some details are in this issue:
https://github.com/microsoft/mu_silicon_arm_tiano/issues/124

Specifically, this comment:
https://github.com/microsoft/mu_silicon_arm_tiano/issues/124#issuecomment-1593550704

On 6/20/2023 9:16 AM, 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)


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.


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.


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.






 > --- a/ArmPkg/ArmPkg.dec
 > +++ b/ArmPkg/ArmPkg.dec
 > @@ -172,6 +172,9 @@ [PcdsFixedAtBuild.common]
 >   
gArmTokenSpaceGuid.PcdCpuVectorBaseAddress|0x|UINT64|0x0004

 >    gArmTokenSpaceGuid.PcdCpuResetAddress|0x|UINT32|0x0005
 >
 > +  # Enable/Disable EFI_MEMORY_ATTRIBUTE_PROTOCOL
 > + 
gArmTokenSpaceGuid.PcdEnableEfiMemoryAttributeProtocol|TRUE|BOOLEAN|0x00EE

 > +
 >    #
 >    # ARM Secure Firmware PCDs
 >    #

Given that I expect we will run into the very same problem on x64 as
soon as EFI_MEMORY_ATTRIBUTE_PROTOCOL gets enabled there we should
probably define the PCD in MdePkg not ArmPkg (which implies splitting
the patch into a mini series with one MdePkg and one ArmPkg patch).

take care,
   Gerd





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106219): https://edk2.groups.io/g/devel/message/106219
Mute This Topic: https://groups.io/mt/99631663/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2023-06-20 Thread Ard Biesheuvel 
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)

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.

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.

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.





> --- a/ArmPkg/ArmPkg.dec
> > +++ b/ArmPkg/ArmPkg.dec
> > @@ -172,6 +172,9 @@ [PcdsFixedAtBuild.common]
> >
> gArmTokenSpaceGuid.PcdCpuVectorBaseAddress|0x|UINT64|0x0004
> >gArmTokenSpaceGuid.PcdCpuResetAddress|0x|UINT32|0x0005
> >
> > +  # Enable/Disable EFI_MEMORY_ATTRIBUTE_PROTOCOL
> > +
> gArmTokenSpaceGuid.PcdEnableEfiMemoryAttributeProtocol|TRUE|BOOLEAN|0x00EE
> > +
> >#
> ># ARM Secure Firmware PCDs
> >#
>
> Given that I expect we will run into the very same problem on x64 as
> soon as EFI_MEMORY_ATTRIBUTE_PROTOCOL gets enabled there we should
> probably define the PCD in MdePkg not ArmPkg (which implies splitting
> the patch into a mini series with one MdePkg and one ArmPkg patch).
>
> take care,
>   Gerd
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106214): https://edk2.groups.io/g/devel/message/106214
Mute This Topic: https://groups.io/mt/99631663/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2023-06-20 Thread Gerd Hoffmann 
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.

> --- a/ArmPkg/ArmPkg.dec
> +++ b/ArmPkg/ArmPkg.dec
> @@ -172,6 +172,9 @@ [PcdsFixedAtBuild.common]
>gArmTokenSpaceGuid.PcdCpuVectorBaseAddress|0x|UINT64|0x0004
>gArmTokenSpaceGuid.PcdCpuResetAddress|0x|UINT32|0x0005
>  
> +  # Enable/Disable EFI_MEMORY_ATTRIBUTE_PROTOCOL
> +  
> gArmTokenSpaceGuid.PcdEnableEfiMemoryAttributeProtocol|TRUE|BOOLEAN|0x00EE
> +
>#
># ARM Secure Firmware PCDs
>#

Given that I expect we will run into the very same problem on x64 as
soon as EFI_MEMORY_ATTRIBUTE_PROTOCOL gets enabled there we should
probably define the PCD in MdePkg not ArmPkg (which implies splitting
the patch into a mini series with one MdePkg and one ArmPkg patch).

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106199): https://edk2.groups.io/g/devel/message/106199
Mute This Topic: https://groups.io/mt/99631663/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2023-06-19 Thread Oliver Steffen 
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').

Additionally, check the return code of the protocol installation calls.

Signed-off-by: Oliver Steffen 
---
PR: https://github.com/tianocore/edk2/pull/4560
---
 ArmPkg/ArmPkg.dec|  3 +++
 ArmPkg/Drivers/CpuDxe/CpuDxe.inf |  1 +
 ArmPkg/Drivers/CpuDxe/CpuDxe.c   | 13 +++--
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
index 1a16d044c94b..bd612c8c4b93 100644
--- a/ArmPkg/ArmPkg.dec
+++ b/ArmPkg/ArmPkg.dec
@@ -172,6 +172,9 @@ [PcdsFixedAtBuild.common]
   gArmTokenSpaceGuid.PcdCpuVectorBaseAddress|0x|UINT64|0x0004
   gArmTokenSpaceGuid.PcdCpuResetAddress|0x|UINT32|0x0005
 
+  # Enable/Disable EFI_MEMORY_ATTRIBUTE_PROTOCOL
+  
gArmTokenSpaceGuid.PcdEnableEfiMemoryAttributeProtocol|TRUE|BOOLEAN|0x00EE
+
   #
   # ARM Secure Firmware PCDs
   #
diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
index 7d8132200e64..feb4f28b65f1 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
@@ -66,6 +66,7 @@ [Guids]
 [Pcd.common]
   gArmTokenSpaceGuid.PcdVFPEnabled
   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy
+  gArmTokenSpaceGuid.PcdEnableEfiMemoryAttributeProtocol
 
 [FeaturePcd.common]
   gArmTokenSpaceGuid.PcdDebuggerExceptionSupport
diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
index f820f3f62189..927b91bde36e 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
@@ -329,10 +329,19 @@ CpuDxeInitialize (
   ,
   ,
   ,
-  ,
-  ,
   NULL
   );
+  ASSERT_EFI_ERROR (Status);
+
+  if (PcdGetBool (PcdEnableEfiMemoryAttributeProtocol)) {
+Status = gBS->InstallMultipleProtocolInterfaces (
+,
+,
+,
+NULL
+);
+ASSERT_EFI_ERROR (Status);
+  }
 
   //
   // Make sure GCD and MMU settings match. This API calls 
gDS->SetMemorySpaceAttributes ()
-- 
2.41.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106181): https://edk2.groups.io/g/devel/message/106181
Mute This Topic: https://groups.io/mt/99631663/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-