[edk2-devel] [PATCH v4 11/14] UefiCpuPkg: Use Attribute From SMM MemoryAttributesTable if Nonzero

Taylor Beebe posted 14 patches 1 year ago
[edk2-devel] [PATCH v4 11/14] UefiCpuPkg: Use Attribute From SMM MemoryAttributesTable if Nonzero
Posted by Taylor Beebe 1 year ago
The function EnforceMemoryMapAttribute() in the SMM MAT logic will
ensure that the CODE and DATA memory types have the desired attributes.
The consumer of the SMM MAT should only override the Attributes field
in the MAT if it is nonzero. This also allows the UEFI and SMM MAT
logic to use ImagePropertiesRecordLib instead of carrying two copies
of the image properties record manipulation logic.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Taylor Beebe <taylor.d.beebe@gmail.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
index 6f498666157e..d302a9b0cbcf 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
@@ -1062,14 +1062,17 @@ SetMemMapAttributes (
   MemoryMap = MemoryMapStart;
   for (Index = 0; Index < MemoryMapEntryCount; Index++) {
     DEBUG ((DEBUG_VERBOSE, "SetAttribute: Memory Entry - 0x%lx, 0x%x\n", MemoryMap->PhysicalStart, MemoryMap->NumberOfPages));
-    if (MemoryMap->Type == EfiRuntimeServicesCode) {
-      MemoryAttribute = EFI_MEMORY_RO;
-    } else {
-      ASSERT ((MemoryMap->Type == EfiRuntimeServicesData) || (MemoryMap->Type == EfiConventionalMemory));
-      //
-      // Set other type memory as NX.
-      //
-      MemoryAttribute = EFI_MEMORY_XP;
+    MemoryAttribute = MemoryMap->Attribute & EFI_MEMORY_ACCESS_MASK;
+    if (MemoryAttribute == 0) {
+      if (MemoryMap->Type == EfiRuntimeServicesCode) {
+        MemoryAttribute = EFI_MEMORY_RO;
+      } else {
+        ASSERT ((MemoryMap->Type == EfiRuntimeServicesData) || (MemoryMap->Type == EfiConventionalMemory));
+        //
+        // Set other type memory as NX.
+        //
+        MemoryAttribute = EFI_MEMORY_XP;
+      }
     }
 
     //
-- 
2.42.0.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110659): https://edk2.groups.io/g/devel/message/110659
Mute This Topic: https://groups.io/mt/102368851/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v4 11/14] UefiCpuPkg: Use Attribute From SMM MemoryAttributesTable if Nonzero
Posted by Laszlo Ersek 1 year ago
On 11/3/23 18:17, Taylor Beebe wrote:
> The function EnforceMemoryMapAttribute() in the SMM MAT logic will
> ensure that the CODE and DATA memory types have the desired attributes.

EnforceMemoryMapAttribute() leaves those descriptors alone where
Attribute is already nonzero ("PE image") [1].

For all other descriptors (i.e., where Attribute is zero), it:

- either sets Attribute to EFI_MEMORY_RO [2],

- or sets the EFI_MEMORY_XP *bit* in the Attribute [3] -- which is
identical to setting Attribute to EFI_MEMORY_XP altogether, given that
Attribute is zero to begin with. (So this |= operator looks like a
thinko in the function! But that's a separate topic.)

> The consumer of the SMM MAT

So this seems to imply that the SMM MAT is produced based on
EnforceMemoryMapAttribute(), and then SetMemMapAttributes(), modified in
this patch, is what consumes the SMM MAT. Is that right?

In other words, SetMemMapAttributes() relies on
EnforceMemoryMapAttribute(); is that your point?

> should only override the Attributes field
> in the MAT if it is nonzero.

I don't understand -- do you mean "override the Attributes field if it
is *zero*"? (Because, if it is nonzero, then it has been pre-populated
by EnforceMemoryMapAttribute(), and then we should *use* it, as the
subject line of the patch says.)

Further question: given that EnforceMemoryMapAttribute() exits with
*all* descriptors having a nonzero Attribute field, how is it possible
for SetMemMapAttributes() to see any descriptor with zero Attribute field?

I see two possibilities:

- something introduces a new descriptor between
EnforceMemoryMapAttribute() and SetMemMapAttributes()

- and/or, SetMemMapAttributes() doesn't actually check the entire
Attribute field for nullity, but only the EFI_MEMORY_ACCESS_MASK
bitfield of it. Cases [2] and [3] above ensure the
EFI_MEMORY_ACCESS_MASK bitmask is nonzero, but case [1] ("PE image")
allows for an Attribute field in the descriptor that is nonzero, but
whose EFI_MEMORY_ACCESS_MASK bitfield is zero.

> This also allows the UEFI and SMM MAT
> logic to use ImagePropertiesRecordLib instead of carrying two copies
> of the image properties record manipulation logic.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Taylor Beebe <taylor.d.beebe@gmail.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> index 6f498666157e..d302a9b0cbcf 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> @@ -1062,14 +1062,17 @@ SetMemMapAttributes (
>    MemoryMap = MemoryMapStart;
>    for (Index = 0; Index < MemoryMapEntryCount; Index++) {
>      DEBUG ((DEBUG_VERBOSE, "SetAttribute: Memory Entry - 0x%lx, 0x%x\n", MemoryMap->PhysicalStart, MemoryMap->NumberOfPages));
> -    if (MemoryMap->Type == EfiRuntimeServicesCode) {
> -      MemoryAttribute = EFI_MEMORY_RO;
> -    } else {
> -      ASSERT ((MemoryMap->Type == EfiRuntimeServicesData) || (MemoryMap->Type == EfiConventionalMemory));
> -      //
> -      // Set other type memory as NX.
> -      //
> -      MemoryAttribute = EFI_MEMORY_XP;
> +    MemoryAttribute = MemoryMap->Attribute & EFI_MEMORY_ACCESS_MASK;

OK, so this line is what makes SetMemMapAttributes() rely on
EnforceMemoryMapAttribute().

What ensures the proper data flow, from EnforceMemoryMapAttribute() to
SetMemMapAttributes()?

> +    if (MemoryAttribute == 0) {

So this seems to identify case [1] (... or an entirely newly added
descriptor)...

> +      if (MemoryMap->Type == EfiRuntimeServicesCode) {
> +        MemoryAttribute = EFI_MEMORY_RO;
> +      } else {
> +        ASSERT ((MemoryMap->Type == EfiRuntimeServicesData) || (MemoryMap->Type == EfiConventionalMemory));
> +        //
> +        // Set other type memory as NX.
> +        //
> +        MemoryAttribute = EFI_MEMORY_XP;
> +      }
>      }
>  
>      //

Makes sense to me, but there are many open questions about the data
flow; I'd like the commit message to ELI5.

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110929): https://edk2.groups.io/g/devel/message/110929
Mute This Topic: https://groups.io/mt/102368851/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v4 11/14] UefiCpuPkg: Use Attribute From SMM MemoryAttributesTable if Nonzero
Posted by Taylor Beebe 11 months, 4 weeks ago
I missed this message before heading out on vacation, sorry :)

Let me know if you have more questions. I've also split the MAT fixes into

separate patches for the v5 version and will wait to hear back from you

before sending to ensure the updated notes for this patch

answer all your questions.

On 11/8/2023 1:57 PM, Laszlo Ersek wrote:
> On 11/3/23 18:17, Taylor Beebe wrote:
>> The function EnforceMemoryMapAttribute() in the SMM MAT logic will
>> ensure that the CODE and DATA memory types have the desired attributes.
> EnforceMemoryMapAttribute() leaves those descriptors alone where
> Attribute is already nonzero ("PE image") [1].
>
> For all other descriptors (i.e., where Attribute is zero), it:
>
> - either sets Attribute to EFI_MEMORY_RO [2],
>
> - or sets the EFI_MEMORY_XP *bit* in the Attribute [3] -- which is
> identical to setting Attribute to EFI_MEMORY_XP altogether, given that
> Attribute is zero to begin with. (So this |= operator looks like a
> thinko in the function! But that's a separate topic.)
>
>> The consumer of the SMM MAT
> So this seems to imply that the SMM MAT is produced based on
> EnforceMemoryMapAttribute(), and then SetMemMapAttributes(), modified in
> this patch, is what consumes the SMM MAT. Is that right?

This is correct. Note that the MAT is installed as a config table with the

gEdkiiPiSmmMemoryAttributesTableGuid but I could not find any info

in the PI spec on this table, just in the header file. Not sure if 
that's usual,

I just assumed that EdkiiPi in the guid name meant it was documented

in the spec.

> In other words, SetMemMapAttributes() relies on
> EnforceMemoryMapAttribute(); is that your point?

EnforceMemoryMapAttribute() is a final filter function run after 
creating the

MAT from the EFI memory map and loaded image list. More info below.

>> should only override the Attributes field
>> in the MAT if it is nonzero.
> I don't understand -- do you mean "override the Attributes field if it
> is *zero*"? (Because, if it is nonzero, then it has been pre-populated
> by EnforceMemoryMapAttribute(), and then we should *use* it, as the
> subject line of the patch says.)

PiSmmCore fetches the EFI memory map and calls SplitTable() to split each

loaded image section into its own descriptor with EFI_MEMORY_XP marking

data sections and EFI_MEMORY_RO marking code sections.


[4] The SMM MAT logic is almost identical to the DXE MAT logic but goes 
a step

further and also updates the memory map descriptors which describe

image code and data sections to be of type EfiRuntimeServicesCodeand

EfiRuntimeServicesData respectively.The consolidated MAT logic more

closely follows the DXE MAT logic which identifies image code sections 
by the

presence of the attribute EFI_MEMORY_RO in the descriptor and image

data sections by the presence of the attribute EFI_MEMORY_XP.

> Further question: given that EnforceMemoryMapAttribute() exits with
> *all* descriptors having a nonzero Attribute field, how is it possible
> for SetMemMapAttributes() to see any descriptor with zero Attribute field?

It is not possible, but SetMemMapAttributes() would apply paging

attributes based on the memory type regardless of what was already

in the Attribute fieldof the SMM MAT. This worked because of [4],

but because this patch series is consolidating the DXE and SMM MAT

logic into a library this no longer works.

> I see two possibilities:
>
> - something introduces a new descriptor between
> EnforceMemoryMapAttribute() and SetMemMapAttributes()
>
> - and/or, SetMemMapAttributes() doesn't actually check the entire
> Attribute field for nullity, but only the EFI_MEMORY_ACCESS_MASK
> bitfield of it. Cases [2] and [3] above ensure the
> EFI_MEMORY_ACCESS_MASK bitmask is nonzero, but case [1] ("PE image")
> allows for an Attribute field in the descriptor that is nonzero, but
> whose EFI_MEMORY_ACCESS_MASK bitfield is zero.

There won't be a descriptor with nonzero attributes after 
EnforceMemoryMapAttribute().

However, SetMemMapAttributes() should use the attributes present in the 
SMM MAT

to apply paging protections. When I wrote this patch comment, I assumed 
the DXE and

SMM MAT logic was more widely understood so describing its flow was not 
necessary. I'm

realizing that the MAT logic is basically a black box nowadays :D

>> This also allows the UEFI and SMM MAT
>> logic to use ImagePropertiesRecordLib instead of carrying two copies
>> of the image properties record manipulation logic.
>>
>> Cc: Eric Dong<eric.dong@intel.com>
>> Cc: Ray Ni<ray.ni@intel.com>
>> Cc: Rahul Kumar<rahul1.kumar@intel.com>
>> Cc: Gerd Hoffmann<kraxel@redhat.com>
>> Signed-off-by: Taylor Beebe<taylor.d.beebe@gmail.com>
>> ---
>>   UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 19 +++++++++++--------
>>   1 file changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>> index 6f498666157e..d302a9b0cbcf 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>> @@ -1062,14 +1062,17 @@ SetMemMapAttributes (
>>     MemoryMap = MemoryMapStart;
>>     for (Index = 0; Index < MemoryMapEntryCount; Index++) {
>>       DEBUG ((DEBUG_VERBOSE, "SetAttribute: Memory Entry - 0x%lx, 0x%x\n", MemoryMap->PhysicalStart, MemoryMap->NumberOfPages));
>> -    if (MemoryMap->Type == EfiRuntimeServicesCode) {
>> -      MemoryAttribute = EFI_MEMORY_RO;
>> -    } else {
>> -      ASSERT ((MemoryMap->Type == EfiRuntimeServicesData) || (MemoryMap->Type == EfiConventionalMemory));
>> -      //
>> -      // Set other type memory as NX.
>> -      //
>> -      MemoryAttribute = EFI_MEMORY_XP;
>> +    MemoryAttribute = MemoryMap->Attribute & EFI_MEMORY_ACCESS_MASK;
> OK, so this line is what makes SetMemMapAttributes() rely on
> EnforceMemoryMapAttribute().
>
> What ensures the proper data flow, from EnforceMemoryMapAttribute() to
> SetMemMapAttributes()?

There's not really a required data flow, instead it's expected that if 
the creator of the

SMM MAT populated the Attributes field then those access attributes 
should be used

when the consumer sets paging attributes.

>> +    if (MemoryAttribute == 0) {
> So this seems to identify case [1] (... or an entirely newly added
> descriptor)...
>
>> +      if (MemoryMap->Type == EfiRuntimeServicesCode) {
>> +        MemoryAttribute = EFI_MEMORY_RO;
>> +      } else {
>> +        ASSERT ((MemoryMap->Type == EfiRuntimeServicesData) || (MemoryMap->Type == EfiConventionalMemory));
>> +        //
>> +        // Set other type memory as NX.
>> +        //
>> +        MemoryAttribute = EFI_MEMORY_XP;
>> +      }
>>       }
>>   
>>       //
> Makes sense to me, but there are many open questions about the data
> flow; I'd like the commit message to ELI5.
>
> Thanks
> Laszlo
>


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


Re: [edk2-devel] [PATCH v4 11/14] UefiCpuPkg: Use Attribute From SMM MemoryAttributesTable if Nonzero
Posted by Laszlo Ersek 11 months, 3 weeks ago
On 11/20/23 23:57, Taylor Beebe wrote:
> I missed this message before heading out on vacation, sorry :)
> 
> Let me know if you have more questions. I've also split the MAT fixes into
> 
> separate patches for the v5 version and will wait to hear back from you
> 
> before sending to ensure the updated notes for this patch
> 
> answer all your questions.
> 
> On 11/8/2023 1:57 PM, Laszlo Ersek wrote:
>> On 11/3/23 18:17, Taylor Beebe wrote:
>>> The function EnforceMemoryMapAttribute() in the SMM MAT logic will
>>> ensure that the CODE and DATA memory types have the desired attributes.
>> EnforceMemoryMapAttribute() leaves those descriptors alone where
>> Attribute is already nonzero ("PE image") [1].
>>
>> For all other descriptors (i.e., where Attribute is zero), it:
>>
>> - either sets Attribute to EFI_MEMORY_RO [2],
>>
>> - or sets the EFI_MEMORY_XP *bit* in the Attribute [3] -- which is
>> identical to setting Attribute to EFI_MEMORY_XP altogether, given that
>> Attribute is zero to begin with. (So this |= operator looks like a
>> thinko in the function! But that's a separate topic.)
>>
>>> The consumer of the SMM MAT
>> So this seems to imply that the SMM MAT is produced based on
>> EnforceMemoryMapAttribute(), and then SetMemMapAttributes(), modified in
>> this patch, is what consumes the SMM MAT. Is that right?
> 
> This is correct. Note that the MAT is installed as a config table with the
> 
> gEdkiiPiSmmMemoryAttributesTableGuid but I could not find any info
> 
> in the PI spec on this table, just in the header file. Not sure if
> that's usual,
> 
> I just assumed that EdkiiPi in the guid name meant it was documented
> 
> in the spec.

The Edkii prefix usually means the thing is an edk2 extension. The
subsequent Pi prefix likely only means that the thing extends PI in some
way (and not, for example, UEFI).

> 
>> In other words, SetMemMapAttributes() relies on
>> EnforceMemoryMapAttribute(); is that your point?
> 
> EnforceMemoryMapAttribute() is a final filter function run after
> creating the
> 
> MAT from the EFI memory map and loaded image list. More info below.
> 
>>> should only override the Attributes field
>>> in the MAT if it is nonzero.
>> I don't understand -- do you mean "override the Attributes field if it
>> is *zero*"? (Because, if it is nonzero, then it has been pre-populated
>> by EnforceMemoryMapAttribute(), and then we should *use* it, as the
>> subject line of the patch says.)
> 
> PiSmmCore fetches the EFI memory map and calls SplitTable() to split each
> 
> loaded image section into its own descriptor with EFI_MEMORY_XP marking
> 
> data sections and EFI_MEMORY_RO marking code sections.
> 
> 
> [4] The SMM MAT logic is almost identical to the DXE MAT logic but goes
> a step
> 
> further and also updates the memory map descriptors which describe
> 
> image code and data sections to be of type EfiRuntimeServicesCodeand
> 
> EfiRuntimeServicesData respectively.The consolidated MAT logic more
> 
> closely follows the DXE MAT logic which identifies image code sections
> by the
> 
> presence of the attribute EFI_MEMORY_RO in the descriptor and image
> 
> data sections by the presence of the attribute EFI_MEMORY_XP.
> 
>> Further question: given that EnforceMemoryMapAttribute() exits with
>> *all* descriptors having a nonzero Attribute field, how is it possible
>> for SetMemMapAttributes() to see any descriptor with zero Attribute field?
> 
> It is not possible, but SetMemMapAttributes() would apply paging
> 
> attributes based on the memory type regardless of what was already
> 
> in the Attribute fieldof the SMM MAT. This worked because of [4],
> 
> but because this patch series is consolidating the DXE and SMM MAT
> 
> logic into a library this no longer works.
> 
>> I see two possibilities:
>>
>> - something introduces a new descriptor between
>> EnforceMemoryMapAttribute() and SetMemMapAttributes()
>>
>> - and/or, SetMemMapAttributes() doesn't actually check the entire
>> Attribute field for nullity, but only the EFI_MEMORY_ACCESS_MASK
>> bitfield of it. Cases [2] and [3] above ensure the
>> EFI_MEMORY_ACCESS_MASK bitmask is nonzero, but case [1] ("PE image")
>> allows for an Attribute field in the descriptor that is nonzero, but
>> whose EFI_MEMORY_ACCESS_MASK bitfield is zero.
> 
> There won't be a descriptor with nonzero attributes after
> EnforceMemoryMapAttribute().
> 
> However, SetMemMapAttributes() should use the attributes present in the
> SMM MAT
> 
> to apply paging protections. When I wrote this patch comment, I assumed
> the DXE and
> 
> SMM MAT logic was more widely understood so describing its flow was not
> necessary. I'm
> 
> realizing that the MAT logic is basically a black box nowadays :D

It is, to me.

This rework should really be reviewed by the original MAT authors. In
their absence, I can't offer a deep review. However, that should
certainly not block serious bug fixes.

If you can work these responses into the commit message for v5, I'll be
satisfied; feel free to add

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

then, to the next version of this patch.

Thanks!
Laszlo

> 
>>> This also allows the UEFI and SMM MAT
>>> logic to use ImagePropertiesRecordLib instead of carrying two copies
>>> of the image properties record manipulation logic.
>>>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Ray Ni <ray.ni@intel.com>
>>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>> Signed-off-by: Taylor Beebe <taylor.d.beebe@gmail.com>
>>> ---
>>>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 19 +++++++++++--------
>>>  1 file changed, 11 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>>> index 6f498666157e..d302a9b0cbcf 100644
>>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>>> @@ -1062,14 +1062,17 @@ SetMemMapAttributes (
>>>    MemoryMap = MemoryMapStart;
>>>    for (Index = 0; Index < MemoryMapEntryCount; Index++) {
>>>      DEBUG ((DEBUG_VERBOSE, "SetAttribute: Memory Entry - 0x%lx, 0x%x\n", MemoryMap->PhysicalStart, MemoryMap->NumberOfPages));
>>> -    if (MemoryMap->Type == EfiRuntimeServicesCode) {
>>> -      MemoryAttribute = EFI_MEMORY_RO;
>>> -    } else {
>>> -      ASSERT ((MemoryMap->Type == EfiRuntimeServicesData) || (MemoryMap->Type == EfiConventionalMemory));
>>> -      //
>>> -      // Set other type memory as NX.
>>> -      //
>>> -      MemoryAttribute = EFI_MEMORY_XP;
>>> +    MemoryAttribute = MemoryMap->Attribute & EFI_MEMORY_ACCESS_MASK;
>> OK, so this line is what makes SetMemMapAttributes() rely on
>> EnforceMemoryMapAttribute().
>>
>> What ensures the proper data flow, from EnforceMemoryMapAttribute() to
>> SetMemMapAttributes()?
> 
> There's not really a required data flow, instead it's expected that if
> the creator of the
> 
> SMM MAT populated the Attributes field then those access attributes
> should be used
> 
> when the consumer sets paging attributes.
> 
>>> +    if (MemoryAttribute == 0) {
>> So this seems to identify case [1] (... or an entirely newly added
>> descriptor)...
>>
>>> +      if (MemoryMap->Type == EfiRuntimeServicesCode) {
>>> +        MemoryAttribute = EFI_MEMORY_RO;
>>> +      } else {
>>> +        ASSERT ((MemoryMap->Type == EfiRuntimeServicesData) || (MemoryMap->Type == EfiConventionalMemory));
>>> +        //
>>> +        // Set other type memory as NX.
>>> +        //
>>> +        MemoryAttribute = EFI_MEMORY_XP;
>>> +      }
>>>      }
>>>  
>>>      //
>> Makes sense to me, but there are many open questions about the data
>> flow; I'd like the commit message to ELI5.
>>
>> Thanks
>> Laszlo
>>
> 



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