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

2024-04-23 Thread Oliver Smith-Denny 

On 4/18/2024 11:43 PM, Ni, Ray wrote:



So this is just junk unallocated memory that we are reporting as
a type it *could* be if an allocation occurs to minimize failures
of ExitBootServices. Which is questionable. But in terms of
attributes, I would expect we either have this unallocated
memory marked the same as the bin type or better, mark it RP
if we can (Taylor is making a change to set RP on free memory
by default, so we would have this in the page table, but we
would need to decide what we tell the OS).

[Ray] When reviewing today's logic of memory protection through page 
table, I feel that it was designed improperly in the beginning.

My rough thought is:
* All memory is RP initially (as you said Taylor will do that)


Correct, Taylor is working on a change here and actually taking this a
step further, that all free memory will be RP, to catch any use after
free and keep a safer environment.

* Allocated memory is mapped as either RO or XD, depending on code/data. 
Or RP if it's a guard page.


This is mostly true, of course it depends on how the memory protections
are configured. I would like to see this go to not being an option but
something that DxeCore enforces by default depending on memory type
allocated.



Maybe I am not aware of some limitations of the above idea. The 
limitations prevented the initial design be in this way.

Or what Taylor will do aligns to the idea?



The issue in this mailing thread is not what DXE's page tables are,
but what get reported in the MAT to the OS. Before Taylor's change
to improve the SplitTable logic the extra RuntimeServicesCode sections
that get reported to the OS (these are the leftover sections in the
memory bins to improve the chance for S4 resume) were getting reported
as XP. Taylor is proposing these get marked RO instead as they are
marked as Code sections (although they really hold junk in them).

Another path would be can we mark them both RO and XP. These are junk
sections, they should not be used and definitely not executed from. We
only report them to the OS so that our memory map changes less between
boots (I also wonder if there are better ways we can do this, but I'll
have to think about this more).

Thanks,
Oliver




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118143): https://edk2.groups.io/g/devel/message/118143
Mute This Topic: https://groups.io/mt/105477564/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2024-04-19 Thread Ni, Ray 


So this is just junk unallocated memory that we are reporting as
a type it *could* be if an allocation occurs to minimize failures
of ExitBootServices. Which is questionable. But in terms of
attributes, I would expect we either have this unallocated
memory marked the same as the bin type or better, mark it RP
if we can (Taylor is making a change to set RP on free memory
by default, so we would have this in the page table, but we
would need to decide what we tell the OS).

[Ray] When reviewing today's logic of memory protection through page table, I 
feel that it was designed improperly in the beginning.
My rough thought is:
* All memory is RP initially (as you said Taylor will do that)
* Allocated memory is mapped as either RO or XD, depending on code/data. Or RP 
if it's a guard page.

Maybe I am not aware of some limitations of the above idea. The limitations 
prevented the initial design be in this way.
Or what Taylor will do aligns to the idea?

Thanks,
Ray


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118006): https://edk2.groups.io/g/devel/message/118006
Mute This Topic: https://groups.io/mt/105477564/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2024-04-18 Thread Oliver Smith-Denny 

On 4/18/2024 6:56 AM, Huang, Yanbo wrote:

The PCD PcdPlatformEfiRtCodeMemorySize is used in 
https://github.com/tianocore/edk2-platforms/blob/master/Platform/Intel/MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.c
This PCD seems defined the size allocated for run time services code, and the 
similar PCD is PcdPlatformEfiRtDataMemorySize, seems defined the size allocated 
for run time services data.
I guess dandan means if the runtime services code size is small than the PCD defined, 
then the " Extra EfiRuntimeServicesCode regions which aren't part of loaded runtime 
images." size should be FixedPcdGet32 (PcdPlatformEfiRtCodeMemorySize) - the actual 
runtime image code size?



Drawing the lines here, we will lie in the EFI_MEMORY_MAP to say that
all bin memory is allocated, regardless of this PCD:

https://github.com/tianocore/edk2/blob/0afb8743493853e30171f6000de51242e22a1eb8/MdeModulePkg/Core/Dxe/Mem/Page.c#L1973-L1989

So this is just junk unallocated memory that we are reporting as
a type it *could* be if an allocation occurs to minimize failures
of ExitBootServices. Which is questionable. But in terms of
attributes, I would expect we either have this unallocated
memory marked the same as the bin type or better, mark it RP
if we can (Taylor is making a change to set RP on free memory
by default, so we would have this in the page table, but we
would need to decide what we tell the OS).

Going back to the questionableness of this: can we not report
the entire bin as allocated memory to the OS? I understand
what the comment is saying; I don't like that we will lie to
the OS and reserve larger chunks of runtime memory than we
actually need and that will get the wrong permissions set on
it (for example making this garbage data be executable).

