When the memory protections were implemented and enabled on ArmVirtQemu
5+ years ago, we had to work around the fact that GRUB at the time
expected EFI_LOADER_DATA to be executable, as that is the memory type it
allocates when loading its modules.

This has been fixed in GRUB in August 2017, so by now, we should be able
to tighten this, and remove execute permissions from EFI_LOADER_DATA
allocations.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmVirtPkg/ArmVirt.dsc.inc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 34575585adbb..462073517a22 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -368,7 +368,7 @@ [PcdsFixedAtBuild.common]
   # reserved ones, with the exception of LoaderData regions, of which OS loaders
   # (i.e., GRUB) may assume that its contents are executable.
   #
-  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD1
+  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD5
 
 [Components.common]
   #
-- 
2.35.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#94326): https://edk2.groups.io/g/devel/message/94326
Mute This Topic: https://groups.io/mt/93922691/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Mon, Sep 26, 2022 at 10:24:58AM +0200, Ard Biesheuvel wrote:
> When the memory protections were implemented and enabled on ArmVirtQemu
> 5+ years ago, we had to work around the fact that GRUB at the time
> expected EFI_LOADER_DATA to be executable, as that is the memory type it
> allocates when loading its modules.
> 
> This has been fixed in GRUB in August 2017, so by now, we should be able
> to tighten this, and remove execute permissions from EFI_LOADER_DATA
> allocations.

Data point: https://bugzilla.redhat.com/show_bug.cgi?id=2149020
tl;dr: fedora 37 grub.efi is still broken.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#96655): https://edk2.groups.io/g/devel/message/96655
Mute This Topic: https://groups.io/mt/93922691/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Mon, Nov 28, 2022 at 04:46:10PM +0100, Gerd Hoffmann wrote:
> On Mon, Sep 26, 2022 at 10:24:58AM +0200, Ard Biesheuvel wrote:
> > When the memory protections were implemented and enabled on ArmVirtQemu
> > 5+ years ago, we had to work around the fact that GRUB at the time
> > expected EFI_LOADER_DATA to be executable, as that is the memory type it
> > allocates when loading its modules.
> > 
> > This has been fixed in GRUB in August 2017, so by now, we should be able
> > to tighten this, and remove execute permissions from EFI_LOADER_DATA
> > allocations.
> 
> Data point: https://bugzilla.redhat.com/show_bug.cgi?id=2149020
> tl;dr: fedora 37 grub.efi is still broken.

This is also the case with existing Ubuntu releases, as well as
AlmaLinux 9.1 and RHEL 8.7[*]. While it does appear to be fixed for
the upcoming Ubuntu 23.04 (presumably via [**]), I plan to revert this
patch in Debian/Ubuntu until it is more ubiquitous. Do you want to do
the same upstream? I'm not sure at what point it would make sense to
reintroduce it, given we can't force users to upgrade their bootloaders.

   -dann

[*] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1025656
[**] https://git.launchpad.net/~ubuntu-core-dev/grub/+git/ubuntu/commit/?id=a0ee822f1c85fcf55066996ab51c5cf77e2728b2)


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97814): https://edk2.groups.io/g/devel/message/97814
Mute This Topic: https://groups.io/mt/93922691/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Thu, 29 Dec 2022 at 19:00, dann frazier <dann.frazier@canonical.com> wrote:
>
> On Mon, Nov 28, 2022 at 04:46:10PM +0100, Gerd Hoffmann wrote:
> > On Mon, Sep 26, 2022 at 10:24:58AM +0200, Ard Biesheuvel wrote:
> > > When the memory protections were implemented and enabled on ArmVirtQemu
> > > 5+ years ago, we had to work around the fact that GRUB at the time
> > > expected EFI_LOADER_DATA to be executable, as that is the memory type it
> > > allocates when loading its modules.
> > >
> > > This has been fixed in GRUB in August 2017, so by now, we should be able
> > > to tighten this, and remove execute permissions from EFI_LOADER_DATA
> > > allocations.
> >
> > Data point: https://bugzilla.redhat.com/show_bug.cgi?id=2149020
> > tl;dr: fedora 37 grub.efi is still broken.
>
> This is also the case with existing Ubuntu releases, as well as
> AlmaLinux 9.1 and RHEL 8.7[*]. While it does appear to be fixed for
> the upcoming Ubuntu 23.04 (presumably via [**]), I plan to revert this
> patch in Debian/Ubuntu until it is more ubiquitous. Do you want to do
> the same upstream? I'm not sure at what point it would make sense to
> reintroduce it, given we can't force users to upgrade their bootloaders.
>

Thanks for the report.

You can override PCDs on the build command line, so I suggest you use
that for building these images as long as it is needed.

E.g,, append this to the build.sh command line

--pcd PcdDxeNxMemoryProtectionPolicy=0xC000000000007FD1

to undo the effects of this patch.

I do not intend to revert this patch - the trend under EFI is towards
much stricter memory permissions, also on the MS side, and this is
especially important under CC scenarios. And if 5+ years is not
sufficient for out-of-tree GRUB to catch up, what is the point of
waiting for it?

Thanks,
Ard.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97865): https://edk2.groups.io/g/devel/message/97865
Mute This Topic: https://groups.io/mt/93922691/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hey Ard,

On 03.01.23 10:59, Ard Biesheuvel wrote:
> On Thu, 29 Dec 2022 at 19:00, dann frazier <dann.frazier@canonical.com> wrote:
>> On Mon, Nov 28, 2022 at 04:46:10PM +0100, Gerd Hoffmann wrote:
>>> On Mon, Sep 26, 2022 at 10:24:58AM +0200, Ard Biesheuvel wrote:
>>>> When the memory protections were implemented and enabled on ArmVirtQemu
>>>> 5+ years ago, we had to work around the fact that GRUB at the time
>>>> expected EFI_LOADER_DATA to be executable, as that is the memory type it
>>>> allocates when loading its modules.
>>>>
>>>> This has been fixed in GRUB in August 2017, so by now, we should be able
>>>> to tighten this, and remove execute permissions from EFI_LOADER_DATA
>>>> allocations.
>>> Data point: https://bugzilla.redhat.com/show_bug.cgi?id=2149020
>>> tl;dr: fedora 37 grub.efi is still broken.
>> This is also the case with existing Ubuntu releases, as well as
>> AlmaLinux 9.1 and RHEL 8.7[*]. While it does appear to be fixed for
>> the upcoming Ubuntu 23.04 (presumably via [**]), I plan to revert this
>> patch in Debian/Ubuntu until it is more ubiquitous. Do you want to do
>> the same upstream? I'm not sure at what point it would make sense to
>> reintroduce it, given we can't force users to upgrade their bootloaders.
>>
> Thanks for the report.
>
> You can override PCDs on the build command line, so I suggest you use
> that for building these images as long as it is needed.
>
> E.g,, append this to the build.sh command line
>
> --pcd PcdDxeNxMemoryProtectionPolicy=0xC000000000007FD1
>
> to undo the effects of this patch.
>
> I do not intend to revert this patch - the trend under EFI is towards
> much stricter memory permissions, also on the MS side, and this is
> especially important under CC scenarios. And if 5+ years is not
> sufficient for out-of-tree GRUB to catch up, what is the point of
> waiting for it?


