Re: [PATCH] MdeModulePkg/DxeCore: deal with allocations spanning several memmap entries

["Zeng, Star" <star.zeng@intel.com>]

Ard,

In fact the first half should be like below if it needs to be there, right?
Entry->Start > End   =>   Entry->Start > (End + 1)

Anyway, I agree the comments "The first half of the expression can be dropped" you supplemented.
With the first half of the expression dropped, Reviewed-by: Star Zeng <star.zeng@intel.com>

Thanks,
Star
-----Original Message-----
From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] 
Sent: Thursday, March 16, 2017 9:19 PM
To: edk2-devel@lists.01.org
Cc: Leif Lindholm <leif.lindholm@linaro.org>; Cohen, Eugene <eugene@hp.com>; Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [PATCH] MdeModulePkg/DxeCore: deal with allocations spanning several memmap entries

On 16 March 2017 at 11:35, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> When attempting to perform page allocations using AllocateAddress, we 
> fail to check whether the entire region is free before splitting the 
> region. This may lead to memory being leaked further into the routine, 
> when it turns out that one of the memory map entries intersected by 
> the region is already occupied. In this case, prior conversions are 
> not rolled back.
>
> For instance, starting from this situation
>
> 0x000040000000-0x00004007ffff [ConventionalMemory ]
> 0x000040080000-0x00004009ffff [Boot Data          ]
> 0x0000400a0000-0x000047ffffff [ConventionalMemory ]
>
> a failed EfiLoaderData allocation @ 0x40000000 that covers the 
> BootData region will fail, but leave the first part of the allocation 
> converted, so we end up with
>
> 0x000040000000-0x00004007ffff [Loader Data        ]
> 0x000040080000-0x00004009ffff [Boot Data          ]
> 0x0000400a0000-0x000047ffffff [ConventionalMemory ]
>
> even though the AllocatePages() call returned an error.
>
> So let's check beforehand that AllocateAddress allocations are covered 
> by a single memory map entry, so that it either succeeds or fails 
> completely, rather than leaking allocations.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  MdeModulePkg/Core/Dxe/Mem/Page.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c 
> b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index 260a30a214c7..92306b2f1b45 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -755,6 +755,17 @@ CoreConvertPagesEx (
>      }
>
>      //
> +    // If we are converting the type of the range from EfiConventionalMemory to
> +    // another type, we have to ensure that the entire range is covered by a
> +    // single entry.
> +    //
> +    if (ChangingType && (NewType != EfiConventionalMemory)) {
> +      if (Entry->Start > End || Entry->End < End) {

I guess this expression is slightly bogus: we know entry [Entry->Start, Entry->End) covers Start, and so the only way the entry could fail to cover [Start, End) is when Entry->End < End. The first half of the expression can be dropped since it can never be true


> +        DEBUG ((DEBUG_ERROR | DEBUG_PAGE, "ConvertPages: range %lx - %lx covers multiple entries\n", Start, End));
> +        return EFI_NOT_FOUND;
> +      }
> +    }
> +    //
>      // Convert range to the end, or to the end of the descriptor
>      // if that's all we've got
>      //
> --
> 2.7.4
>