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

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/ImagePropertiesRecordLib.c b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
> index 7c0ecd07c1bb..9d4082280bf5 100644
> --- a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
> +++ b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecor
> +++ 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 (#117710): https://edk2.groups.io/g/devel/message/117710
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]
-=-=-=-=-=-=-=-=-=-=-=-