I think we need to be smarter here. ArmVirtPkg is used by a lot of 
virtualization software - such as QEMU. Typically, users (and 
developers) expect that an update to a newer version will preserve 
compatibility.

Let me contrive an example: In 1 year, QEMU updates to the latest AAVMF. 
It ships that as part of its pc-bios directory. Suddenly, we 
accidentally break old (immutable!) iso images that used to work. So 
someone that updates QEMU to a newer version will have a regression in 
running a Fedora 37 installation. Or RHEL 8.7 installation. Not good :).

Outside of OSVs providing new iso images, there is also not much you can 
do about this. The grub binary here runs way before any updater code 
that could pull a fixed version from the internet.

What alternatives do we have? Assuming grub is the only offender we have 
to worry about, is there a way we can identify that the allocation came 
from an unpatched version? Or have a database of hashes (with all known 
grub binaries that have this bug in existing isos) that would force 
disable NX protection for it if we hit it? Or disable NX when Secure 
Boot is disabled?

I really think we need to be a bit more creative than make NX mandatory 
in all cases when we know the are immutable images out there that won't 
work with it, otherwise we break very legit use cases.


Alex



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97890): https://edk2.groups.io/g/devel/message/97890
Mute This Topic: https://groups.io/mt/93922691/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Tue, Jan 03, 2023 at 08:39:24PM +0100, Alexander Graf wrote:
> Hey Ard,
> 
> On 03.01.23 10:59, Ard Biesheuvel wrote:
> > On Thu, 29 Dec 2022 at 19:00, dann frazier <dann.frazier@canonical.com> wrote:
> > > On Mon, Nov 28, 2022 at 04:46:10PM +0100, Gerd Hoffmann wrote:
> > > > On Mon, Sep 26, 2022 at 10:24:58AM +0200, Ard Biesheuvel wrote:
> > > > > When the memory protections were implemented and enabled on ArmVirtQemu
> > > > > 5+ years ago, we had to work around the fact that GRUB at the time
> > > > > expected EFI_LOADER_DATA to be executable, as that is the memory type it
> > > > > allocates when loading its modules.
> > > > > 
> > > > > This has been fixed in GRUB in August 2017, so by now, we should be able
> > > > > to tighten this, and remove execute permissions from EFI_LOADER_DATA
> > > > > allocations.
> > > > Data point: https://bugzilla.redhat.com/show_bug.cgi?id=2149020
> > > > tl;dr: fedora 37 grub.efi is still broken.
> > > This is also the case with existing Ubuntu releases, as well as
> > > AlmaLinux 9.1 and RHEL 8.7[*]. While it does appear to be fixed for
> > > the upcoming Ubuntu 23.04 (presumably via [**]), I plan to revert this
> > > patch in Debian/Ubuntu until it is more ubiquitous. Do you want to do
> > > the same upstream? I'm not sure at what point it would make sense to
> > > reintroduce it, given we can't force users to upgrade their bootloaders.
> > > 
> > Thanks for the report.
> > 
> > You can override PCDs on the build command line, so I suggest you use
> > that for building these images as long as it is needed.
> > 
> > E.g,, append this to the build.sh command line
> > 
> > --pcd PcdDxeNxMemoryProtectionPolicy=0xC000000000007FD1
> > 
> > to undo the effects of this patch.
> > 
> > I do not intend to revert this patch - the trend under EFI is towards
> > much stricter memory permissions, also on the MS side, and this is
> > especially important under CC scenarios. And if 5+ years is not
> > sufficient for out-of-tree GRUB to catch up, what is the point of
> > waiting for it?
> 
> 
> I think we need to be smarter here. ArmVirtPkg is used by a lot of
> virtualization software - such as QEMU. Typically, users (and developers)
> expect that an update to a newer version will preserve compatibility.
> 
> Let me contrive an example: In 1 year, QEMU updates to the latest AAVMF. It
> ships that as part of its pc-bios directory. Suddenly, we accidentally break
> old (immutable!) iso images that used to work. So someone that updates QEMU
> to a newer version will have a regression in running a Fedora 37
> installation. Or RHEL 8.7 installation. Not good :).
> 
> Outside of OSVs providing new iso images, there is also not much you can do
> about this. The grub binary here runs way before any updater code that could
> pull a fixed version from the internet.
> 
> What alternatives do we have? Assuming grub is the only offender we have to
> worry about, is there a way we can identify that the allocation came from an
> unpatched version? Or have a database of hashes (with all known grub
> binaries that have this bug in existing isos) that would force disable NX
> protection for it if we hit it? Or disable NX when Secure Boot is disabled?
> 
> I really think we need to be a bit more creative than make NX mandatory in
> all cases when we know the are immutable images out there that won't work
> with it, otherwise we break very legit use cases.

