Re: [edk2-devel] MdeModulePkg: Fix MAT SplitRecord() Logic introduce one bug and will cause SUT reset when boot to windows

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

[-- Attachment #1: Type: text/plain, Size: 5226 bytes --]

Hi Yanbo,

Can you confirm that the following resolves the issue you're seeing?

[PATCH v1] MdeModulePkg: Fixup MAT Attributes After Splitting EFI Memory 
Map (groups.io) <https://edk2.groups.io/g/devel/message/117889>

-Taylor

On 4/15/2024 6:17 PM, Taylor Beebe wrote:
> On 4/15/2024 3:57 AM, Bi, Dandan wrote:
>> Hi Taylor,
>>
>> With this patch, MAT contains some entries with Attribute - 0x8000000000000000, doesn't have EFI_MEMORY_RO or EFI_MEMORY_XP.
>> After revert this patch, don't see such entries in MAT.
>>
>> a. MAT with this patch:
>> Entry (0x609E4268)
>>    Type              - 0x5
>>    PhysicalStart     - 0x00000000769CF000
>>    VirtualStart      - 0x0000000000000000
>>    NumberOfPages     - 0x0000000000000016
>>    Attribute         - 0x8000000000000000
>> Entry (0x609E4298)
>>    Type              - 0x5
>>    PhysicalStart     - 0x00000000769E5000
>>    VirtualStart      - 0x0000000000000000
>>    NumberOfPages     - 0x0000000000000001
>>    Attribute         - 0x8000000000004000
>> Entry (0x609E42C8)
>>    Type              - 0x5
>>    PhysicalStart     - 0x00000000769E6000
>>    VirtualStart      - 0x0000000000000000
>>    NumberOfPages     - 0x0000000000000002
>>    Attribute         - 0x8000000000020000
>>
>> b. MAT without this patch:
>> Entry (0x609E4268)
>>    Type              - 0x5
>>    PhysicalStart     - 0x00000000769CF000
>>    VirtualStart      - 0x0000000000000000
>>    NumberOfPages     - 0x0000000000000017
>>    Attribute         - 0x8000000000004000
>> Entry (0x609E4298)
>>    Type              - 0x5
>>    PhysicalStart     - 0x00000000769E6000
>>    VirtualStart      - 0x0000000000000000
>>    NumberOfPages     - 0x0000000000000002
>>    Attribute         - 0x8000000000020000
>>
>> 1. For example, when OldRecord in old memory map with:
>>          Type - 0x00000005
>>          Attribute - 0x800000000000000F
>>          PhysicalStart - 0x769CF000
>>      PhysicalStart is smaller than ImageBase 0x769E5000, with this patch, it will create a new memory descriptor entry for range 0x769CF000~0x769E5000 and without EFI_MEMORY_RO or EFI_MEMORY_XP Attribute.
>>      Then it will only contain EFI_MEMORY_RUNTIME Attribute in MAT as doing  MemoryAttributesEntry->Attribute &= (EFI_MEMORY_RO|EFI_MEMORY_XP|EFI_MEMORY_RUNTIME); when install MAT.
>>      It seems not aligned with UEFI Spec " The only valid bits for Attribute field currently are EFI_MEMORY_RO ,EFI_MEMORY_XP , plus EFI_MEMORY_RUNTIME "?
>>      Could you please help double check? Thanks.
> Agreed that this is currently the behaviour and that the range should 
> have a restrictive access attribute. More below.
>> 2. In function SetNewRecord, it semes already cover the DATA entry before the CODE and the DATA entry after the CODE.
>>      And old SplitRecord function without this patch, also has the entry to cover the reaming range of this record if no more image covered by this range.
>>      Why do we still need this patch? Could you please help explain? Thanks.
>
> GetMemoryMap() will merge adjacent descriptors which have the same 
> type and attributes. This means that a single EfiRuntimeServicesCode 
> descriptor within the memory map returned by CoreGetMemoryMap() could 
> describe memory with the following layout (NOTE: this layout is odd 
> but needs to be handled to fulfill the SplitTable() contract):
>
> -------------------------
> Some EfiRuntimeServicesCode memory
> -------------------------
> Runtime Image DATA Section
> -------------------------
> Runtime Image CODE Section
> -------------------------
> Runtime Image DATA Section
> -------------------------
> Some EfiRuntimeServicesCode memory
> -------------------------
>
> In this possible layout, the pre-patch logic would assume that the 
> regions before and after the image were part of the image's data 
> sections and would mark them as EFI_MEMORY_XP. The post-patch logic 
> does not mark them with any access attributes which is fine but the 
> DXE MAT logic needs to walk the memory map returned by SplitTable() to 
> add access attributes to runtime regions which don't have any.
>
> In your example, because PhysicalStart < ImageBase and 
> 0769CF000-0x769E5000 is EfiRuntimeServicesCode, the access attribute 
> should technically be EFI_MEMORY_RO. It's likely that 
> 0769CF000-0x769E5000is just the unused memory bucket and so it might 
> be best to mark it both EFI_MEMORY_RO and EFI_MEMORY_XP.
>
> *An open question to the community:* Are there cases where runtime 
> executable code is in a buffer separate from a loaded runtime image? 
> Can the EfiRuntimeServicesCode memory regions not part of an image be 
> specified in the MAT as both EFI_MEMORY_RO and EFI_MEMORY_XP, or even 
> dropped entirely?
>
> Thanks :)
> -Taylor


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



[-- Attachment #2: Type: text/html, Size: 6993 bytes --]