[edk2-devel] [PATCH v1] MdeModulePkg: Fixup MAT Attributes After Splitting EFI Memory Map

[edk2-devel] [PATCH v1] MdeModulePkg: Fixup MAT Attributes After Splitting EFI Memory Map

[Taylor Beebe]

The Memory Attributes Table is generated by fetching the EFI
memory map and splitting entries which contain loaded
images so DATA and CODE sections have separate descriptors.
The splitting is done via a call to SplitTable() which
marks image DATA sections with the EFI_MEMORY_XP attribute
and CODE sections with the EFI_MEMORY_RO attribute when
splitting. After this process, there may still be
EfiRuntimeServicesCode regions which did not have their
attributes set because they are not part of loaded images.

This patch updates the MAT EnforceMemoryMapAttribute logic
to set the access attributes of runtime memory regions
which are not part of loaded images (have not had their
access attributes set).

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Signed-off-by: Taylor Beebe <taylor.d.beebe@gmail.com>
---
 MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c | 29 ++++++++++----------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
index e9343a2c4e..1d9f935db0 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
@@ -425,8 +425,8 @@ MergeMemoryMap (
 }
 
 /**
-  Enforce memory map attributes.
-  This function will set EfiRuntimeServicesData/EfiMemoryMappedIO/EfiMemoryMappedIOPortSpace to be EFI_MEMORY_XP.
+  Walk the memory map and set EfiRuntimeServicesData/EfiMemoryMappedIO/EfiMemoryMappedIOPortSpace
+  to EFI_MEMORY_XP and EfiRuntimeServicesCode to EFI_MEMORY_RO.
 
   @param  MemoryMap              A pointer to the buffer in which firmware places
                                  the current memory map.
@@ -447,18 +447,19 @@ EnforceMemoryMapAttribute (
   MemoryMapEntry = MemoryMap;
   MemoryMapEnd   = (EFI_MEMORY_DESCRIPTOR *)((UINT8 *)MemoryMap + MemoryMapSize);
   while ((UINTN)MemoryMapEntry < (UINTN)MemoryMapEnd) {
-    switch (MemoryMapEntry->Type) {
-      case EfiRuntimeServicesCode:
-        // do nothing
-        break;
-      case EfiRuntimeServicesData:
-      case EfiMemoryMappedIO:
-      case EfiMemoryMappedIOPortSpace:
-        MemoryMapEntry->Attribute |= EFI_MEMORY_XP;
-        break;
-      case EfiReservedMemoryType:
-      case EfiACPIMemoryNVS:
-        break;
+    if ((MemoryMapEntry->Attribute & EFI_MEMORY_ACCESS_MASK) == 0) {
+      switch (MemoryMapEntry->Type) {
+        case EfiRuntimeServicesCode:
+          MemoryMapEntry->Attribute |= EFI_MEMORY_RO;
+          break;
+        case EfiRuntimeServicesData:
+        case EfiMemoryMappedIO:
+        case EfiMemoryMappedIOPortSpace:
+          MemoryMapEntry->Attribute |= EFI_MEMORY_XP;
+          break;
+        default:
+          break;
+      }
     }
 
     MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
-- 
2.40.1.vfs.0.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117889): https://edk2.groups.io/g/devel/message/117889
Mute This Topic: https://groups.io/mt/105570114/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1] MdeModulePkg: Fixup MAT Attributes After Splitting EFI Memory Map

[Ard Biesheuvel]

Hi Taylor,

On Wed, 17 Apr 2024 at 04:28, Taylor Beebe <taylor.d.beebe@gmail.com> wrote:
>
> The Memory Attributes Table is generated by fetching the EFI
> memory map and splitting entries which contain loaded
> images so DATA and CODE sections have separate descriptors.
> The splitting is done via a call to SplitTable() which
> marks image DATA sections with the EFI_MEMORY_XP attribute
> and CODE sections with the EFI_MEMORY_RO attribute when
> splitting. After this process, there may still be
> EfiRuntimeServicesCode regions which did not have their
> attributes set because they are not part of loaded images.
>
> This patch updates the MAT EnforceMemoryMapAttribute logic
> to set the access attributes of runtime memory regions
> which are not part of loaded images (have not had their
> access attributes set).
>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Signed-off-by: Taylor Beebe <taylor.d.beebe@gmail.com>
> ---
>  MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c | 29 ++++++++++----------
>  1 file changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
> index e9343a2c4e..1d9f935db0 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
> @@ -425,8 +425,8 @@ MergeMemoryMap (
>  }
>
>  /**
> -  Enforce memory map attributes.
> -  This function will set EfiRuntimeServicesData/EfiMemoryMappedIO/EfiMemoryMappedIOPortSpace to be EFI_MEMORY_XP.
> +  Walk the memory map and set EfiRuntimeServicesData/EfiMemoryMappedIO/EfiMemoryMappedIOPortSpace
> +  to EFI_MEMORY_XP and EfiRuntimeServicesCode to EFI_MEMORY_RO.
>
>    @param  MemoryMap              A pointer to the buffer in which firmware places
>                                   the current memory map.
> @@ -447,18 +447,19 @@ EnforceMemoryMapAttribute (
>    MemoryMapEntry = MemoryMap;
>    MemoryMapEnd   = (EFI_MEMORY_DESCRIPTOR *)((UINT8 *)MemoryMap + MemoryMapSize);
>    while ((UINTN)MemoryMapEntry < (UINTN)MemoryMapEnd) {
> -    switch (MemoryMapEntry->Type) {
> -      case EfiRuntimeServicesCode:
> -        // do nothing
> -        break;
> -      case EfiRuntimeServicesData:
> -      case EfiMemoryMappedIO:
> -      case EfiMemoryMappedIOPortSpace:
> -        MemoryMapEntry->Attribute |= EFI_MEMORY_XP;
> -        break;
> -      case EfiReservedMemoryType:
> -      case EfiACPIMemoryNVS:
> -        break;
> +    if ((MemoryMapEntry->Attribute & EFI_MEMORY_ACCESS_MASK) == 0) {
> +      switch (MemoryMapEntry->Type) {
> +        case EfiRuntimeServicesCode:
> +          MemoryMapEntry->Attribute |= EFI_MEMORY_RO;

I'm not sure this is safe. If EFI_MEMORY_RO is not set, it might be intentional.

Note that the purpose of the MAT is primarily to describe permission
attributes that are more granular than the entry itself. Originally,
we tried to split those in the memory map but that broke
SetVirtualAddressMap() so we were forced to add a second table
instead.

For entries where we lack such additional metadata, I don't think we
can make assumptions based on the type beyond mapping data and MMIO
regions XP. We have no idea how those EfiRuntimeServicesCode regions
may be used, and currently, the spec permits RWX semantics for those
if no restrictions are specified.

The spec suggests that omitting an entry from the MAT is the same as
listing it with RO|XP cleared. How RO|XP from the original entry
should be interpreted wrt to the MAT is unspecified. So I think the
only thing we can do at this point is preserve the entry if it has
RO|XP set, or just drop it otherwise.

Note that the spec also mentions that the MAT must only contain
EfiRuntimeServicesCode or EfiRuntimeServicesData entries, and it looks
like this is not being enforced either.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117895): https://edk2.groups.io/g/devel/message/117895
Mute This Topic: https://groups.io/mt/105570114/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1] MdeModulePkg: Fixup MAT Attributes After Splitting EFI Memory Map

[Oliver Smith-Denny]

Hi Ard,

On 4/16/2024 11:38 PM, Ard Biesheuvel wrote:
> 
> For entries where we lack such additional metadata, I don't think we
> can make assumptions based on the type beyond mapping data and MMIO
> regions XP. We have no idea how those EfiRuntimeServicesCode regions
> may be used, and currently, the spec permits RWX semantics for those
> if no restrictions are specified.
> 
> The spec suggests that omitting an entry from the MAT is the same as
> listing it with RO|XP cleared. How RO|XP from the original entry
> should be interpreted wrt to the MAT is unspecified. So I think the
> only thing we can do at this point is preserve the entry if it has
> RO|XP set, or just drop it otherwise.

I do agree that it is probably safer to exclude the sub-region from
the MAT entirely. However, from my reading of the spec, it is
unclear to me whether it envisions that. From section 4.6.4 of
UEFI spec 2.10:

"The Memory Attributes Table may define multiple entries to describe
sub-regions that comprise a single entry returned by GetMemoryMap()
however the sub-regions must total to completely describe the larger
region and may not cross boundaries between entries reported by
GetMemoryMap() . If a run-time region returned in GetMemoryMap() entry
is not described within the Memory Attributes Table, this region is
assumed to not be compatible with any memory protections."

The unclear part to me here is "the sub-regions must total to completely
describe the larger region." To me that says that in Taylor's case,
where we have a memory map descriptor with attributes set for part of
the region but not the whole region, that the spec envisions the MAT
having either the whole region split into sub-regions or not
including the MAT entry for this region. In this reading the final
sentence would say that if an entire memory map entry is not
described in the MAT, then it is assumed to be incompatible.

A different reading would say what you are saying, that a sub-region
can be dropped from the MAT (although it is hard to justify that with
the language that says the sub-regions must total to completely
describe the larger region). What are your thoughts here?

Aside from this, I wonder if we can be more aspirational here. These
EfiRuntimeServicesCode regions without attributes set are, if I am
understanding correctly, from loaded images. The spec does call out
that EfiRuntimeServicesCode is explicitly for code sections of loaded
images. Can we just say outright that any memory of this type should
be RO? I understand that existing drivers may attempt to break this,
but from the core, I think this is reasonable to say, but would love
to hear thoughts.

> 
> Note that the spec also mentions that the MAT must only contain
> EfiRuntimeServicesCode or EfiRuntimeServicesData entries, and it looks
> like this is not being enforced either.

Agreed, this should be amended.

Thanks,
Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117920): https://edk2.groups.io/g/devel/message/117920
Mute This Topic: https://groups.io/mt/105570114/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1] MdeModulePkg: Fixup MAT Attributes After Splitting EFI Memory Map

[Taylor Beebe]

On 4/17/2024 6:40 AM, Oliver Smith-Denny wrote:
> Hi Ard,
>
> On 4/16/2024 11:38 PM, Ard Biesheuvel wrote:
>>
>> For entries where we lack such additional metadata, I don't think we
>> can make assumptions based on the type beyond mapping data and MMIO
>> regions XP. We have no idea how those EfiRuntimeServicesCode regions
>> may be used, and currently, the spec permits RWX semantics for those
>> if no restrictions are specified.
>>
The logic before the ImagePropertiesRecordLib addition was applying
XP to these EfiRuntimeServicesCode regions which are not part of loaded
images. I agree with you that we cannot make assumptions on how these
regions are used, but it seems the logic was this way long enough for
expectations to be built around the incorrect behaviour. Something to
consider.
>> The spec suggests that omitting an entry from the MAT is the same as
>> listing it with RO|XP cleared. How RO|XP from the original entry
>> should be interpreted wrt to the MAT is unspecified. So I think the
>> only thing we can do at this point is preserve the entry if it has
>> RO|XP set, or just drop it otherwise.
>
> I do agree that it is probably safer to exclude the sub-region from
> the MAT entirely. However, from my reading of the spec, it is
> unclear to me whether it envisions that. From section 4.6.4 of
> UEFI spec 2.10:
>
> "The Memory Attributes Table may define multiple entries to describe
> sub-regions that comprise a single entry returned by GetMemoryMap()
> however the sub-regions must total to completely describe the larger
> region and may not cross boundaries between entries reported by
> GetMemoryMap() . If a run-time region returned in GetMemoryMap() entry
> is not described within the Memory Attributes Table, this region is
> assumed to not be compatible with any memory protections."
>
> The unclear part to me here is "the sub-regions must total to completely
> describe the larger region." To me that says that in Taylor's case,
> where we have a memory map descriptor with attributes set for part of
> the region but not the whole region, that the spec envisions the MAT
> having either the whole region split into sub-regions or not
> including the MAT entry for this region. In this reading the final
> sentence would say that if an entire memory map entry is not
> described in the MAT, then it is assumed to be incompatible.
>
> A different reading would say what you are saying, that a sub-region
> can be dropped from the MAT (although it is hard to justify that with
> the language that says the sub-regions must total to completely
> describe the larger region). What are your thoughts here?
>
> Aside from this, I wonder if we can be more aspirational here. These
> EfiRuntimeServicesCode regions without attributes set are, if I am
> understanding correctly, from loaded images. 
These EfiRuntimeServicesCode regions without attributes set are
not part of loaded image memory. I think that's what you meant but
wanted to clarify.
> The spec does call out
> that EfiRuntimeServicesCode is explicitly for code sections of loaded
> images. Can we just say outright that any memory of this type should
> be RO? I understand that existing drivers may attempt to break this,
> but from the core, I think this is reasonable to say, but would love
> to hear thoughts.
>>
>> Note that the spec also mentions that the MAT must only contain
>> EfiRuntimeServicesCode or EfiRuntimeServicesData entries, and it looks
>> like this is not being enforced either.
>
> Agreed, this should be amended.
The InstallMemoryAttributesTable() logic will only add the 
EfiRuntimeServicesCode
and EfiRuntimeServicesData regions to the final MAT. I'm not sure why
EnforceMemoryMapAttribute() is setting XP on EfiMemoryMappedIO and
EfiMemoryMappedIOPortSpace here when those entries are not included in the
MAT anyways.
>
> Thanks,
> Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117922): https://edk2.groups.io/g/devel/message/117922
Mute This Topic: https://groups.io/mt/105570114/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1] MdeModulePkg: Fixup MAT Attributes After Splitting EFI Memory Map

[Oliver Smith-Denny]

On 4/17/2024 7:05 AM, Taylor Beebe wrote:
> 
> On 4/17/2024 6:40 AM, Oliver Smith-Denny wrote:
>> Aside from this, I wonder if we can be more aspirational here. These
>> EfiRuntimeServicesCode regions without attributes set are, if I am
>> understanding correctly, from loaded images. 
> These EfiRuntimeServicesCode regions without attributes set are
> not part of loaded image memory. I think that's what you meant but
> wanted to clarify.

Are these regions without attributes from image sections that have
been padded to RUNTIME_PAGE_ALLOCATION_GRANULARITY, i.e. they are
the pads? Or are we saying we don't know what these regions are
at this point? It is true in theory someone could just allocate
an EfiRuntimeServicesCode section.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117923): https://edk2.groups.io/g/devel/message/117923
Mute This Topic: https://groups.io/mt/105570114/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1] MdeModulePkg: Fixup MAT Attributes After Splitting EFI Memory Map

[Taylor Beebe]

On 4/17/2024 7:09 AM, Oliver Smith-Denny wrote:
> On 4/17/2024 7:05 AM, Taylor Beebe wrote:
>>
>> On 4/17/2024 6:40 AM, Oliver Smith-Denny wrote:
>>> Aside from this, I wonder if we can be more aspirational here. These
>>> EfiRuntimeServicesCode regions without attributes set are, if I am
>>> understanding correctly, from loaded images. 
>> These EfiRuntimeServicesCode regions without attributes set are
>> not part of loaded image memory. I think that's what you meant but
>> wanted to clarify.
>
> Are these regions without attributes from image sections that have
> been padded to RUNTIME_PAGE_ALLOCATION_GRANULARITY, i.e. they are
> the pads? Or are we saying we don't know what these regions are
> at this point? It is true in theory someone could just allocate
> an EfiRuntimeServicesCode section.
Good question -- I had not considered the extra padding applied
to these allocations. It could be either. The memory map returned
via GetMemoryMap() will merge descriptors together based on type
so it's possible to mistake an unrelated EfiRuntimeServicesCode
allocation with padding applied to a runtime image memory
allocation if they are contiguous.

When the IMAGE_PROPERTIES_RECORD entry is created, perhaps
it would be best to set the ImageSize field to the padded allocation
size instead of the file size. Is this the difference between virtual size
and raw data size? I recall you recently did this in 
ImagePropertiesRecordLib
for the code size of a new entry.

-Taylor



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117924): https://edk2.groups.io/g/devel/message/117924
Mute This Topic: https://groups.io/mt/105570114/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1] MdeModulePkg: Fixup MAT Attributes After Splitting EFI Memory Map

[Ard Biesheuvel]

On Wed, 17 Apr 2024 at 16:34, Taylor Beebe <taylor.d.beebe@gmail.com> wrote:
>
>
> On 4/17/2024 7:09 AM, Oliver Smith-Denny wrote:
> > On 4/17/2024 7:05 AM, Taylor Beebe wrote:
> >>
> >> On 4/17/2024 6:40 AM, Oliver Smith-Denny wrote:
> >>> Aside from this, I wonder if we can be more aspirational here. These
> >>> EfiRuntimeServicesCode regions without attributes set are, if I am
> >>> understanding correctly, from loaded images.
> >> These EfiRuntimeServicesCode regions without attributes set are
> >> not part of loaded image memory. I think that's what you meant but
> >> wanted to clarify.
> >
> > Are these regions without attributes from image sections that have
> > been padded to RUNTIME_PAGE_ALLOCATION_GRANULARITY, i.e. they are
> > the pads? Or are we saying we don't know what these regions are
> > at this point? It is true in theory someone could just allocate
> > an EfiRuntimeServicesCode section.
> Good question -- I had not considered the extra padding applied
> to these allocations. It could be either. The memory map returned
> via GetMemoryMap() will merge descriptors together based on type
> so it's possible to mistake an unrelated EfiRuntimeServicesCode
> allocation with padding applied to a runtime image memory
> allocation if they are contiguous.
>

An arbitary Linux/arm64 on edk2 VM boot gives me

memory map:
...
0x00007c770000-0x00007c7effff [Runtime Code|RUN|  |  |  |  |  |  |  |  ]
0x00007c7f0000-0x00007c95ffff [Runtime Data|RUN|  |  |  |  |  |  |  |  ]
0x00007c960000-0x00007c9affff [Runtime Code|RUN|  |  |  |  |  |  |  |  ]
0x00007c9b0000-0x00007ca4ffff [Runtime Data|RUN|  |  |  |  |  |  |  |  ]
0x00007ca50000-0x00007cb3ffff [Runtime Code|RUN|  |  |  |  |  |  |  |  ]
0x00007cb40000-0x00007cb42fff [Conventional|   |  |  |  |  |  |  |  |  ]
0x00007cb43000-0x00007cb43fff [Loader Data |   |  |  |  |  |  |  |  |  ]
0x00007cb44000-0x00007ecd5fff [Conventional|   |  |  |  |  |  |  |  |  ]
0x00007ecd6000-0x00007fa37fff [Boot Data   |   |  |  |  |  |  |  |  |  ]
0x00007fa38000-0x00007faa0fff [Conventional|   |  |  |  |  |  |  |  |  ]
0x00007faa1000-0x00007fe1ffff [Boot Code   |   |  |  |  |  |  |  |  |  ]
0x00007fe20000-0x00007feaffff [Runtime Code|RUN|  |  |  |  |  |  |  |  ]
0x00007feb0000-0x00007febffff [Conventional|   |  |  |  |  |  |  |  |  ]
0x00007fec0000-0x00007ffdffff [Runtime Data|RUN|  |  |  |  |  |  |  |  ]
...

MAT:
0x00007c770000-0x00007c77ffff [Runtime Code|RUN|  |  |  |  |XP|  |  |  ]
0x00007c780000-0x00007c78ffff [Runtime Code|RUN|  |  |  |  |  |  |  |RO]
0x00007c790000-0x00007c7bffff [Runtime Code|RUN|  |  |  |  |XP|  |  |  ]
0x00007c7c0000-0x00007c7cffff [Runtime Code|RUN|  |  |  |  |  |  |  |RO]
0x00007c7d0000-0x00007c7effff [Runtime Code|RUN|  |  |  |  |XP|  |  |  ]
0x00007c7f0000-0x00007c95ffff [Runtime Data|RUN|  |  |  |  |XP|  |  |  ]
0x00007c960000-0x00007c96ffff [Runtime Code|RUN|  |  |  |  |XP|  |  |  ]
0x00007c970000-0x00007c97ffff [Runtime Code|RUN|  |  |  |  |  |  |  |RO]
0x00007c980000-0x00007c9affff [Runtime Code|RUN|  |  |  |  |XP|  |  |  ]
0x00007c9b0000-0x00007ca4ffff [Runtime Data|RUN|  |  |  |  |XP|  |  |  ]
0x00007ca50000-0x00007ca5ffff [Runtime Code|RUN|  |  |  |  |XP|  |  |  ]
0x00007ca60000-0x00007ca6ffff [Runtime Code|RUN|  |  |  |  |  |  |  |RO]
0x00007ca70000-0x00007caaffff [Runtime Code|RUN|  |  |  |  |XP|  |  |  ]
0x00007cab0000-0x00007cabffff [Runtime Code|RUN|  |  |  |  |  |  |  |RO]
0x00007cac0000-0x00007cafffff [Runtime Code|RUN|  |  |  |  |XP|  |  |  ]
0x00007cb00000-0x00007cb0ffff [Runtime Code|RUN|  |  |  |  |  |  |  |RO]
0x00007cb10000-0x00007cb3ffff [Runtime Code|RUN|  |  |  |  |XP|  |  |  ]
0x00007fe20000-0x00007fe2ffff [Runtime Code|RUN|  |  |  |  |XP|  |  |  ]
0x00007fe30000-0x00007fe3ffff [Runtime Code|RUN|  |  |  |  |  |  |  |RO]
0x00007fe40000-0x00007fe6ffff [Runtime Code|RUN|  |  |  |  |XP|  |  |  ]
0x00007fe70000-0x00007fe7ffff [Runtime Code|RUN|  |  |  |  |  |  |  |RO]
0x00007fe80000-0x00007feaffff [Runtime Code|RUN|  |  |  |  |XP|  |  |  ]
0x00007fec0000-0x00007ffdffff [Runtime Data|RUN|  |  |  |  |XP|  |  |  ]

So there are far fewer entries in the memory map than in the MAT, due
to the merging. AFAICT, there are ~8 runtime DXEs here.

I imagine other Rt Code regions could easily be get merged in here as well.

The wording about describing the larger region entirely is
unfortunate: I was involved in the creation of the MAT, and this
merging was definitely not taken into account.

The MAT replaced a feature where runtime DXE images were split into
separate Rt code and date memmap entries based on the PE sections, and
this broke spectacularly due to the fact that SetVirtualAddressMap()
may reorder those regions in virtual memory, which breaks relative
references between code and data in those runtime DXE programs.

So the intent has always been to describe memory attributes at a
higher level of detail without relying on the PE/COFF header directly
(which principle went out the window with PRM but that is another
discussion)

So the purpose of the MAT is to describe RT code (and to a lesser
extent, RT data) regions where we cannot apply either RO or XP to the
whole thing. IIRC there was never an intent to exhaustively describe
all memory runtime regions. Also note that RO was introduced at this
point, because WP was already being used in the ordinary memory map in
a deviating manner. RO is defined both for the memory map and the MAT,
and so it can occur in either.


> When the IMAGE_PROPERTIES_RECORD entry is created, perhaps
> it would be best to set the ImageSize field to the padded allocation
> size instead of the file size. Is this the difference between virtual size
> and raw data size? I recall you recently did this in
> ImagePropertiesRecordLib
> for the code size of a new entry.
>

Not sure whether the padding should matter tbh. All these entries
should describe the situation after the PE image was rounded outwards
to the minimum runtime alignment.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117925): https://edk2.groups.io/g/devel/message/117925
Mute This Topic: https://groups.io/mt/105570114/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1] MdeModulePkg: Fixup MAT Attributes After Splitting EFI Memory Map

[Oliver Smith-Denny]

On 4/17/2024 7:34 AM, Taylor Beebe wrote:
> 
> On 4/17/2024 7:09 AM, Oliver Smith-Denny wrote:
>> On 4/17/2024 7:05 AM, Taylor Beebe wrote:
>>>
>>> On 4/17/2024 6:40 AM, Oliver Smith-Denny wrote:
>>>> Aside from this, I wonder if we can be more aspirational here. These
>>>> EfiRuntimeServicesCode regions without attributes set are, if I am
>>>> understanding correctly, from loaded images. 
>>> These EfiRuntimeServicesCode regions without attributes set are
>>> not part of loaded image memory. I think that's what you meant but
>>> wanted to clarify.
>>
>> Are these regions without attributes from image sections that have
>> been padded to RUNTIME_PAGE_ALLOCATION_GRANULARITY, i.e. they are
>> the pads? Or are we saying we don't know what these regions are
>> at this point? It is true in theory someone could just allocate
>> an EfiRuntimeServicesCode section.
> Good question -- I had not considered the extra padding applied
> to these allocations. It could be either. The memory map returned
> via GetMemoryMap() will merge descriptors together based on type
> so it's possible to mistake an unrelated EfiRuntimeServicesCode
> allocation with padding applied to a runtime image memory
> allocation if they are contiguous.

Taylor and I had an offline conversation and checked on this, my
recent patch moving ImagePropertiesRecordLib to use VirtualSize
instead of SizeOfRawData fixed this. So we should not see any
padding sections as without attributes.

Now, for the case of ARM64, where you have 64k runtime
granularity and often will end up with the case of many
extra pages in a code section, those pages will be marked
as RO and executable, even though they contain garbage. I think
it would be worthwhile to mark the excess garbage pages, if they
exist for a given section, as RP. Nothing should be using them
in any fashion, they are padding.

Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117934): https://edk2.groups.io/g/devel/message/117934
Mute This Topic: https://groups.io/mt/105570114/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1] MdeModulePkg: Fixup MAT Attributes After Splitting EFI Memory Map

[Oliver Smith-Denny]

On 4/17/2024 9:52 AM, Ard Biesheuvel wrote:
> So the purpose of the MAT is to describe RT code (and to a lesser
> extent, RT data) regions where we cannot apply either RO or XP to the
> whole thing. IIRC there was never an intent to exhaustively describe
> all memory runtime regions. Also note that RO was introduced at this
> point, because WP was already being used in the ordinary memory map in
> a deviating manner. RO is defined both for the memory map and the MAT,
> and so it can occur in either.
> 

At the principle level, I think we can say that we want all runtime code
regions to RO and all runtime data regions to be XP. Regardless of
whatever situation we have today, I think this is a reasonable
principle to maintain. If you don't want those attributes, a different
memory type should be allocated. If we agree on this principle, I think
we should put it into practice. Again, the UEFI spec calls out that
EfiRuntimeServicesCode is for image code. From a security and safety
standpoint, we know we want image code to be RO.

To help with any existing (mis)use of EfiRuntimeServicesCode, I do
think we should put a big old assert in the MAT generation logic that
says I found a EfiRuntimeServicesCode section that is not described
in an image record, something is wrong with your configuration, you
are not using EfiRuntimeServicesCode correctly. If I am missing a
legitimate use of EfiRuntimeServicesCode, please help educate me.

Also, I know that modern Windows security features rely on the MAT
describing all EfiRuntimeServicesCode and EfiRuntimeServicesData
regions. Here is an MSDN link that makes a statement towards
that:

https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/unified-extensible-firmware-interface

In a more actionable way, the Windows testing infrastructure will
test to ensure that there are no EfiRuntimeServices[Code|Data]
sections in the EFI memory map that are not described in the MAT.
Again, there are various security features that rely on this.

Thanks,
Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117935): https://edk2.groups.io/g/devel/message/117935
Mute This Topic: https://groups.io/mt/105570114/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1] MdeModulePkg: Fixup MAT Attributes After Splitting EFI Memory Map

[Oliver Smith-Denny]

On 4/17/2024 2:41 PM, Oliver Smith-Denny wrote:
> Now, for the case of ARM64, where you have 64k runtime
> granularity and often will end up with the case of many
> extra pages in a code section, those pages will be marked
> as RO and executable, even though they contain garbage. I think
> it would be worthwhile to mark the excess garbage pages, if they
> exist for a given section, as RP. Nothing should be using them
> in any fashion, they are padding.

I thought this through some more. We can't do this because of
the UEFI spec provision that says no 64k region can have different
attributes set. So, we leave open the possibility of executing
garbage :)

Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117936): https://edk2.groups.io/g/devel/message/117936
Mute This Topic: https://groups.io/mt/105570114/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-