While it has its own issues, I suppose another way to go about it is
for distributors to provide multiple AAVMF_CODE images, and perhaps
invent a "feature" flag for the json descriptor for management tools
to select an appropriate one.

  -dann


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97893): https://edk2.groups.io/g/devel/message/97893
Mute This Topic: https://groups.io/mt/93922691/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Tue, 3 Jan 2023 at 23:47, dann frazier <dann.frazier@canonical.com> wrote:
>
> On Tue, Jan 03, 2023 at 08:39:24PM +0100, Alexander Graf wrote:
> > Hey Ard,
> >
> > On 03.01.23 10:59, Ard Biesheuvel wrote:
> > > On Thu, 29 Dec 2022 at 19:00, dann frazier <dann.frazier@canonical.com> wrote:
> > > > On Mon, Nov 28, 2022 at 04:46:10PM +0100, Gerd Hoffmann wrote:
> > > > > On Mon, Sep 26, 2022 at 10:24:58AM +0200, Ard Biesheuvel wrote:
> > > > > > When the memory protections were implemented and enabled on ArmVirtQemu
> > > > > > 5+ years ago, we had to work around the fact that GRUB at the time
> > > > > > expected EFI_LOADER_DATA to be executable, as that is the memory type it
> > > > > > allocates when loading its modules.
> > > > > >
> > > > > > This has been fixed in GRUB in August 2017, so by now, we should be able
> > > > > > to tighten this, and remove execute permissions from EFI_LOADER_DATA
> > > > > > allocations.
> > > > > Data point: https://bugzilla.redhat.com/show_bug.cgi?id=2149020
> > > > > tl;dr: fedora 37 grub.efi is still broken.
> > > > This is also the case with existing Ubuntu releases, as well as
> > > > AlmaLinux 9.1 and RHEL 8.7[*]. While it does appear to be fixed for
> > > > the upcoming Ubuntu 23.04 (presumably via [**]), I plan to revert this
> > > > patch in Debian/Ubuntu until it is more ubiquitous. Do you want to do
> > > > the same upstream? I'm not sure at what point it would make sense to
> > > > reintroduce it, given we can't force users to upgrade their bootloaders.
> > > >
> > > Thanks for the report.
> > >
> > > You can override PCDs on the build command line, so I suggest you use
> > > that for building these images as long as it is needed.
> > >
> > > E.g,, append this to the build.sh command line
> > >
> > > --pcd PcdDxeNxMemoryProtectionPolicy=0xC000000000007FD1
> > >
> > > to undo the effects of this patch.
> > >
> > > I do not intend to revert this patch - the trend under EFI is towards
> > > much stricter memory permissions, also on the MS side, and this is
> > > especially important under CC scenarios. And if 5+ years is not
> > > sufficient for out-of-tree GRUB to catch up, what is the point of
> > > waiting for it?
> >
> >
> > I think we need to be smarter here. ArmVirtPkg is used by a lot of
> > virtualization software - such as QEMU. Typically, users (and developers)
> > expect that an update to a newer version will preserve compatibility.
> >
> > Let me contrive an example: In 1 year, QEMU updates to the latest AAVMF. It
> > ships that as part of its pc-bios directory. Suddenly, we accidentally break
> > old (immutable!) iso images that used to work. So someone that updates QEMU
> > to a newer version will have a regression in running a Fedora 37
> > installation. Or RHEL 8.7 installation. Not good :).
> >
> > Outside of OSVs providing new iso images, there is also not much you can do
> > about this. The grub binary here runs way before any updater code that could
> > pull a fixed version from the internet.
> >
> > What alternatives do we have? Assuming grub is the only offender we have to
> > worry about, is there a way we can identify that the allocation came from an
> > unpatched version? Or have a database of hashes (with all known grub
> > binaries that have this bug in existing isos) that would force disable NX
> > protection for it if we hit it? Or disable NX when Secure Boot is disabled?
> >
> > I really think we need to be a bit more creative than make NX mandatory in
> > all cases when we know the are immutable images out there that won't work
> > with it, otherwise we break very legit use cases.
>
> While it has its own issues, I suppose another way to go about it is
> for distributors to provide multiple AAVMF_CODE images, and perhaps
> invent a "feature" flag for the json descriptor for management tools
> to select an appropriate one.
>

I don't think having different versions of the image makes sense, tbh,
but of course, this is up to the distros.

Compatibility with ancient downstream GRUB builds is not a goal of the
EDK2 upstream, so as long as distros can tweak the build to their
needs, I don't see a reason to revert this change upstream.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97927): https://edk2.groups.io/g/devel/message/97927
Mute This Topic: https://groups.io/mt/93922691/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 04.01.23 10:35, Ard Biesheuvel wrote:
> On Tue, 3 Jan 2023 at 23:47, dann frazier <dann.frazier@canonical.com> wrote:
>> On Tue, Jan 03, 2023 at 08:39:24PM +0100, Alexander Graf wrote:
>>> Hey Ard,
>>>
>>> On 03.01.23 10:59, Ard Biesheuvel wrote:
>>>> On Thu, 29 Dec 2022 at 19:00, dann frazier <dann.frazier@canonical.com> wrote:
>>>>> On Mon, Nov 28, 2022 at 04:46:10PM +0100, Gerd Hoffmann wrote:
>>>>>> On Mon, Sep 26, 2022 at 10:24:58AM +0200, Ard Biesheuvel wrote:
>>>>>>> When the memory protections were implemented and enabled on ArmVirtQemu
>>>>>>> 5+ years ago, we had to work around the fact that GRUB at the time
>>>>>>> expected EFI_LOADER_DATA to be executable, as that is the memory type it
>>>>>>> allocates when loading its modules.
>>>>>>>
>>>>>>> This has been fixed in GRUB in August 2017, so by now, we should be able
>>>>>>> to tighten this, and remove execute permissions from EFI_LOADER_DATA
>>>>>>> allocations.
>>>>>> Data point: https://bugzilla.redhat.com/show_bug.cgi?id=2149020
>>>>>> tl;dr: fedora 37 grub.efi is still broken.
>>>>> This is also the case with existing Ubuntu releases, as well as
>>>>> AlmaLinux 9.1 and RHEL 8.7[*]. While it does appear to be fixed for
>>>>> the upcoming Ubuntu 23.04 (presumably via [**]), I plan to revert this
>>>>> patch in Debian/Ubuntu until it is more ubiquitous. Do you want to do
>>>>> the same upstream? I'm not sure at what point it would make sense to
>>>>> reintroduce it, given we can't force users to upgrade their bootloaders.
>>>>>
>>>> Thanks for the report.
>>>>
>>>> You can override PCDs on the build command line, so I suggest you use
>>>> that for building these images as long as it is needed.
>>>>
>>>> E.g,, append this to the build.sh command line
>>>>
>>>> --pcd PcdDxeNxMemoryProtectionPolicy=0xC000000000007FD1
>>>>
>>>> to undo the effects of this patch.
>>>>
>>>> I do not intend to revert this patch - the trend under EFI is towards
>>>> much stricter memory permissions, also on the MS side, and this is
>>>> especially important under CC scenarios. And if 5+ years is not
>>>> sufficient for out-of-tree GRUB to catch up, what is the point of
>>>> waiting for it?
>>>
>>> I think we need to be smarter here. ArmVirtPkg is used by a lot of
>>> virtualization software - such as QEMU. Typically, users (and developers)
>>> expect that an update to a newer version will preserve compatibility.
>>>
>>> Let me contrive an example: In 1 year, QEMU updates to the latest AAVMF. It
>>> ships that as part of its pc-bios directory. Suddenly, we accidentally break
>>> old (immutable!) iso images that used to work. So someone that updates QEMU
>>> to a newer version will have a regression in running a Fedora 37
>>> installation. Or RHEL 8.7 installation. Not good :).
>>>
>>> Outside of OSVs providing new iso images, there is also not much you can do
>>> about this. The grub binary here runs way before any updater code that could
>>> pull a fixed version from the internet.
>>>
>>> What alternatives do we have? Assuming grub is the only offender we have to
>>> worry about, is there a way we can identify that the allocation came from an
>>> unpatched version? Or have a database of hashes (with all known grub
>>> binaries that have this bug in existing isos) that would force disable NX
>>> protection for it if we hit it? Or disable NX when Secure Boot is disabled?
>>>
>>> I really think we need to be a bit more creative than make NX mandatory in
>>> all cases when we know the are immutable images out there that won't work
>>> with it, otherwise we break very legit use cases.
>> While it has its own issues, I suppose another way to go about it is
>> for distributors to provide multiple AAVMF_CODE images, and perhaps
>> invent a "feature" flag for the json descriptor for management tools
>> to select an appropriate one.
>>
> I don't think having different versions of the image makes sense, tbh,
> but of course, this is up to the distros.
>
> Compatibility with ancient downstream GRUB builds is not a goal of the
> EDK2 upstream, so as long as distros can tweak the build to their
> needs, I don't see a reason to revert this change upstream.


First of all, I don't think we should revert this change. We should 
augment it to give users the out-of-the-box experience they expect.