Thanks,
Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117994): https://edk2.groups.io/g/devel/message/117994
Mute This Topic: https://groups.io/mt/105477564/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2024-04-18 Thread Huang, Yanbo 
The PCD PcdPlatformEfiRtCodeMemorySize is used in 
https://github.com/tianocore/edk2-platforms/blob/master/Platform/Intel/MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.c
This PCD seems defined the size allocated for run time services code, and the 
similar PCD is PcdPlatformEfiRtDataMemorySize, seems defined the size allocated 
for run time services data.
I guess dandan means if the runtime services code size is small than the PCD 
defined, then the " Extra EfiRuntimeServicesCode regions which aren't part of 
loaded runtime images." size should be FixedPcdGet32 
(PcdPlatformEfiRtCodeMemorySize) - the actual runtime image code size?

Best Regards,
Yanbo Huang
-Original Message-
From: Ard Biesheuvel  
Sent: Thursday, April 18, 2024 9:18 PM
To: devel@edk2.groups.io; Bi, Dandan 
Cc: Taylor Beebe ; Huang, Yanbo 
; Wang, Jian J ; Gao, Liming 
; Zhou, Jianfeng 
Subject: Re: [edk2-devel] MdeModulePkg: Fix MAT SplitRecord() Logic introduce 
one bug and will cause SUT reset when boot to windows

Hello Dandan,

On Thu, 18 Apr 2024 at 15:03, Dandan Bi  wrote:
>
> Hi Taylor,
>
>
>
> >>Extra EfiRuntimeServicesCode regions which aren't part of loaded runtime 
> >>images.
>
> This may be related to the original size of EfiRuntimeServicesCode in memory 
> map, and the size can be configured via PcdPlatformEfiRtCodeMemorySize.
>

That PCD does not exist in EDK2. Can you explain what it does on your platform?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117992): https://edk2.groups.io/g/devel/message/117992
Mute This Topic: https://groups.io/mt/105477564/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2024-04-18 Thread Ard Biesheuvel 
Hello Dandan,

On Thu, 18 Apr 2024 at 15:03, Dandan Bi  wrote:
>
> Hi Taylor,
>
>
>
> >>Extra EfiRuntimeServicesCode regions which aren't part of loaded runtime 
> >>images.
>
> This may be related to the original size of EfiRuntimeServicesCode in memory 
> map, and the size can be configured via PcdPlatformEfiRtCodeMemorySize.
>

That PCD does not exist in EDK2. Can you explain what it does on your platform?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117990): https://edk2.groups.io/g/devel/message/117990
Mute This Topic: https://groups.io/mt/105477564/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2024-04-18 Thread Dandan Bi 
Hi Taylor,

>>Extra EfiRuntimeServicesCode regions which aren't part of loaded runtime 
>>images.
This may be related to the original size of EfiRuntimeServicesCode in memory 
map, and the size can be configured via PcdPlatformEfiRtCodeMemorySize.

If the size is large enough to hold all the runtime images and still has some 
remaining regions, then these regions are with EfiRuntimeServicesCode type and 
aren't part of loaded runtime images, right?
Pre-change 
https://github.com/tianocore/edk2/commit/e2f2bbe208b4c7ebcedacfc8333df1e52cbf07eb,
 such EfiRuntimeServicesCode regions are set with EFI_MEMORY_XP attribute.
Post-change 
https://github.com/tianocore/edk2/commit/e2f2bbe208b4c7ebcedacfc8333df1e52cbf07eb,
  these regions don’t have any access attribute.
And now this patch [PATCH v1] MdeModulePkg: Fixup MAT Attributes After 
Splitting EFI Memory Map 
(groups.io)<https://edk2.groups.io/g/devel/message/117889> set these regions 
with EFI_MEMORY_RO.
I am not sure which attribute should be set for these regions.  And old codes 
with EFI_MEMORY_XP attribute for a long time and seems not cause any issue.


Thanks,
Dandan
From: Taylor Beebe 
Sent: Thursday, April 18, 2024 7:54 AM
To: Huang, Yanbo ; devel@edk2.groups.io; Bi, Dandan 

Cc: Wang, Jian J ; Gao, Liming 
; Zhou, Jianfeng 
Subject: Re: [edk2-devel] MdeModulePkg: Fix MAT SplitRecord() Logic introduce 
one bug and will cause SUT reset when boot to windows


