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

["Dandan Bi" <dandan.bi@intel.com>]

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.

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.



Thanks,
Dandan
-----Original Message-----
From: Huang, Yanbo <yanbo.huang@intel.com> 
Sent: Sunday, April 14, 2024 10:36 PM
To: Taylor Beebe <taylor.d.beebe@gmail.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Bi, Dandan <dandan.bi@intel.com>; Zhou, Jianfeng <jianfeng.zhou@intel.com>
Subject: RE: MdeModulePkg: Fix MAT SplitRecord() Logic introduce one bug and will cause SUT reset when boot to windows

Hi Taylor,

For your mentioned: "In this case, because the memory type of the buffer is EfiRuntimeServicesCode, shouldn't the final pages be EFI_MEMORY_RO?"

After print the attributes, the attribute are not set to EFI_MEMORY_RO, nearly all of the NewRecord->Attribute are set to 0 in SplitRecord API.

Best Regards,
Yanbo Huang
-----Original Message-----
From: Taylor Beebe <taylor.d.beebe@gmail.com>
Sent: Friday, April 12, 2024 11:10 PM
To: 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>; Bi, Dandan <dandan.bi@intel.com>; Zhou, Jianfeng <jianfeng.zhou@intel.com>
Subject: Re: MdeModulePkg: Fix MAT SplitRecord() Logic introduce one bug and will cause SUT reset when boot to windows

Hi Yanbo,

Can you help me understand the memory layout which causes this issue?

If a single EfiRuntimeServicesCode descriptor needs to be split because an image is within the memory range. I think that descriptor is split like so in the case you're encountering:

-------------------  ---       ---
|       DATA      |     |        |
-------------------     |        |
|       CODE      |     | Image  |
-------------------     | Memory | EfiRuntimeServicesCode
|       DATA      |     |        |
-------------------  ---         |
|   Extra Pages   |              |
-------------------            ---

In this case, because the memory type of the buffer is EfiRuntimeServicesCode, shouldn't the final pages be EFI_MEMORY_RO?