On top of that, I don't think it's a problem of "distros". Every 
consumer of AAVMF will run into this problem as soon as their users will 
want to run any Red Hat OS (installer / image) all the way into 2022. 
That's  very likely >90% of the user base. Because of that, I'm pretty 
sure no Cloud vendor will be able to enable NX in its current shape for 
example.

I'm very happy to see NX proliferate through the stack, but let's please 
make sure we do it compatibly by default, otherwise the net result is 
that *everyone* who compiles AAVMF will disable NX by default again - 
and all you end up with is more frustration and more downstream code / 
forks.

IMHO the most obvious approach would be a fingerprint based override. 
There should be a finite number of known broken grub binaries. If we 
just maintain a database with them and then apply some magic when we 
detect such a binary gets loaded, we'll have tightened security by 
default, without breaking backwards compat.

For environments that know they're running in environments with CC 
requirements, we can automatically disable the fingerprint override :).


Alex



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97937): https://edk2.groups.io/g/devel/message/97937
Mute This Topic: https://groups.io/mt/93922691/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 04.01.23 14:13, Alexander Graf wrote:
>
> On 04.01.23 10:35, Ard Biesheuvel wrote:
>> On Tue, 3 Jan 2023 at 23:47, dann frazier 
>> <dann.frazier@canonical.com> wrote:
>>> On Tue, Jan 03, 2023 at 08:39:24PM +0100, Alexander Graf wrote:
>>>> Hey Ard,
>>>>
>>>> On 03.01.23 10:59, Ard Biesheuvel wrote:
>>>>> On Thu, 29 Dec 2022 at 19:00, dann frazier 
>>>>> <dann.frazier@canonical.com> wrote:
>>>>>> On Mon, Nov 28, 2022 at 04:46:10PM +0100, Gerd Hoffmann wrote:
>>>>>>> On Mon, Sep 26, 2022 at 10:24:58AM +0200, Ard Biesheuvel wrote:
>>>>>>>> When the memory protections were implemented and enabled on 
>>>>>>>> ArmVirtQemu
>>>>>>>> 5+ years ago, we had to work around the fact that GRUB at the time
>>>>>>>> expected EFI_LOADER_DATA to be executable, as that is the 
>>>>>>>> memory type it
>>>>>>>> allocates when loading its modules.
>>>>>>>>
>>>>>>>> This has been fixed in GRUB in August 2017, so by now, we 
>>>>>>>> should be able
>>>>>>>> to tighten this, and remove execute permissions from 
>>>>>>>> EFI_LOADER_DATA
>>>>>>>> allocations.
>>>>>>> Data point: https://bugzilla.redhat.com/show_bug.cgi?id=2149020
>>>>>>> tl;dr: fedora 37 grub.efi is still broken.
>>>>>> This is also the case with existing Ubuntu releases, as well as
>>>>>> AlmaLinux 9.1 and RHEL 8.7[*]. While it does appear to be fixed for
>>>>>> the upcoming Ubuntu 23.04 (presumably via [**]), I plan to revert 
>>>>>> this
>>>>>> patch in Debian/Ubuntu until it is more ubiquitous. Do you want 
>>>>>> to do
>>>>>> the same upstream? I'm not sure at what point it would make sense to
>>>>>> reintroduce it, given we can't force users to upgrade their 
>>>>>> bootloaders.
>>>>>>
>>>>> Thanks for the report.
>>>>>
>>>>> You can override PCDs on the build command line, so I suggest you use
>>>>> that for building these images as long as it is needed.
>>>>>
>>>>> E.g,, append this to the build.sh command line
>>>>>
>>>>> --pcd PcdDxeNxMemoryProtectionPolicy=0xC000000000007FD1
>>>>>
>>>>> to undo the effects of this patch.
>>>>>
>>>>> I do not intend to revert this patch - the trend under EFI is towards
>>>>> much stricter memory permissions, also on the MS side, and this is
>>>>> especially important under CC scenarios. And if 5+ years is not
>>>>> sufficient for out-of-tree GRUB to catch up, what is the point of
>>>>> waiting for it?
>>>>
>>>> I think we need to be smarter here. ArmVirtPkg is used by a lot of
>>>> virtualization software - such as QEMU. Typically, users (and 
>>>> developers)
>>>> expect that an update to a newer version will preserve compatibility.
>>>>
>>>> Let me contrive an example: In 1 year, QEMU updates to the latest 
>>>> AAVMF. It
>>>> ships that as part of its pc-bios directory. Suddenly, we 
>>>> accidentally break
>>>> old (immutable!) iso images that used to work. So someone that 
>>>> updates QEMU
>>>> to a newer version will have a regression in running a Fedora 37
>>>> installation. Or RHEL 8.7 installation. Not good :).
>>>>
>>>> Outside of OSVs providing new iso images, there is also not much 
>>>> you can do
>>>> about this. The grub binary here runs way before any updater code 
>>>> that could
>>>> pull a fixed version from the internet.
>>>>
>>>> What alternatives do we have? Assuming grub is the only offender we 
>>>> have to
>>>> worry about, is there a way we can identify that the allocation 
>>>> came from an
>>>> unpatched version? Or have a database of hashes (with all known grub
>>>> binaries that have this bug in existing isos) that would force 
>>>> disable NX
>>>> protection for it if we hit it? Or disable NX when Secure Boot is 
>>>> disabled?
>>>>
>>>> I really think we need to be a bit more creative than make NX 
>>>> mandatory in
>>>> all cases when we know the are immutable images out there that 
>>>> won't work
>>>> with it, otherwise we break very legit use cases.
>>> While it has its own issues, I suppose another way to go about it is
>>> for distributors to provide multiple AAVMF_CODE images, and perhaps
>>> invent a "feature" flag for the json descriptor for management tools
>>> to select an appropriate one.
>>>
>> I don't think having different versions of the image makes sense, tbh,
>> but of course, this is up to the distros.
>>
>> Compatibility with ancient downstream GRUB builds is not a goal of the
>> EDK2 upstream, so as long as distros can tweak the build to their
>> needs, I don't see a reason to revert this change upstream.
>
>
> First of all, I don't think we should revert this change. We should 
> augment it to give users the out-of-the-box experience they expect.
>
> On top of that, I don't think it's a problem of "distros". Every 
> consumer of AAVMF will run into this problem as soon as their users 
> will want to run any Red Hat OS (installer / image) all the way into 
> 2022. That's  very likely >90% of the user base. Because of that, I'm 
> pretty sure no Cloud vendor will be able to enable NX in its current 
> shape for example.
>
> I'm very happy to see NX proliferate through the stack, but let's 
> please make sure we do it compatibly by default, otherwise the net 
> result is that *everyone* who compiles AAVMF will disable NX by 
> default again - and all you end up with is more frustration and more 
> downstream code / forks.
>
> IMHO the most obvious approach would be a fingerprint based override. 
> There should be a finite number of known broken grub binaries. If we 
> just maintain a database with them and then apply some magic when we 
> detect such a binary gets loaded, we'll have tightened security by 
> default, without breaking backwards compat.
>
> For environments that know they're running in environments with CC 
> requirements, we can automatically disable the fingerprint override :).


