Re: [edk2-devel] [PATCH] MdeModulePkg/Gcd: Check memory allocation when initializing memory

["Laszlo Ersek" <lersek@redhat.com>]

Hi Jeff,

On 10/24/20 00:10, Jeff Brasen wrote:
> CoreInitializeMemoryServices was not checking for any existing memory
> allocation created in the HOB producer phase. If there are memory
> allocations outside of the region covered by the HOB List then Gcd could
> select that region for memory which can result in the memory allocation
> to not be handled and memory overwrites.
> 
> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> ---
>  MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 60 +++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)

can you please run "BaseTools/Scripts/SetupGit.py" in your edk2 clone?

Thanks,
Laszlo

> 
> diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> index 2d8c076f71..4a22ee96b7 100644
> --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> @@ -2097,6 +2097,62 @@ CalculateTotalMemoryBinSizeNeeded (
>    return TotalSize;
> 
>  }
> 
>  
> 
> +/**
> 
> +   Find the largest region in the specified region that is not covered by an existing memory allocation
> 
> +
> 
> +   @param BaseAddress   On input start of the region to check.
> 
> +                        On output start of the largest free region.
> 
> +   @param Length        On input size of region to check.
> 
> +                        On output size of the largest free region.
> 
> +   @param MemoryHob     Hob pointer for the first memory allocation pointer to check
> 
> +**/
> 
> +VOID
> 
> +FindLargestFreeRegion (
> 
> +    IN OUT EFI_PHYSICAL_ADDRESS  *BaseAddress,
> 
> +    IN OUT UINT64                *Length,
> 
> +    IN EFI_HOB_MEMORY_ALLOCATION *MemoryHob
> 
> +    )
> 
> +{
> 
> +  EFI_PHYSICAL_ADDRESS TopAddress;
> 
> +
> 
> +  TopAddress = *BaseAddress + *Length;
> 
> +  while (MemoryHob != NULL) {
> 
> +    EFI_PHYSICAL_ADDRESS AllocatedTop;
> 
> +
> 
> +    AllocatedTop = MemoryHob->AllocDescriptor.MemoryBaseAddress + MemoryHob->AllocDescriptor.MemoryLength;
> 
> +
> 
> +    if ((MemoryHob->AllocDescriptor.MemoryBaseAddress >= *BaseAddress) &&
> 
> +        (AllocatedTop <= TopAddress)) {
> 
> +      EFI_PHYSICAL_ADDRESS LowerBase;
> 
> +      UINT64               LowerSize;
> 
> +      EFI_PHYSICAL_ADDRESS UpperBase;
> 
> +      UINT64               UpperSize;
> 
> +
> 
> +      LowerBase = *BaseAddress;
> 
> +      LowerSize = MemoryHob->AllocDescriptor.MemoryBaseAddress - *BaseAddress;
> 
> +      UpperBase = AllocatedTop;
> 
> +      UpperSize = TopAddress - AllocatedTop;
> 
> +
> 
> +      if (LowerSize != 0) {
> 
> +        FindLargestFreeRegion (&LowerBase, &LowerSize, (EFI_HOB_MEMORY_ALLOCATION *) GetNextHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, GET_NEXT_HOB (MemoryHob)));
> 
> +      }
> 
> +      if (UpperSize != 0) {
> 
> +        FindLargestFreeRegion (&UpperBase, &UpperSize, (EFI_HOB_MEMORY_ALLOCATION *) GetNextHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, GET_NEXT_HOB (MemoryHob)));
> 
> +      }
> 
> +
> 
> +      if (UpperSize >= LowerSize) {
> 
> +        *Length = UpperSize;
> 
> +        *BaseAddress = UpperBase;
> 
> +      } else {
> 
> +        *Length = LowerSize;
> 
> +        *BaseAddress = LowerBase;
> 
> +      }
> 
> +      return;
> 
> +    }
> 
> +    MemoryHob = GetNextHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, GET_NEXT_HOB (MemoryHob));
> 
> +  }
> 
> +}
> 
> +
> 
>  /**
> 
>    External function. Initializes memory services based on the memory
> 
>    descriptor HOBs.  This function is responsible for priming the memory
> 
> @@ -2235,6 +2291,7 @@ CoreInitializeMemoryServices (
>      Attributes  = PhitResourceHob->ResourceAttribute;
> 
>      BaseAddress = PageAlignAddress (PhitHob->EfiMemoryTop);
> 
>      Length      = PageAlignLength  (ResourceHob->PhysicalStart + ResourceHob->ResourceLength - BaseAddress);
> 
> +    FindLargestFreeRegion (&BaseAddress, &Length, (EFI_HOB_MEMORY_ALLOCATION *)GetFirstHob (EFI_HOB_TYPE_MEMORY_ALLOCATION));
> 
>      if (Length < MinimalMemorySizeNeeded) {
> 
>        //
> 
>        // If that range is not large enough to intialize the DXE Core, then
> 
> @@ -2242,6 +2299,7 @@ CoreInitializeMemoryServices (
>        //
> 
>        BaseAddress = PageAlignAddress (PhitHob->EfiFreeMemoryBottom);
> 
>        Length      = PageAlignLength  (PhitHob->EfiFreeMemoryTop - BaseAddress);
> 
> +      //This region is required to have no memory allocation inside it, skip check for entries in HOB List
> 
>        if (Length < MinimalMemorySizeNeeded) {
> 
>          //
> 
>          // If that range is not large enough to intialize the DXE Core, then
> 
> @@ -2249,6 +2307,7 @@ CoreInitializeMemoryServices (
>          //
> 
>          BaseAddress = PageAlignAddress (ResourceHob->PhysicalStart);
> 
>          Length      = PageAlignLength  ((UINT64)((UINTN)*HobStart - BaseAddress));
> 
> +        FindLargestFreeRegion (&BaseAddress, &Length, (EFI_HOB_MEMORY_ALLOCATION *)GetFirstHob (EFI_HOB_TYPE_MEMORY_ALLOCATION));
> 
>        }
> 
>      }
> 
>      break;
> 
> @@ -2312,6 +2371,7 @@ CoreInitializeMemoryServices (
>        //
> 
>        TestedMemoryBaseAddress = PageAlignAddress (ResourceHob->PhysicalStart);
> 
>        TestedMemoryLength      = PageAlignLength  (ResourceHob->PhysicalStart + ResourceHob->ResourceLength - TestedMemoryBaseAddress);
> 
> +      FindLargestFreeRegion (&TestedMemoryBaseAddress, &TestedMemoryLength, (EFI_HOB_MEMORY_ALLOCATION *)GetFirstHob (EFI_HOB_TYPE_MEMORY_ALLOCATION));
> 
>        if (TestedMemoryLength < MinimalMemorySizeNeeded) {
> 
>          continue;
> 
>        }
>