Thanks!
-Taylor
On 4/11/2024 10:14 PM, Huang, Yanbo wrote:
> Hi Beebe,
>
> Recently we found this commit " MdeModulePkg: Fix MAT SplitRecord() Logic " will cause SUT reset after enable some knobs.
> I filed one Bugzilla for it: 
> https://bugzilla.tianocore.org/show_bug.cgi?id=4751
>
> After debug, we found in SplitRecord API, many entries attribute are set to 0, not align with the UEFI spec:
> "Memory Attributes Table (MAT):
> EFI_MEMORY_ATTRIBUTES_TABLE. The entire UEFI runtime must be described by this table.
> All entries must include attributes EFI_MEMORY_RO, EFI_MEMORY_XP, or both. Memory MUST be either readable and executable OR writeable and non-executable."
> This should be the root cause of this issue.
> When we update "NewRecord->Attribute     = TempRecord.Attribute;" to "NewRecord->Attribute     = TempRecord.Attribute | EFI_MEMORY_XP;", SUT can boot to windows.
>
> @taylor.d.beebe@gmail.com Could you please help to send one formal fix patch for this issue?
> Thanks!
>
> Best Regards,
> Yanbo Huang
>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Taylor 
> Beebe
> Sent: Tuesday, November 28, 2023 2:18 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming 
> <gaoliming@byosoft.com.cn>; Bi, Dandan <dandan.bi@intel.com>
> Subject: [edk2-devel] [PATCH v5 10/16] MdeModulePkg: Fix MAT
> SplitRecord() Logic
>
> SplitRecord() does not handle the case where a memory descriptor describes an image region plus extra pages before or after the image region. This patch fixes this case by carving off the unrelated regions into their own descriptors.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Signed-off-by: Taylor Beebe <taylor.d.beebe@gmail.com>
> Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
> ---
>   MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c | 56 ++++++++++----------
>   1 file changed, 27 insertions(+), 29 deletions(-)
>
> diff --git
> a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordL
> ib.c
> b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordL
> ib.c index 7c0ecd07c1bb..9d4082280bf5 100644
> ---
> a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordL
> ib.c
> +++ b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRec
> +++ or
> +++ dLib.c
> @@ -323,7 +323,6 @@ SplitRecord (
>     UINT64                   PhysicalEnd;
>     UINTN                    NewRecordCount;
>     UINTN                    TotalNewRecordCount;
> -  BOOLEAN                  IsLastRecordData;
>   
>     if (MaxSplitRecordCount == 0) {
>       CopyMem (NewRecord, OldRecord, DescriptorSize); @@ -344,35 +343,16 @@ SplitRecord (
>       NewImageRecord = GetImageRecordByAddress (PhysicalStart, PhysicalEnd - PhysicalStart, ImageRecordList);
>       if (NewImageRecord == NULL) {
>         //
> -      // No more image covered by this range, stop
> +      // No more images cover this range, check if we've reached the end of the old descriptor. If not,
> +      // add the remaining range to the new descriptor list.
>         //
> -      if ((PhysicalEnd > PhysicalStart) && (ImageRecord != NULL)) {
> -        //
> -        // If this is still address in this record, need record.
> -        //
> -        NewRecord        = PREVIOUS_MEMORY_DESCRIPTOR (NewRecord, DescriptorSize);
> -        IsLastRecordData = FALSE;
> -        if ((NewRecord->Attribute & EFI_MEMORY_XP) != 0) {
> -          IsLastRecordData = TRUE;
> -        }
> -
> -        if (IsLastRecordData) {
> -          //
> -          // Last record is DATA, just merge it.
> -          //
> -          NewRecord->NumberOfPages = EfiSizeToPages (PhysicalEnd - NewRecord->PhysicalStart);
> -        } else {
> -          //
> -          // Last record is CODE, create a new DATA entry.
> -          //
> -          NewRecord                = NEXT_MEMORY_DESCRIPTOR (NewRecord, DescriptorSize);
> -          NewRecord->Type          = TempRecord.Type;
> -          NewRecord->PhysicalStart = TempRecord.PhysicalStart;
> -          NewRecord->VirtualStart  = 0;
> -          NewRecord->NumberOfPages = TempRecord.NumberOfPages;
> -          NewRecord->Attribute     = TempRecord.Attribute | EFI_MEMORY_XP;
> -          TotalNewRecordCount++;
> -        }
> +      if (PhysicalEnd > PhysicalStart) {
> +        NewRecord->Type          = TempRecord.Type;
> +        NewRecord->PhysicalStart = PhysicalStart;
> +        NewRecord->VirtualStart  = 0;
> +        NewRecord->NumberOfPages = EfiSizeToPages (PhysicalEnd - PhysicalStart);
> +        NewRecord->Attribute     = TempRecord.Attribute;
> +        TotalNewRecordCount++;
>         }
>   
>         break;
> @@ -380,6 +360,24 @@ SplitRecord (
>   
>       ImageRecord = NewImageRecord;
>   
> +    //
> +    // Update PhysicalStart to exclude the portion before the image buffer
> +    //
> +    if (TempRecord.PhysicalStart < ImageRecord->ImageBase) {
> +      NewRecord->Type          = TempRecord.Type;
> +      NewRecord->PhysicalStart = TempRecord.PhysicalStart;
> +      NewRecord->VirtualStart  = 0;
> +      NewRecord->NumberOfPages = EfiSizeToPages (ImageRecord->ImageBase - TempRecord.PhysicalStart);
> +      NewRecord->Attribute     = TempRecord.Attribute;
> +      TotalNewRecordCount++;
> +
> +      PhysicalStart            = ImageRecord->ImageBase;
> +      TempRecord.PhysicalStart = PhysicalStart;
> +      TempRecord.NumberOfPages = EfiSizeToPages (PhysicalEnd - 
> + PhysicalStart);
> +
> +      NewRecord = (EFI_MEMORY_DESCRIPTOR *)((UINT8 *)NewRecord + DescriptorSize);
> +    }
> +
>       //
>       // Set new record
>       //
> --
> 2.42.0.windows.2
>
>
>
> 
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117773): https://edk2.groups.io/g/devel/message/117773
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]
-=-=-=-=-=-=-=-=-=-=-=-