* [PATCH v2] EmbeddedPkg/PrePiMemoryAllocationLib: Check for space on offset allocation
@ 2022-09-07 15:45 Jeff Brasen
2022-09-07 16:16 ` Ard Biesheuvel
0 siblings, 1 reply; 3+ messages in thread
From: Jeff Brasen @ 2022-09-07 15:45 UTC (permalink / raw)
To: devel; +Cc: ardb+tianocore, abner.chang, git, quic_llindhol, Jeff Brasen
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] EmbeddedPkg/PrePiMemoryAllocationLib: Check for space on offset allocation
2022-09-07 15:45 [PATCH v2] EmbeddedPkg/PrePiMemoryAllocationLib: Check for space on offset allocation Jeff Brasen
@ 2022-09-07 16:16 ` Ard Biesheuvel
2022-09-07 20:56 ` Jeff Brasen
0 siblings, 1 reply; 3+ messages in thread
From: Ard Biesheuvel @ 2022-09-07 16:16 UTC (permalink / raw)
To: Jeff Brasen; +Cc: devel, ardb+tianocore, abner.chang, git, quic_llindhol
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;
}
/**
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] EmbeddedPkg/PrePiMemoryAllocationLib: Check for space on offset allocation
2022-09-07 16:16 ` Ard Biesheuvel
@ 2022-09-07 20:56 ` Jeff Brasen
0 siblings, 0 replies; 3+ messages in thread
From: Jeff Brasen @ 2022-09-07 20:56 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: devel@edk2.groups.io, ardb+tianocore@kernel.org,
abner.chang@amd.com, git@danielschaefer.me,
quic_llindhol@quicinc.com
> -----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;
> }
>
> /**
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-09-07 20:56 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-09-07 15:45 [PATCH v2] EmbeddedPkg/PrePiMemoryAllocationLib: Check for space on offset allocation Jeff Brasen
2022-09-07 16:16 ` Ard Biesheuvel
2022-09-07 20:56 ` Jeff Brasen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox