On 16 March 2017 at 11:35, Ard Biesheuvel <[email protected]> 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 <[email protected]> > --- > 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 > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
