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

["Huang, Yanbo" <yanbo.huang@intel.com>]

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

Hi Taylor,

Thanks for your update.
After test, issue can be fixed by your patch.
But why we not set the EFI_MEMORY_XP or EFI_MEMORY_RO attribute in SplitRecord API?
If we set the attribute in the beginning of the NewRecord created, it seems we don’t need to EnforceMemoryMapAttribute later?

Best Regards,
Yanbo Huang

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Taylor Beebe
Sent: Wednesday, April 17, 2024 10:33 AM
To: Bi, Dandan <dandan.bi@intel.com>; Huang, Yanbo <yanbo.huang@intel.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Zhou, Jianfeng <jianfeng.zhou@intel.com>
Subject: Re: [edk2-devel] MdeModulePkg: Fix MAT SplitRecord() Logic introduce one bug and will cause SUT reset when boot to windows


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-0x769E5000 is 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 (#117921): https://edk2.groups.io/g/devel/message/117921
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: 11973 bytes --]