Hi Yanbo,
I didn't do it in the way you suggest for the same reason that the SplitTable() 
logic doesn't set attributes
on descriptors of type EfiRuntimeServicesData or other memory types. The 
purpose of the SplitTable() function
is to use the input image records to split descriptors so each image section 
has it's own descriptor. I think
it's reasonable to set the attributes on descriptors associated with images 
because those new descriptors are the
intended side effect of the function, but I don't think setting attributes on 
other descriptors is a good design pattern.
This pattern matches what's done in PiSmmCore/MemoryAttributesTable.c. Also, 
even if we did
it the way you suggest, we would still need call EnforceMemoryMapAttribute() 
later to set XP on the
EfiRuntimeServicesData descriptors.

Can you or Dandan explain the origin of the extra  EfiRuntimeServicesCode 
regions which aren't
part of loaded runtime images? It would be a good datapoint for our discussion 
on the proposed fix.

-Taylor
On 4/17/2024 7:04 AM, Huang, Yanbo wrote:
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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117989): https://edk2.groups.io/g/devel/message/117989
Mute This Topic: https://groups.io/mt/105477564/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2024-04-17 Thread Taylor Beebe 

Hi Yanbo,

I didn't do it in the way you suggest for the same reason that the 
SplitTable() logic doesn't set attributes
on descriptors of type EfiRuntimeServicesData or other memory types. The 
purpose of the SplitTable() function
is to use the input image records to split descriptors so each image 
section has it's own descriptor. I think
it's reasonable to set the attributes on descriptors associated with 
images because those new descriptors are the
intended side effect of the function, but I don't think setting 
attributes on other descriptors is a good design pattern.
This pattern matches what's done in PiSmmCore/MemoryAttributesTable.c. 
Also, even if we did
it the way you suggest, we would still need call 
EnforceMemoryMapAttribute() later to set XP on the

EfiRuntimeServicesData descriptors.

Can you or Dandan explain the origin of the extra EfiRuntimeServicesCode 
regions which aren't
part of loaded runtime images? It would be a good datapoint for our 
discussion on the proposed fix.


-Taylor
On 4/17/2024 7:04 AM, Huang, Yanbo wrote:


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




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117937): https://edk2.groups.io/g/devel/message/117937
Mute This Topic: https://groups.io/mt/105477564/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2024-04-17 Thread Huang, Yanbo 
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  On Behalf Of Taylor Beebe
Sent: Wednesday, April 17, 2024 10:33 AM
To: Bi, Dandan ; Huang, Yanbo ; 
devel@edk2.groups.io
Cc: Wang, Jian J ; Gao, Liming 
; Zhou, Jianfeng 
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 - 0x8000, 
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 - 0x769CF000

  VirtualStart  - 0x

  NumberOfPages - 0x0016

  Attribute - 0x8000

Entry (0x609E4298)

  Type  - 0x5

  PhysicalStart - 0x769E5000

  VirtualStart  - 0x

  NumberOfPages - 0x0001

  Attribute - 0x80004000

Entry (0x609E42C8)

  Type  - 0x5

  PhysicalStart - 0x769E6000

  VirtualStart  - 0x

  NumberOfPages - 0x0002

  Attribute - 0x8002



b. MAT without this patch:

Entry (0x609E4268)

  Type  - 0x5

  PhysicalStart - 0x769CF000

  VirtualStart  - 0x

  NumberOfPages - 0x0017

  Attribute - 0x80004000

Entry (0x609E4298)

  Type  - 0x5

  PhysicalStart - 0x769E6000

  VirtualStart  - 0x

  NumberOfPages - 0x0002

  Attribute - 0x8002



1. For example, when OldRecord in old memory map with:

Type - 0x0005

Attribute - 0x800F

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 
co

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

2024-04-16 Thread Taylor Beebe 

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) 


-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 - 0x8000, 
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 - 0x769CF000
   VirtualStart  - 0x
   NumberOfPages - 0x0016
   Attribute - 0x8000
Entry (0x609E4298)
   Type  - 0x5
   PhysicalStart - 0x769E5000
   VirtualStart  - 0x
   NumberOfPages - 0x0001
   Attribute - 0x80004000
Entry (0x609E42C8)
   Type  - 0x5
   PhysicalStart - 0x769E6000
   VirtualStart  - 0x
   NumberOfPages - 0x0002
   Attribute - 0x8002

b. MAT without this patch:
Entry (0x609E4268)
   Type  - 0x5
   PhysicalStart - 0x769CF000
   VirtualStart  - 0x
   NumberOfPages - 0x0017
   Attribute - 0x80004000
Entry (0x609E4298)
   Type  - 0x5
   PhysicalStart - 0x769E6000
   VirtualStart  - 0x
   NumberOfPages - 0x0002
   Attribute - 0x8002

1. For example, when OldRecord in old memory map with:
 Type - 0x0005
 Attribute - 0x800F
 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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2024-04-15 Thread Taylor Beebe 


On 4/15/2024 3:57 AM, Bi, Dandan wrote:

Hi Taylor,

