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

["Oliver Smith-Denny" <osde@linux.microsoft.com>]

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]
-=-=-=-=-=-=-=-=-=-=-=-