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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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
>