To clarify, I mean something like the patch below, but with an 
additional callback notification similar to the Emu one in LoadImage(), 
so that we can make sure we only enable the quirk when we load a 
known-bad grub binary. That way we still force distros to ship fixed 
versions of their code, but enable old code to continue running.


Alex


diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c 
b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
index 3ad1ecd9d2..365eb1c157 100644
--- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
@@ -902,6 +902,25 @@ PlatformBootManagerBeforeConsole (
    FilterAndProcess (&gEfiPciIoProtocolGuid, IsVirtioPciRng, Connect);
  }

+static EFI_ALLOCATE_PAGES RealAllocatePages;
+
+static EFI_STATUS EFIAPI AllocatePagesForceLoaderCode(
+  IN     EFI_ALLOCATE_TYPE            Type,
+  IN     EFI_MEMORY_TYPE              MemoryType,
+  IN     UINTN                        Pages,
+  IN OUT EFI_PHYSICAL_ADDRESS         *Memory
+)
+{
+    /*
+     * Broken grub versions do LoaderData allocations for code. Let's patch
+     * them into LoaderCode allocations instead.
+     */
+    if (MemoryType == EfiLoaderData)
+        MemoryType = EfiLoaderCode;
+
+    return RealAllocatePages(Type, MemoryType, Pages, Memory);
+}
+
  /**
    Do the platform specific action after the console is ready
    Possible things that can be done in PlatformBootManagerAfterConsole:
@@ -964,6 +983,14 @@ PlatformBootManagerAfterConsole (
    SetBootOrderFromQemu ();

    PlatformBmPrintScRegisterHandler ();
+
+  /* TODO: Only run this as part of a notify callback in ImageLoad() 
when we
+           load a grub binary with a known-broken hash */
+  BOOLEAN is_broken_grub = TRUE;
+  if (is_broken_grub) {
+    RealAllocatePages = gBS->AllocatePages;
+    gBS->AllocatePages = AllocatePagesForceLoaderCode;
+  }
  }

  /**



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97954): https://edk2.groups.io/g/devel/message/97954
Mute This Topic: https://groups.io/mt/93922691/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

  Hi,

> To clarify, I mean something like the patch below, but with an additional
> callback notification similar to the Emu one in LoadImage(), so that we can
> make sure we only enable the quirk when we load a known-bad grub binary.
> That way we still force distros to ship fixed versions of their code, but
> enable old code to continue running.

> +  /* TODO: Only run this as part of a notify callback in ImageLoad() when
> we
> +           load a grub binary with a known-broken hash */
> +  BOOLEAN is_broken_grub = TRUE;
> +  if (is_broken_grub) {
> +    RealAllocatePages = gBS->AllocatePages;
> +    gBS->AllocatePages = AllocatePagesForceLoaderCode;
> +  }

You left out the hard part, which is the list of hashes.  And I suspect
you underestimate the number of broken grub binaries in the wild ...

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97976): https://edk2.groups.io/g/devel/message/97976
Mute This Topic: https://groups.io/mt/93922691/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> Am 05.01.2023 um 09:11 schrieb Gerd Hoffmann <kraxel@redhat.com>:
> 
>   Hi,
> 
>> To clarify, I mean something like the patch below, but with an additional
>> callback notification similar to the Emu one in LoadImage(), so that we can
>> make sure we only enable the quirk when we load a known-bad grub binary.
>> That way we still force distros to ship fixed versions of their code, but
>> enable old code to continue running.
> 
>> +  /* TODO: Only run this as part of a notify callback in ImageLoad() when
>> we
>> +           load a grub binary with a known-broken hash */
>> +  BOOLEAN is_broken_grub = TRUE;
>> +  if (is_broken_grub) {
>> +    RealAllocatePages = gBS->AllocatePages;
>> +    gBS->AllocatePages = AllocatePagesForceLoaderCode;
>> +  }
> 
> You left out the hard part, which is the list of hashes.

Yes, I'd crowd source that list. If someone has vested interest to keep their old grub binaries working, they can send an upstream patch to add their hash :). At least we'd have a path forward to make things work that is not "revert NX enablement".

>  And I suspect
> you underestimate the number of broken grub binaries in the wild ...

What number would you expect? I'd hope that we get to <100 realistically.

I'm happy to hear about alternatives to this approach. I'm very confident that forcing NX on always is going to have the opposite effect of what we want: Everyone who ships AAVMF binaries will disable NX because they eventually get bug reports from users that their shiny update regressed some legit use case.

The only alternative I can think of would be logic similar to the patch I sent without any grub hash check: Exclude AllocatePages for LoaderData from the NX logic. Keep NX for any other non-code memory type as well as LoaderData pool allocations.


Alex



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97977): https://edk2.groups.io/g/devel/message/97977
Mute This Topic: https://groups.io/mt/93922691/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Thu, 5 Jan 2023 at 09:43, Alexander Graf <agraf@csgraf.de> wrote:
>
>
>
> > Am 05.01.2023 um 09:11 schrieb Gerd Hoffmann <kraxel@redhat.com>:
> >
> >   Hi,
> >
> >> To clarify, I mean something like the patch below, but with an additional
> >> callback notification similar to the Emu one in LoadImage(), so that we can
> >> make sure we only enable the quirk when we load a known-bad grub binary.
> >> That way we still force distros to ship fixed versions of their code, but
> >> enable old code to continue running.
> >
> >> +  /* TODO: Only run this as part of a notify callback in ImageLoad() when
> >> we
> >> +           load a grub binary with a known-broken hash */
> >> +  BOOLEAN is_broken_grub = TRUE;
> >> +  if (is_broken_grub) {
> >> +    RealAllocatePages = gBS->AllocatePages;
> >> +    gBS->AllocatePages = AllocatePagesForceLoaderCode;
> >> +  }
> >
> > You left out the hard part, which is the list of hashes.
>
> Yes, I'd crowd source that list. If someone has vested interest to keep their old grub binaries working, they can send an upstream patch to add their hash :). At least we'd have a path forward to make things work that is not "revert NX enablement".
>
> >  And I suspect
> > you underestimate the number of broken grub binaries in the wild ...
>
> What number would you expect? I'd hope that we get to <100 realistically.
>
> I'm happy to hear about alternatives to this approach. I'm very confident that forcing NX on always is going to have the opposite effect of what we want: Everyone who ships AAVMF binaries will disable NX because they eventually get bug reports from users that their shiny update regressed some legit use case.
>
> The only alternative I can think of would be logic similar to the patch I sent without any grub hash check: Exclude AllocatePages for LoaderData from the NX logic. Keep NX for any other non-code memory type as well as LoaderData pool allocations.
>

Another thing we might consider is trapping exec permission violations
and switching the pages in question from rw- to r-x.

Does GRUB generally load/map executable modules at page granularity?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97981): https://edk2.groups.io/g/devel/message/97981
Mute This Topic: https://groups.io/mt/93922691/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

  Hi,

