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

["Taylor Beebe" <taylor.d.beebe@gmail.com>]

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