Instead of relying on a questionable heuristic that avoids calling into
the SetMemoryAttributes () DXE service when the old memory type and the
new one are subjected to the same NX memory protection policy, make this
call unconditionally. This avoids corner cases where memory region
attributes are out of sync with the policy, either due to the fact that
we are in the middle of ramping up the protections, or due to explicit
invocations of SetMemoryAttributes() by drivers.
This requires the architecture page table code to be able to deal with
this, in particular, it needs to be robust against potential recursion
due to NX policies being applied to newly allocated page tables.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 29 --------------------
1 file changed, 29 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index 36987843f142..503feb72b5d0 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -1263,9 +1263,7 @@ ApplyMemoryProtectionPolicy (
IN UINT64 Length
)
{
- UINT64 OldAttributes;
UINT64 NewAttributes;
- EFI_STATUS Status;
//
// The policy configured in PcdDxeNxMemoryProtectionPolicy
@@ -1320,32 +1318,5 @@ ApplyMemoryProtectionPolicy (
//
NewAttributes = GetPermissionAttributeForMemoryType (NewType);
- if (OldType != EfiMaxMemoryType) {
- OldAttributes = GetPermissionAttributeForMemoryType (OldType);
- if (!mAfterDxeNxMemoryProtectionInit &&
- (OldAttributes == NewAttributes)) {
- return EFI_SUCCESS;
- }
-
- //
- // If available, use the EFI memory attribute protocol to obtain
- // the current attributes of the region. If the entire region is
- // covered and the attributes match, we don't have to do anything.
- //
- if (mMemoryAttribute != NULL) {
- Status = mMemoryAttribute->GetMemoryAttributes (mMemoryAttribute,
- Memory,
- Length,
- &OldAttributes
- );
- if (!EFI_ERROR (Status) && (OldAttributes == NewAttributes)) {
- return EFI_SUCCESS;
- }
- }
- } else if (NewAttributes == 0) {
- // newly added region of a type that does not require protection
- return EFI_SUCCESS;
- }
-
return gCpu->SetMemoryAttributes (gCpu, Memory, Length, NewAttributes);
}
--
2.39.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99804): https://edk2.groups.io/g/devel/message/99804
Mute This Topic: https://groups.io/mt/96835917/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-On Wed, 8 Feb 2023 at 18:58, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> Instead of relying on a questionable heuristic that avoids calling into
> the SetMemoryAttributes () DXE service when the old memory type and the
> new one are subjected to the same NX memory protection policy, make this
> call unconditionally. This avoids corner cases where memory region
> attributes are out of sync with the policy, either due to the fact that
> we are in the middle of ramping up the protections, or due to explicit
> invocations of SetMemoryAttributes() by drivers.
>
> This requires the architecture page table code to be able to deal with
> this, in particular, it needs to be robust against potential recursion
> due to NX policies being applied to newly allocated page tables.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 29 --------------------
> 1 file changed, 29 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> index 36987843f142..503feb72b5d0 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> @@ -1263,9 +1263,7 @@ ApplyMemoryProtectionPolicy (
> IN UINT64 Length
> )
> {
> - UINT64 OldAttributes;
> UINT64 NewAttributes;
> - EFI_STATUS Status;
>
> //
> // The policy configured in PcdDxeNxMemoryProtectionPolicy
> @@ -1320,32 +1318,5 @@ ApplyMemoryProtectionPolicy (
> //
> NewAttributes = GetPermissionAttributeForMemoryType (NewType);
>
> - if (OldType != EfiMaxMemoryType) {
> - OldAttributes = GetPermissionAttributeForMemoryType (OldType);
> - if (!mAfterDxeNxMemoryProtectionInit &&
> - (OldAttributes == NewAttributes)) {
> - return EFI_SUCCESS;
> - }
> -
This removes some code that does not actually exist - apologies.
It comes down to just removing the conditional checks here, though,
and perform the tail call below unconditionally.
> - //
> - // If available, use the EFI memory attribute protocol to obtain
> - // the current attributes of the region. If the entire region is
> - // covered and the attributes match, we don't have to do anything.
> - //
> - if (mMemoryAttribute != NULL) {
> - Status = mMemoryAttribute->GetMemoryAttributes (mMemoryAttribute,
> - Memory,
> - Length,
> - &OldAttributes
> - );
> - if (!EFI_ERROR (Status) && (OldAttributes == NewAttributes)) {
> - return EFI_SUCCESS;
> - }
> - }
> - } else if (NewAttributes == 0) {
> - // newly added region of a type that does not require protection
> - return EFI_SUCCESS;
> - }
> -
> return gCpu->SetMemoryAttributes (gCpu, Memory, Length, NewAttributes);
> }
> --
> 2.39.1
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99809): https://edk2.groups.io/g/devel/message/99809
Mute This Topic: https://groups.io/mt/96835917/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
I ran some tests and did some quick napkin math. Based on the time it
takes to perform the SetMemoryAttributes() routine on QEMU, as long as
<79% of the calls to ApplyMemoryProtectionPolicy() actually require a
subsequent call to SetMemoryAttributes(), it is more efficient to walk
the page/translation table to check if the attributes actually need to
be updated. Even on a platform with varied NX policy settings, the
number of times the attributes need to be updated is less than 0.0005%
of all calls to ApplyMemoryProtectionPolicy() (likely due to most
allocations being BootServicesData).
Once the ARM and X64 implementations of the Memory Attribute Protocol
are in, would you be open to updating this block to utilize the protocol
to check the attributes of the region being updated?
On 2/8/2023 10:25 AM, Ard Biesheuvel wrote:
> On Wed, 8 Feb 2023 at 18:58, Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> Instead of relying on a questionable heuristic that avoids calling into
>> the SetMemoryAttributes () DXE service when the old memory type and the
>> new one are subjected to the same NX memory protection policy, make this
>> call unconditionally. This avoids corner cases where memory region
>> attributes are out of sync with the policy, either due to the fact that
>> we are in the middle of ramping up the protections, or due to explicit
>> invocations of SetMemoryAttributes() by drivers.
>>
>> This requires the architecture page table code to be able to deal with
>> this, in particular, it needs to be robust against potential recursion
>> due to NX policies being applied to newly allocated page tables.
>>
>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>> ---
>> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 29 --------------------
>> 1 file changed, 29 deletions(-)
>>
>> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>> index 36987843f142..503feb72b5d0 100644
>> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>> @@ -1263,9 +1263,7 @@ ApplyMemoryProtectionPolicy (
>> IN UINT64 Length
>> )
>> {
>> - UINT64 OldAttributes;
>> UINT64 NewAttributes;
>> - EFI_STATUS Status;
>>
>> //
>> // The policy configured in PcdDxeNxMemoryProtectionPolicy
>> @@ -1320,32 +1318,5 @@ ApplyMemoryProtectionPolicy (
>> //
>> NewAttributes = GetPermissionAttributeForMemoryType (NewType);
>>
>> - if (OldType != EfiMaxMemoryType) {
>> - OldAttributes = GetPermissionAttributeForMemoryType (OldType);
>> - if (!mAfterDxeNxMemoryProtectionInit &&
>> - (OldAttributes == NewAttributes)) {
>> - return EFI_SUCCESS;
>> - }
>> -
>
> This removes some code that does not actually exist - apologies.
>
> It comes down to just removing the conditional checks here, though,
> and perform the tail call below unconditionally.
>
>> - //
>> - // If available, use the EFI memory attribute protocol to obtain
>> - // the current attributes of the region. If the entire region is
>> - // covered and the attributes match, we don't have to do anything.
>> - //
>> - if (mMemoryAttribute != NULL) {
>> - Status = mMemoryAttribute->GetMemoryAttributes (mMemoryAttribute,
>> - Memory,
>> - Length,
>> - &OldAttributes
>> - );
>> - if (!EFI_ERROR (Status) && (OldAttributes == NewAttributes)) {
>> - return EFI_SUCCESS;
>> - }
>> - }
>> - } else if (NewAttributes == 0) {
>> - // newly added region of a type that does not require protection
>> - return EFI_SUCCESS;
>> - }
>> -
>> return gCpu->SetMemoryAttributes (gCpu, Memory, Length, NewAttributes);
>> }
>> --
>> 2.39.1
>>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99814): https://edk2.groups.io/g/devel/message/99814
Mute This Topic: https://groups.io/mt/96835917/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On Wed, 8 Feb 2023 at 20:12, Taylor Beebe <t@taylorbeebe.com> wrote:
>
> I ran some tests and did some quick napkin math. Based on the time it
> takes to perform the SetMemoryAttributes() routine on QEMU, as long as
> <79% of the calls to ApplyMemoryProtectionPolicy() actually require a
> subsequent call to SetMemoryAttributes(), it is more efficient to walk
> the page/translation table to check if the attributes actually need to
> be updated. Even on a platform with varied NX policy settings, the
> number of times the attributes need to be updated is less than 0.0005%
> of all calls to ApplyMemoryProtectionPolicy() (likely due to most
> allocations being BootServicesData).
>
> Once the ARM and X64 implementations of the Memory Attribute Protocol
> are in, would you be open to updating this block to utilize the protocol
> to check the attributes of the region being updated?
>
Yes, that was what I had in my initial prototype.
However, I'm not sure how walking the page tables to retrieve all
existing attributes is fundamentally different from walking the page
tables to set them, given that everything is cached and we are running
uniprocessor at this point.
> On 2/8/2023 10:25 AM, Ard Biesheuvel wrote:
> > On Wed, 8 Feb 2023 at 18:58, Ard Biesheuvel <ardb@kernel.org> wrote:
> >>
> >> Instead of relying on a questionable heuristic that avoids calling into
> >> the SetMemoryAttributes () DXE service when the old memory type and the
> >> new one are subjected to the same NX memory protection policy, make this
> >> call unconditionally. This avoids corner cases where memory region
> >> attributes are out of sync with the policy, either due to the fact that
> >> we are in the middle of ramping up the protections, or due to explicit
> >> invocations of SetMemoryAttributes() by drivers.
> >>
> >> This requires the architecture page table code to be able to deal with
> >> this, in particular, it needs to be robust against potential recursion
> >> due to NX policies being applied to newly allocated page tables.
> >>
> >> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >> ---
> >> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 29 --------------------
> >> 1 file changed, 29 deletions(-)
> >>
> >> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> >> index 36987843f142..503feb72b5d0 100644
> >> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> >> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> >> @@ -1263,9 +1263,7 @@ ApplyMemoryProtectionPolicy (
> >> IN UINT64 Length
> >> )
> >> {
> >> - UINT64 OldAttributes;
> >> UINT64 NewAttributes;
> >> - EFI_STATUS Status;
> >>
> >> //
> >> // The policy configured in PcdDxeNxMemoryProtectionPolicy
> >> @@ -1320,32 +1318,5 @@ ApplyMemoryProtectionPolicy (
> >> //
> >> NewAttributes = GetPermissionAttributeForMemoryType (NewType);
> >>
> >> - if (OldType != EfiMaxMemoryType) {
> >> - OldAttributes = GetPermissionAttributeForMemoryType (OldType);
> >> - if (!mAfterDxeNxMemoryProtectionInit &&
> >> - (OldAttributes == NewAttributes)) {
> >> - return EFI_SUCCESS;
> >> - }
> >> -
> >
> > This removes some code that does not actually exist - apologies.
> >
> > It comes down to just removing the conditional checks here, though,
> > and perform the tail call below unconditionally.
> >
> >> - //
> >> - // If available, use the EFI memory attribute protocol to obtain
> >> - // the current attributes of the region. If the entire region is
> >> - // covered and the attributes match, we don't have to do anything.
> >> - //
> >> - if (mMemoryAttribute != NULL) {
> >> - Status = mMemoryAttribute->GetMemoryAttributes (mMemoryAttribute,
> >> - Memory,
> >> - Length,
> >> - &OldAttributes
> >> - );
> >> - if (!EFI_ERROR (Status) && (OldAttributes == NewAttributes)) {
> >> - return EFI_SUCCESS;
> >> - }
> >> - }
> >> - } else if (NewAttributes == 0) {
> >> - // newly added region of a type that does not require protection
> >> - return EFI_SUCCESS;
> >> - }
> >> -
> >> return gCpu->SetMemoryAttributes (gCpu, Memory, Length, NewAttributes);
> >> }
> >> --
> >> 2.39.1
> >>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99817): https://edk2.groups.io/g/devel/message/99817
Mute This Topic: https://groups.io/mt/96835917/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 2/8/2023 2:08 PM, Ard Biesheuvel wrote:
> On Wed, 8 Feb 2023 at 20:12, Taylor Beebe <t@taylorbeebe.com> wrote:
>>
>> I ran some tests and did some quick napkin math. Based on the time it
>> takes to perform the SetMemoryAttributes() routine on QEMU, as long as
>> <79% of the calls to ApplyMemoryProtectionPolicy() actually require a
>> subsequent call to SetMemoryAttributes(), it is more efficient to walk
>> the page/translation table to check if the attributes actually need to
>> be updated. Even on a platform with varied NX policy settings, the
>> number of times the attributes need to be updated is less than 0.0005%
>> of all calls to ApplyMemoryProtectionPolicy() (likely due to most
>> allocations being BootServicesData).
>>
>> Once the ARM and X64 implementations of the Memory Attribute Protocol
>> are in, would you be open to updating this block to utilize the protocol
>> to check the attributes of the region being updated?
>>
>
> Yes, that was what I had in my initial prototype.
>
> However, I'm not sure how walking the page tables to retrieve all
> existing attributes is fundamentally different from walking the page
> tables to set them, given that everything is cached and we are running
> uniprocessor at this point.
>
I was also skeptical, but running the experiment revealed that checking
the page table is the clear winner. My guess is that branch prediction
expects the table walk to reveal that the SetAttributes() call is
unnecessary, and the cost of doing an instruction pipeline flush due to
improper branch prediction is similar to the cost of doing a TLB
flush/invalidation after the call to SetAttributes() making the upside
outweigh the downside in almost all cases.
>
>> On 2/8/2023 10:25 AM, Ard Biesheuvel wrote:
>>> On Wed, 8 Feb 2023 at 18:58, Ard Biesheuvel <ardb@kernel.org> wrote:
>>>>
>>>> Instead of relying on a questionable heuristic that avoids calling into
>>>> the SetMemoryAttributes () DXE service when the old memory type and the
>>>> new one are subjected to the same NX memory protection policy, make this
>>>> call unconditionally. This avoids corner cases where memory region
>>>> attributes are out of sync with the policy, either due to the fact that
>>>> we are in the middle of ramping up the protections, or due to explicit
>>>> invocations of SetMemoryAttributes() by drivers.
>>>>
>>>> This requires the architecture page table code to be able to deal with
>>>> this, in particular, it needs to be robust against potential recursion
>>>> due to NX policies being applied to newly allocated page tables.
>>>>
>>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>>> ---
>>>> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 29 --------------------
>>>> 1 file changed, 29 deletions(-)
>>>>
>>>> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>>>> index 36987843f142..503feb72b5d0 100644
>>>> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>>>> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>>>> @@ -1263,9 +1263,7 @@ ApplyMemoryProtectionPolicy (
>>>> IN UINT64 Length
>>>> )
>>>> {
>>>> - UINT64 OldAttributes;
>>>> UINT64 NewAttributes;
>>>> - EFI_STATUS Status;
>>>>
>>>> //
>>>> // The policy configured in PcdDxeNxMemoryProtectionPolicy
>>>> @@ -1320,32 +1318,5 @@ ApplyMemoryProtectionPolicy (
>>>> //
>>>> NewAttributes = GetPermissionAttributeForMemoryType (NewType);
>>>>
>>>> - if (OldType != EfiMaxMemoryType) {
>>>> - OldAttributes = GetPermissionAttributeForMemoryType (OldType);
>>>> - if (!mAfterDxeNxMemoryProtectionInit &&
>>>> - (OldAttributes == NewAttributes)) {
>>>> - return EFI_SUCCESS;
>>>> - }
>>>> -
>>>
>>> This removes some code that does not actually exist - apologies.
>>>
>>> It comes down to just removing the conditional checks here, though,
>>> and perform the tail call below unconditionally.
>>>
>>>> - //
>>>> - // If available, use the EFI memory attribute protocol to obtain
>>>> - // the current attributes of the region. If the entire region is
>>>> - // covered and the attributes match, we don't have to do anything.
>>>> - //
>>>> - if (mMemoryAttribute != NULL) {
>>>> - Status = mMemoryAttribute->GetMemoryAttributes (mMemoryAttribute,
>>>> - Memory,
>>>> - Length,
>>>> - &OldAttributes
>>>> - );
>>>> - if (!EFI_ERROR (Status) && (OldAttributes == NewAttributes)) {
>>>> - return EFI_SUCCESS;
>>>> - }
>>>> - }
>>>> - } else if (NewAttributes == 0) {
>>>> - // newly added region of a type that does not require protection
>>>> - return EFI_SUCCESS;
>>>> - }
>>>> -
>>>> return gCpu->SetMemoryAttributes (gCpu, Memory, Length, NewAttributes);
>>>> }
>>>> --
>>>> 2.39.1
>>>>
--
Taylor Beebe
Software Engineer @ Microsoft
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99819): https://edk2.groups.io/g/devel/message/99819
Mute This Topic: https://groups.io/mt/96835917/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
> On 8. Feb 2023, at 19:49, Ard Biesheuvel <ardb@kernel.org> wrote: > > This is all copy-pasted from MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c :( > > The ordering here is a bit tricky. As soon as the CPU arch protocol is > exposed, every allocation will be remapped individually, resulting in > page table splits and therefore recursion. So the issue is the order of event handlers or allocations within the event dispatcher? :( Oh lord... Can we maybe clarify the comment with something like "While DxeCore/InitializeDxeNxMemoryProtectionPolicy() would in theory perform this task, allocations between the protocol installation and the invocation of its event handler may trigger the issue."? -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#99812): https://edk2.groups.io/g/devel/message/99812 Mute This Topic: https://groups.io/mt/96835917/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.