> > What number would you expect? I'd hope that we get to <100 realistically.
> >
> > I'm happy to hear about alternatives to this approach. I'm very confident that forcing NX on always is going to have the opposite effect of what we want: Everyone who ships AAVMF binaries will disable NX because they eventually get bug reports from users that their shiny update regressed some legit use case.
> >
> > The only alternative I can think of would be logic similar to the patch I sent without any grub hash check: Exclude AllocatePages for LoaderData from the NX logic. Keep NX for any other non-code memory type as well as LoaderData pool allocations.

> Another thing we might consider is trapping exec permission violations
> and switching the pages in question from rw- to r-x.

That sounds neat, especially as we can print a big'n'fat warning in that
case, so the problem gets attention without actually breaking users.

Looking at the efi calls it looks like edk2 doesn't track the owner of
an allocation (say by image handle), so I suspect it is not possible to
automatically figure who is to blame?

> Does GRUB generally load/map executable modules at page granularity?

I don't think so, at least the code handles modules not being page
aligned.  But I think it's not grub modules, that fix was actually
picked up meanwhile.  But there are downstream patches for image
loader code which look suspicious to me ...

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97982): https://edk2.groups.io/g/devel/message/97982
Mute This Topic: https://groups.io/mt/93922691/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Thu, 5 Jan 2023 at 12:19, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > What number would you expect? I'd hope that we get to <100 realistically.
> > >
> > > I'm happy to hear about alternatives to this approach. I'm very confident that forcing NX on always is going to have the opposite effect of what we want: Everyone who ships AAVMF binaries will disable NX because they eventually get bug reports from users that their shiny update regressed some legit use case.
> > >
> > > The only alternative I can think of would be logic similar to the patch I sent without any grub hash check: Exclude AllocatePages for LoaderData from the NX logic. Keep NX for any other non-code memory type as well as LoaderData pool allocations.
>
> > Another thing we might consider is trapping exec permission violations
> > and switching the pages in question from rw- to r-x.
>
> That sounds neat, especially as we can print a big'n'fat warning in that
> case, so the problem gets attention without actually breaking users.
>

That, and a sleep(5)

> Looking at the efi calls it looks like edk2 doesn't track the owner of
> an allocation (say by image handle), so I suspect it is not possible to
> automatically figure who is to blame?
>
> > Does GRUB generally load/map executable modules at page granularity?
>
> I don't think so, at least the code handles modules not being page
> aligned.  But I think it's not grub modules, that fix was actually
> picked up meanwhile.  But there are downstream patches for image
> loader code which look suspicious to me ...
>

OK, so the GRUB PE/COFF loader strikes again :-(

So shim may be broken in the exact same way, and the things shim loads
may not adhere to page alignment for the sections. Loading the kernel
itself this way should be fine, though - we always had section
alignment in the EFI stub kernel.

The downside of this approach is that it can only be done on a
page-by-page basis, given that the EFI_LOADER_DATA region in question
will cover both the .text/.rodata and the .data/.bss of the loaded
image.

Could someone check/confirm whether shim builds need to be take into
account here? Thanks.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97984): https://edk2.groups.io/g/devel/message/97984
Mute This Topic: https://groups.io/mt/93922691/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> Am 05.01.2023 um 12:44 schrieb Ard Biesheuvel <ardb@kernel.org>:
> 
> On Thu, 5 Jan 2023 at 12:19, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> 
>>  Hi,
>> 
>>>> What number would you expect? I'd hope that we get to <100 realistically.
>>>> 
>>>> I'm happy to hear about alternatives to this approach. I'm very confident that forcing NX on always is going to have the opposite effect of what we want: Everyone who ships AAVMF binaries will disable NX because they eventually get bug reports from users that their shiny update regressed some legit use case.
>>>> 
>>>> The only alternative I can think of would be logic similar to the patch I sent without any grub hash check: Exclude AllocatePages for LoaderData from the NX logic. Keep NX for any other non-code memory type as well as LoaderData pool allocations.
>> 
>>> Another thing we might consider is trapping exec permission violations
>>> and switching the pages in question from rw- to r-x.
>> 
>> That sounds neat, especially as we can print a big'n'fat warning in that
>> case, so the problem gets attention without actually breaking users.
>> 
> 
> That, and a sleep(5)

I like the direction this is moving :)

> 
>> Looking at the efi calls it looks like edk2 doesn't track the owner of
>> an allocation (say by image handle), so I suspect it is not possible to
>> automatically figure who is to blame?
>> 
>>> Does GRUB generally load/map executable modules at page granularity?
>> 
>> I don't think so, at least the code handles modules not being page
>> aligned.  But I think it's not grub modules, that fix was actually
>> picked up meanwhile.  But there are downstream patches for image
>> loader code which look suspicious to me ...
>> 
> 
> OK, so the GRUB PE/COFF loader strikes again :-(
> 
> So shim may be broken in the exact same way, and the things shim loads
> may not adhere to page alignment for the sections. Loading the kernel
> itself this way should be fine, though - we always had section
> alignment in the EFI stub kernel.
> 
> The downside of this approach is that it can only be done on a
> page-by-page basis, given that the EFI_LOADER_DATA region in question
> will cover both the .text/.rodata and the .data/.bss of the loaded
> image.

Does it have to be r-x instead of rwx? If we add the warning and sleep(5), that should hopefully give enough incentive already to OSVs to fix their grub binaries :). And then we don't even need to think about alignment.

If I map a region as LoaderCode, I get rwx as well, no? So we effectively make LoaderData behave like LoaderCode plus warning plus sleep.


Alex

> 
> Could someone check/confirm whether shim builds need to be take into
> account here? Thanks.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98038): https://edk2.groups.io/g/devel/message/98038
Mute This Topic: https://groups.io/mt/93922691/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

  Hi,

> > That sounds neat, especially as we can print a big'n'fat warning in that
> > case, so the problem gets attention without actually breaking users.
> >
> 
> That, and a sleep(5)
> 
> > Looking at the efi calls it looks like edk2 doesn't track the owner of
> > an allocation (say by image handle), so I suspect it is not possible to
> > automatically figure who is to blame?
> >
> > > Does GRUB generally load/map executable modules at page granularity?
> >
> > I don't think so, at least the code handles modules not being page
> > aligned.  But I think it's not grub modules, that fix was actually
> > picked up meanwhile.  But there are downstream patches for image
> > loader code which look suspicious to me ...
> 
> OK, so the GRUB PE/COFF loader strikes again :-(

Yep.

> Could someone check/confirm whether shim builds need to be take into
> account here? Thanks.

Tried booting grub.efi directly and via shim.efi, on Fedora 37 GA.

In both cases I get a pagefault on linux kernel boot (before any other
message is printed), which I guess happens because the loader places the
linux kernel efi stub in EfiLoaderData memory.

I'd say that confirms grub.efi being buggy.

Not sure about shim.efi.  It managed to run grub.efi without hitting a
fault, which is good.  But it also installs efi protocols for the boot
loader to call, so it could be involved too.  But maybe that happens
only in case secure boot is active, which is not supported by
ArmVirtPkg.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98001): https://edk2.groups.io/g/devel/message/98001
Mute This Topic: https://groups.io/mt/93922691/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

  Hi,

