1 file changed, 30 insertions(+), 23 deletions(-)
Update check for enough space to occur prior to alignment offset
modification. This prevents a case where EfiFreeMemoryTop could be
less than EfiFreeMemoryBottom
Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
---
.../MemoryAllocationLib.c | 53 +++++++++++--------
1 file changed, 30 insertions(+), 23 deletions(-)
diff --git a/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c b/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
index 2cc2a71121..9208826565 100644
--- a/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
+++ b/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
@@ -27,37 +27,44 @@ InternalAllocatePages (
Hob.Raw = GetHobList ();
- // Check to see if on 4k boundary
Offset = Hob.HandoffInformationTable->EfiFreeMemoryTop & 0xFFF;
+ //
+ // Verify that there is sufficient memory to satisfy the allocation and padding prior to updating anything
+ //
+ if ((Hob.HandoffInformationTable->EfiFreeMemoryTop - ((Pages * EFI_PAGE_SIZE) + sizeof (EFI_HOB_MEMORY_ALLOCATION)) - Offset) < Hob.HandoffInformationTable->EfiFreeMemoryBottom) {
+ if (Offset != 0) {
+ DEBUG ((DEBUG_ERROR, "Offset applied without enough space\r\n"));
+ } else {
+ DEBUG ((DEBUG_ERROR, "Out of memory\r\n"));
+ }
+
+ ASSERT (FALSE);
+ return 0;
+ }
+
+ // Check to see if on 4k boundary
if (Offset != 0) {
// If not aligned, make the allocation aligned.
Hob.HandoffInformationTable->EfiFreeMemoryTop -= Offset;
}
//
- // Verify that there is sufficient memory to satisfy the allocation
+ // Update the PHIT to reflect the memory usage
//
- if (Hob.HandoffInformationTable->EfiFreeMemoryTop - ((Pages * EFI_PAGE_SIZE) + sizeof (EFI_HOB_MEMORY_ALLOCATION)) < Hob.HandoffInformationTable->EfiFreeMemoryBottom) {
- return 0;
- } else {
- //
- // Update the PHIT to reflect the memory usage
- //
- Hob.HandoffInformationTable->EfiFreeMemoryTop -= Pages * EFI_PAGE_SIZE;
-
- // This routine used to create a memory allocation HOB a la PEI, but that's not
- // necessary for us.
-
- //
- // Create a memory allocation HOB.
- //
- BuildMemoryAllocationHob (
- Hob.HandoffInformationTable->EfiFreeMemoryTop,
- Pages * EFI_PAGE_SIZE,
- MemoryType
- );
- return (VOID *)(UINTN)Hob.HandoffInformationTable->EfiFreeMemoryTop;
- }
+ Hob.HandoffInformationTable->EfiFreeMemoryTop -= Pages * EFI_PAGE_SIZE;
+
+ // This routine used to create a memory allocation HOB a la PEI, but that's not
+ // necessary for us.
+
+ //
+ // Create a memory allocation HOB.
+ //
+ BuildMemoryAllocationHob (
+ Hob.HandoffInformationTable->EfiFreeMemoryTop,
+ Pages * EFI_PAGE_SIZE,
+ MemoryType
+ );
+ return (VOID *)(UINTN)Hob.HandoffInformationTable->EfiFreeMemoryTop;
}
/**
--
2.25.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#93448): https://edk2.groups.io/g/devel/message/93448
Mute This Topic: https://groups.io/mt/93527827/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-On Wed, 7 Sept 2022 at 17:46, Jeff Brasen <jbrasen@nvidia.com> wrote:
>
> Update check for enough space to occur prior to alignment offset
> modification. This prevents a case where EfiFreeMemoryTop could be
> less than EfiFreeMemoryBottom
>
> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
Thanks for the respin. I have caught up in the mean time.
> ---
> .../MemoryAllocationLib.c | 53 +++++++++++--------
> 1 file changed, 30 insertions(+), 23 deletions(-)
>
> diff --git a/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c b/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
> index 2cc2a71121..9208826565 100644
> --- a/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
> +++ b/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
> @@ -27,37 +27,44 @@ InternalAllocatePages (
>
> Hob.Raw = GetHobList ();
>
> - // Check to see if on 4k boundary
> Offset = Hob.HandoffInformationTable->EfiFreeMemoryTop & 0xFFF;
> + //
> + // Verify that there is sufficient memory to satisfy the allocation and padding prior to updating anything
> + //
> + if ((Hob.HandoffInformationTable->EfiFreeMemoryTop - ((Pages * EFI_PAGE_SIZE) + sizeof (EFI_HOB_MEMORY_ALLOCATION)) - Offset) < Hob.HandoffInformationTable->EfiFreeMemoryBottom) {
> + if (Offset != 0) {
> + DEBUG ((DEBUG_ERROR, "Offset applied without enough space\r\n"));
> + } else {
> + DEBUG ((DEBUG_ERROR, "Out of memory\r\n"));
> + }
> +
> + ASSERT (FALSE);
> + return 0;
> + }
> +
> + // Check to see if on 4k boundary
> if (Offset != 0) {
> // If not aligned, make the allocation aligned.
> Hob.HandoffInformationTable->EfiFreeMemoryTop -= Offset;
> }
>
I understand how you are trying to stick with the original code as
much as possible, but this is all extremely clunky, and I'd prefer we
just clean it up, if you don't mind.
Would something like the below work for you as well?
diff --git a/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
b/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
index 2cc2a7112197..547117dc13d6 100644
--- a/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
+++ b/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
@@ -23,41 +23,36 @@ InternalAllocatePages (
)
{
EFI_PEI_HOB_POINTERS Hob;
- EFI_PHYSICAL_ADDRESS Offset;
+ EFI_PHYSICAL_ADDRESS NewTop;
Hob.Raw = GetHobList ();
- // Check to see if on 4k boundary
- Offset = Hob.HandoffInformationTable->EfiFreeMemoryTop & 0xFFF;
- if (Offset != 0) {
- // If not aligned, make the allocation aligned.
- Hob.HandoffInformationTable->EfiFreeMemoryTop -= Offset;
- }
+ NewTop = Hob.HandoffInformationTable->EfiFreeMemoryTop &
~(EFI_PHYSICAL_ADDRESS)EFI_PAGE_MASK;
+ NewTop -= Pages * EFI_PAGE_SIZE;
//
// Verify that there is sufficient memory to satisfy the allocation
//
- if (Hob.HandoffInformationTable->EfiFreeMemoryTop - ((Pages *
EFI_PAGE_SIZE) + sizeof (EFI_HOB_MEMORY_ALLOCATION)) <
Hob.HandoffInformationTable->EfiFreeMemoryBottom) {
- return 0;
- } else {
- //
- // Update the PHIT to reflect the memory usage
- //
- Hob.HandoffInformationTable->EfiFreeMemoryTop -= Pages * EFI_PAGE_SIZE;
-
- // This routine used to create a memory allocation HOB a la PEI,
but that's not
- // necessary for us.
-
- //
- // Create a memory allocation HOB.
- //
- BuildMemoryAllocationHob (
- Hob.HandoffInformationTable->EfiFreeMemoryTop,
- Pages * EFI_PAGE_SIZE,
- MemoryType
- );
- return (VOID *)(UINTN)Hob.HandoffInformationTable->EfiFreeMemoryTop;
+ if (NewTop < (Hob.HandoffInformationTable->EfiFreeMemoryBottom +
sizeof (EFI_HOB_MEMORY_ALLOCATION)))
+ {
+ return NULL;
}
+
+ //
+ // Update the PHIT to reflect the memory usage
+ //
+ Hob.HandoffInformationTable->EfiFreeMemoryTop = NewTop;
+
+ //
+ // Create a memory allocation HOB.
+ //
+ BuildMemoryAllocationHob (
+ Hob.HandoffInformationTable->EfiFreeMemoryTop,
+ Pages * EFI_PAGE_SIZE,
+ MemoryType
+ );
+
+ return (VOID *)(UINTN)Hob.HandoffInformationTable->EfiFreeMemoryTop;
}
/**
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#93451): https://edk2.groups.io/g/devel/message/93451
Mute This Topic: https://groups.io/mt/93527827/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Wednesday, September 7, 2022 10:16 AM
> To: Jeff Brasen <jbrasen@nvidia.com>
> Cc: devel@edk2.groups.io; ardb+tianocore@kernel.org;
> abner.chang@amd.com; git@danielschaefer.me; quic_llindhol@quicinc.com
> Subject: Re: [PATCH v2] EmbeddedPkg/PrePiMemoryAllocationLib: Check for
> space on offset allocation
>
> External email: Use caution opening links or attachments
>
>
> On Wed, 7 Sept 2022 at 17:46, Jeff Brasen <jbrasen@nvidia.com> wrote:
> >
> > Update check for enough space to occur prior to alignment offset
> > modification. This prevents a case where EfiFreeMemoryTop could be
> > less than EfiFreeMemoryBottom
> >
> > Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
>
> Thanks for the respin. I have caught up in the mean time.
>
>
> > ---
> > .../MemoryAllocationLib.c | 53 +++++++++++--------
> > 1 file changed, 30 insertions(+), 23 deletions(-)
> >
> > diff --git
> >
> a/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
> >
> b/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
> > index 2cc2a71121..9208826565 100644
> > ---
> >
> a/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
> > +++
> b/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib
> > +++ .c
> > @@ -27,37 +27,44 @@ InternalAllocatePages (
> >
> > Hob.Raw = GetHobList ();
> >
> > - // Check to see if on 4k boundary
> > Offset = Hob.HandoffInformationTable->EfiFreeMemoryTop & 0xFFF;
> > + //
> > + // Verify that there is sufficient memory to satisfy the allocation
> > + and padding prior to updating anything // if
> > + ((Hob.HandoffInformationTable->EfiFreeMemoryTop - ((Pages *
> EFI_PAGE_SIZE) + sizeof (EFI_HOB_MEMORY_ALLOCATION)) - Offset) <
> Hob.HandoffInformationTable->EfiFreeMemoryBottom) {
> > + if (Offset != 0) {
> > + DEBUG ((DEBUG_ERROR, "Offset applied without enough
> space\r\n"));
> > + } else {
> > + DEBUG ((DEBUG_ERROR, "Out of memory\r\n"));
> > + }
> > +
> > + ASSERT (FALSE);
> > + return 0;
> > + }
> > +
> > + // Check to see if on 4k boundary
> > if (Offset != 0) {
> > // If not aligned, make the allocation aligned.
> > Hob.HandoffInformationTable->EfiFreeMemoryTop -= Offset;
> > }
> >
>
> I understand how you are trying to stick with the original code as much as
> possible, but this is all extremely clunky, and I'd prefer we just clean it up, if
> you don't mind.
>
> Would something like the below work for you as well?
Yup this looks good to me and cleaner as well.
>
>
>
> diff --git
> a/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
> b/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
> index 2cc2a7112197..547117dc13d6 100644
> ---
> a/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
> +++
> b/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
> @@ -23,41 +23,36 @@ InternalAllocatePages (
> )
> {
> EFI_PEI_HOB_POINTERS Hob;
> - EFI_PHYSICAL_ADDRESS Offset;
> + EFI_PHYSICAL_ADDRESS NewTop;
>
> Hob.Raw = GetHobList ();
>
> - // Check to see if on 4k boundary
> - Offset = Hob.HandoffInformationTable->EfiFreeMemoryTop & 0xFFF;
> - if (Offset != 0) {
> - // If not aligned, make the allocation aligned.
> - Hob.HandoffInformationTable->EfiFreeMemoryTop -= Offset;
> - }
> + NewTop = Hob.HandoffInformationTable->EfiFreeMemoryTop &
> ~(EFI_PHYSICAL_ADDRESS)EFI_PAGE_MASK;
> + NewTop -= Pages * EFI_PAGE_SIZE;
>
> //
> // Verify that there is sufficient memory to satisfy the allocation
> //
> - if (Hob.HandoffInformationTable->EfiFreeMemoryTop - ((Pages *
> EFI_PAGE_SIZE) + sizeof (EFI_HOB_MEMORY_ALLOCATION)) <
> Hob.HandoffInformationTable->EfiFreeMemoryBottom) {
> - return 0;
> - } else {
> - //
> - // Update the PHIT to reflect the memory usage
> - //
> - Hob.HandoffInformationTable->EfiFreeMemoryTop -= Pages *
> EFI_PAGE_SIZE;
> -
> - // This routine used to create a memory allocation HOB a la PEI,
> but that's not
> - // necessary for us.
> -
> - //
> - // Create a memory allocation HOB.
> - //
> - BuildMemoryAllocationHob (
> - Hob.HandoffInformationTable->EfiFreeMemoryTop,
> - Pages * EFI_PAGE_SIZE,
> - MemoryType
> - );
> - return (VOID *)(UINTN)Hob.HandoffInformationTable-
> >EfiFreeMemoryTop;
> + if (NewTop < (Hob.HandoffInformationTable->EfiFreeMemoryBottom +
> sizeof (EFI_HOB_MEMORY_ALLOCATION)))
> + {
> + return NULL;
> }
> +
> + //
> + // Update the PHIT to reflect the memory usage //
> + Hob.HandoffInformationTable->EfiFreeMemoryTop = NewTop;
> +
> + //
> + // Create a memory allocation HOB.
> + //
> + BuildMemoryAllocationHob (
> + Hob.HandoffInformationTable->EfiFreeMemoryTop,
> + Pages * EFI_PAGE_SIZE,
> + MemoryType
> + );
> +
> + return (VOID *)(UINTN)Hob.HandoffInformationTable-
> >EfiFreeMemoryTop;
> }
>
> /**
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#93456): https://edk2.groups.io/g/devel/message/93456
Mute This Topic: https://groups.io/mt/93527827/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.