With this patch, MAT contains some entries with Attribute - 0x8000, 
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 - 0x769CF000
   VirtualStart  - 0x
   NumberOfPages - 0x0016
   Attribute - 0x8000
Entry (0x609E4298)
   Type  - 0x5
   PhysicalStart - 0x769E5000
   VirtualStart  - 0x
   NumberOfPages - 0x0001
   Attribute - 0x80004000
Entry (0x609E42C8)
   Type  - 0x5
   PhysicalStart - 0x769E6000
   VirtualStart  - 0x
   NumberOfPages - 0x0002
   Attribute - 0x8002

b. MAT without this patch:
Entry (0x609E4268)
   Type  - 0x5
   PhysicalStart - 0x769CF000
   VirtualStart  - 0x
   NumberOfPages - 0x0017
   Attribute - 0x80004000
Entry (0x609E4298)
   Type  - 0x5
   PhysicalStart - 0x769E6000
   VirtualStart  - 0x
   NumberOfPages - 0x0002
   Attribute - 0x8002

1. For example, when OldRecord in old memory map with:
 Type - 0x0005
 Attribute - 0x800F
 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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2024-04-15 Thread Dandan Bi 
Hi Taylor,

With this patch, MAT contains some entries with Attribute - 0x8000, 
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 - 0x769CF000
  VirtualStart  - 0x
  NumberOfPages - 0x0016
  Attribute - 0x8000
Entry (0x609E4298)
  Type  - 0x5
  PhysicalStart - 0x769E5000
  VirtualStart  - 0x
  NumberOfPages - 0x0001
  Attribute - 0x80004000
Entry (0x609E42C8)
  Type  - 0x5
  PhysicalStart - 0x769E6000
  VirtualStart  - 0x
  NumberOfPages - 0x0002
  Attribute - 0x8002

b. MAT without this patch:
Entry (0x609E4268)
  Type  - 0x5
  PhysicalStart - 0x769CF000
  VirtualStart  - 0x
  NumberOfPages - 0x0017
  Attribute - 0x80004000
Entry (0x609E4298)
  Type  - 0x5
  PhysicalStart - 0x769E6000
  VirtualStart  - 0x
  NumberOfPages - 0x0002
  Attribute - 0x8002

1. For example, when OldRecord in old memory map with:
Type - 0x0005
Attribute - 0x800F
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  
Sent: Sunday, April 14, 2024 10:36 PM
To: Taylor Beebe ; devel@edk2.groups.io
Cc: Wang, Jian J ; Gao, Liming 
; Bi, Dandan ; Zhou, Jianfeng 

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 
Sent: Friday, April 12, 2024 11:10 PM
To: Huang, Yanbo ; devel@edk2.groups.io
Cc: Wang, Jian J ; Gao, Liming 
; Bi, Dandan ; Zhou, Jianfeng 

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.

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

2024-04-14 Thread Huang, Yanbo 
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  
Sent: Friday, April 12, 2024 11:10 PM
To: Huang, Yanbo ; devel@edk2.groups.io
Cc: Wang, Jian J ; Gao, Liming 
; Bi, Dandan ; Zhou, Jianfeng 

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.be...@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  On Behalf Of Taylor 
> Beebe
> Sent: Tuesday, November 28, 2023 2:18 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J ; Gao, Liming 
> ; Bi, Dandan 
> 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 
> Cc: Liming Gao 
> Cc: Dandan Bi 
> Signed-off-by: Taylor Beebe 
> Reviewed-by: Liming Gao 
> ---
>   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;
> UINTNNewRecordCount;
> UINTNTotalNewRecordCount;
> -  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 =

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

2024-04-12 Thread Taylor Beebe 

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.be...@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  On Behalf Of Taylor Beebe
Sent: Tuesday, November 28, 2023 2:18 AM
To: devel@edk2.groups.io
Cc: Wang, Jian J ; Gao, Liming ; Bi, 
Dandan 
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 
Cc: Liming Gao 
Cc: Dandan Bi 
Signed-off-by: Taylor Beebe 
Reviewed-by: Liming Gao 
---
  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;
UINTNNewRecordCount;
UINTNTotalNewRecordCount;
-  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 -

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

2024-04-11 Thread Huang, Yanbo 
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.be...@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  On Behalf Of Taylor Beebe
Sent: Tuesday, November 28, 2023 2:18 AM
To: devel@edk2.groups.io
Cc: Wang, Jian J ; Gao, Liming 
; Bi, Dandan 
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 
Cc: Liming Gao 
Cc: Dandan Bi 
Signed-off-by: Taylor Beebe 
Reviewed-by: Liming Gao 
---
 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;
   UINTNNewRecordCount;
   UINTNTotalNewRecordCount;
-  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;