> Tried booting grub.efi directly and via shim.efi, on Fedora 37 GA.
> 
> In both cases I get a pagefault on linux kernel boot (before any other
> message is printed), which I guess happens because the loader places the
> linux kernel efi stub in EfiLoaderData memory.

When compiling ovmf with the same pcd value I get the same behavior
on x64, so it's consistently buggy across architectures.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98035): https://edk2.groups.io/g/devel/message/98035
Mute This Topic: https://groups.io/mt/93922691/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Gerd/Ard,

This is a great topic and shows some of the challenges of tightening up 
security/enforcing correctness in UEFI.

I wanted to let you know that we have been doing a lot of work in both 
edk2 firmware and discussing with partners in the Linux community and PC 
ecosystem.  The Shim and Grub teams have been directly engaged and have 
code patches that correct a number of problems as well as make their 
code compliant with even tighter restrictions.   Engineers from redhat, 
canonical, oracle, and others have all been involved.  Also note 
Microsoft's UEFI CA now requires submissions to be compliant with a 
strict set of memory mitigations. UEFI CA Memory Mitigation Requirements 
for Signing - Windows drivers | Microsoft Learn 
<https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/uefi-ca-memory-mitigation-requirements> 
and UPDATED: UEFI Signing Requirements - Microsoft Community Hub 
<https://techcommunity.microsoft.com/t5/hardware-dev-center/updated-uefi-signing-requirements/ba-p/1062916>.  
This means any new shim submitted must now meet these requirements. How 
long it takes for distros to update to a new shim is still unknown.


Hopefully sometime in the next few weeks we can prepare a comprehensive 
set of patches and changes needed in edk2 to implement this strict 
environment.  One of the relevant changes to this discussion and patch 
series is we switched the configuration from PCD to hob 
(mu_basecore/DxeMemoryProtectionSettings.h at release/202208 · 
microsoft/mu_basecore (github.com) 
<https://github.com/microsoft/mu_basecore/blob/release/202208/MdeModulePkg/Include/Guid/DxeMemoryProtectionSettings.h>). 
This allows our platforms complete control of the config per boot.  Some 
platforms have compatibility requirements and have implemented code so 
that when a PF is triggered by incompatible software, it is recorded and 
then the system rebooted.  On the next boot the platform changes the HOB 
configuration to be in a more compatible mode (this mode could be 
measured in a PCR and/or other hints to the user/system of degraded 
security).  We hope this balance makes compatibility possible but 
inconvenient and with a less than desirable user experience.  Will this 
help move the industry, I don't know?


Anyway, rather than a patchwork of changes going into different 
platforms and components of edk2 I would like to see alignment between 
x86/arm64 in edk2 and a complete set of changes for edk2. We have 
developed a tool and some tests that can capture the memory environment 
in DXE and export it to then allow a developer to review.  This has 
identified dozens of problems in edk2 code as well as platform code.  
See attached a report file showing a passing report for our q35 qemu 
based platform. mu_tiano_platforms/Platforms/QemuQ35Pkg at main · 
microsoft/mu_tiano_platforms (github.com) 
<https://github.com/microsoft/mu_tiano_platforms/tree/main/Platforms/QemuQ35Pkg>  
We are still working on getting the equivalent for arm64.   Happy to 
discuss more if anyone else is interested.


Thanks

sean



On 1/5/2023 11:58 AM, Gerd Hoffmann wrote:
>    Hi,
>
>> Tried booting grub.efi directly and via shim.efi, on Fedora 37 GA.
>>
>> In both cases I get a pagefault on linux kernel boot (before any other
>> message is printed), which I guess happens because the loader places the
>> linux kernel efi stub in EfiLoaderData memory.
> When compiling ovmf with the same pcd value I get the same behavior
> on x64, so it's consistently buggy across architectures.
>
> take care,
>    Gerd
>
>
>
> 
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98047): https://edk2.groups.io/g/devel/message/98047
Mute This Topic: https://groups.io/mt/93922691/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

  Hi,

> Hopefully sometime in the next few weeks we can prepare a comprehensive set
> of patches and changes needed in edk2 to implement this strict environment. 
> One of the relevant changes to this discussion and patch series is we
> switched the configuration from PCD to hob
> (mu_basecore/DxeMemoryProtectionSettings.h at release/202208 ·
> microsoft/mu_basecore (github.com) <https://github.com/microsoft/mu_basecore/blob/release/202208/MdeModulePkg/Include/Guid/DxeMemoryProtectionSettings.h>).
> This allows our platforms complete control of the config per boot.

Why a HOB?  I guess because dynamic PCDs are available too late in the
boot process?

> Some platforms have compatibility requirements and have implemented
> code so that when a PF is triggered by incompatible software, it is
> recorded and then the system rebooted.  On the next boot the platform
> changes the HOB configuration to be in a more compatible mode (this
> mode could be measured in a PCR and/or other hints to the user/system
> of degraded security).

Where is the configuration stored?

> Anyway, rather than a patchwork of changes going into different platforms
> and components of edk2 I would like to see alignment between x86/arm64 in
> edk2 and a complete set of changes for edk2.

Sure, it totally makes sense to have that as core edk2 feature instead
of adding this to each platform individually.

Looking forward to see the patches.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98079): https://edk2.groups.io/g/devel/message/98079
Mute This Topic: https://groups.io/mt/93922691/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

  Hi,

> > > > You can override PCDs on the build command line, so I suggest you use
> > > > that for building these images as long as it is needed.
> > > >
> > > > E.g,, append this to the build.sh command line
> > > >
> > > > --pcd PcdDxeNxMemoryProtectionPolicy=0xC000000000007FD1
> > > >
> > > > to undo the effects of this patch.

Can this also be flipped at runtime?
Does this pcd work the same way on all architectures?

> I don't think having different versions of the image makes sense, tbh,
> but of course, this is up to the distros.

Fedora has reverted the patch for now, and I don't see how we can enable
that anytime soon given that RHEL-8,9 with loooooong support times ship
broken grub binaries today.

> Compatibility with ancient downstream GRUB builds is not a goal of the
> EDK2 upstream, so as long as distros can tweak the build to their
> needs, I don't see a reason to revert this change upstream.

The versions are not that ancient.  The problem is more that upstream
grub is really slow on integrating patches so every distro does carry
a huge pile of downstream patches.  And they seem to re-introduce the
bug ...

But, yes, just reverting upstream too doesn't look like a good option
either, we need at least a little pressure to get things fixed.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97932): https://edk2.groups.io/g/devel/message/97932
Mute This Topic: https://groups.io/mt/93922691/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 1/4/23 12:11, Gerd Hoffmann wrote:

> The versions are not that ancient.  The problem is more that upstream
> grub is really slow on integrating patches so every distro does carry
> a huge pile of downstream patches.  And they seem to re-introduce the
> bug ...

