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: 4817 bytes --]


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 (#117813): https://edk2.groups.io/g/devel/message/117813
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: 6314 bytes --]