See also: commit d20b06a3afdf ("OvmfPkg: disable no-exec DXE stack by
default", 2015-09-15). That was more than seven years ago, and it's what

  git blame master -- OvmfPkg/OvmfPkgX64.dsc | grep PcdSetNxForStack

reports today.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98096): https://edk2.groups.io/g/devel/message/98096
Mute This Topic: https://groups.io/mt/93922691/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 1/6/23 10:55, Laszlo Ersek wrote:
> On 1/4/23 12:11, Gerd Hoffmann wrote:
> 
>> The versions are not that ancient.  The problem is more that upstream
>> grub is really slow on integrating patches so every distro does carry
>> a huge pile of downstream patches.  And they seem to re-introduce the
>> bug ...
> 
> See also: commit d20b06a3afdf ("OvmfPkg: disable no-exec DXE stack by
> default", 2015-09-15). That was more than seven years ago, and it's what
> 
>   git blame master -- OvmfPkg/OvmfPkgX64.dsc | grep PcdSetNxForStack
> 
> reports today.

On a more constructive note.

By the book, this guest OS-level quirk should be exposed from the
firmware up to libosinfo / osinfo-db.

Starting with a dynamic PCD or HOB exposed via fw_cfg (with the fw_cfg
filename conforming to "docs/specs/fw_cfg.rst" in QEMU), handled by
libvirtd and other management applications (domain XML and other config
formats, matching code, etc), and ultimately recorded in a "w^x"
capability entry in the libosinfo schema and the osinfo database.

All other guest OS compatibility settings are tracked in osinfo
nowadays, security related or not, and they are so important that recent
virt-install even refuses (by default) to install a domain if it doesn't
recognize (and the user doesn't say) what the guest OS is.

https://github.com/virt-manager/virt-manager/commit/26ecf8a5e3e4721488159605afd10e39f68e6382

Those settings control various CPU vulnerability mitigations even, IIUC,
so it's almost certainly the right place to implement this new quirk too.

Let us not sweep it under the carpet, and heap on more technical debt.
Storing grub hashes in the firmware is similar to Windows video drivers
tailoring themselves to the game the user happens to start. "Tailoring"
is fine, but not from the bottom up.

Here's what we could do for, and in, ArmVirtQemu *upstream*:

- file the proper RFEs for the above-described components,

- get their maintainers publicly commit to implementing them (that will
take a while),

- once each RFE has been committed to, and we think the whole picture is
covered, downgrade the "w^x" default in ArmVirtQemu as follows:

- list the ticket links near the code that does the downgrade

- *gate* the downgrade on the platform RTC reading a date that's before
a specific, open-coded constant. We'll forget about the downgrade, but
the RTC won't forget about time passing. This will make us revise the
concession in time (unlike how we've completely forgotten about
PcdSetNxForStack). Once all the RFEs have been fixed upstream, and
widely shipped in products, we can remove the downgrade.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98098): https://edk2.groups.io/g/devel/message/98098
Mute This Topic: https://groups.io/mt/93922691/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Wed, 4 Jan 2023 at 12:11, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > > > You can override PCDs on the build command line, so I suggest you use
> > > > > that for building these images as long as it is needed.
> > > > >
> > > > > E.g,, append this to the build.sh command line
> > > > >
> > > > > --pcd PcdDxeNxMemoryProtectionPolicy=0xC000000000007FD1
> > > > >
> > > > > to undo the effects of this patch.
>
> Can this also be flipped at runtime?

Currently, it is fixed or patchable, which means that you can override
it at build time only. I don't think making this a dynamic PCD would
be difficult, and on QEMU, we can set the value early enough if we key
it off fw_cfg or something like that.

But that implies that you need a 'permissive' mode to invoke QEMU,
which ends up being always enabled, most likely, so I'm not sure this
is an improvement.

> Does this pcd work the same way on all architectures?
>

In principle, yes. However, I cannot vouch for the X86 code not doing
dodgy things with data regions, so whether the same *value* works
reliably across all architectures is a separate matter.

> > I don't think having different versions of the image makes sense, tbh,
> > but of course, this is up to the distros.
>
> Fedora has reverted the patch for now, and I don't see how we can enable
> that anytime soon given that RHEL-8,9 with loooooong support times ship
> broken grub binaries today.
>

Yeah. This is really disappointing.

> > Compatibility with ancient downstream GRUB builds is not a goal of the
> > EDK2 upstream, so as long as distros can tweak the build to their
> > needs, I don't see a reason to revert this change upstream.
>
> The versions are not that ancient.  The problem is more that upstream
> grub is really slow on integrating patches so every distro does carry
> a huge pile of downstream patches.  And they seem to re-introduce the
> bug ...
>
> But, yes, just reverting upstream too doesn't look like a good option
> either, we need at least a little pressure to get things fixed.
>

Indeed.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97934): https://edk2.groups.io/g/devel/message/97934
Mute This Topic: https://groups.io/mt/93922691/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Wed, Jan 04, 2023 at 01:04:41PM +0100, Ard Biesheuvel wrote:
> On Wed, 4 Jan 2023 at 12:11, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> >   Hi,
> >
> > > > > > --pcd PcdDxeNxMemoryProtectionPolicy=0xC000000000007FD1
> >
> > Can this also be flipped at runtime?
> 
> Currently, it is fixed or patchable, which means that you can override
> it at build time only. I don't think making this a dynamic PCD would
> be difficult, and on QEMU, we can set the value early enough if we key
> it off fw_cfg or something like that.
> 
> But that implies that you need a 'permissive' mode to invoke QEMU,
> which ends up being always enabled, most likely, so I'm not sure this
> is an improvement.

It works both ways.  Being able to enable nx protection at runtime on
builds which have it disabled by default would be quite useful.  Write
test cases.  Write reproducer instructions which don't include building
edk2 yourself.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97936): https://edk2.groups.io/g/devel/message/97936
Mute This Topic: https://groups.io/mt/93922691/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 2022-09-26 01:24, Ard Biesheuvel wrote:
> When the memory protections were implemented and enabled on ArmVirtQemu
> 5+ years ago, we had to work around the fact that GRUB at the time
> expected EFI_LOADER_DATA to be executable, as that is the memory type it
> allocates when loading its modules.
> 
> This has been fixed in GRUB in August 2017, so by now, we should be able
> to tighten this, and remove execute permissions from EFI_LOADER_DATA
> allocations.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>



> ---
>   ArmVirtPkg/ArmVirt.dsc.inc | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index 34575585adbb..462073517a22 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -368,7 +368,7 @@ [PcdsFixedAtBuild.common]
>     # reserved ones, with the exception of LoaderData regions, of which OS loaders
> 
>     # (i.e., GRUB) may assume that its contents are executable.
> 

Should the comment be updated too ("old versions of GRUB")?

Regardless:
Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>

/
     Leif

>     #
> 
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD1
> 
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD5
> 
>   
> 
>   [Components.common]
> 
>     #
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#94364): https://edk2.groups.io/g/devel/message/94364
Mute This Topic: https://groups.io/mt/93922691/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-