From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x230.google.com (mail-it0-x230.google.com [IPv6:2607:f8b0:4001:c0b::230]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2BCAF80329 for ; Thu, 16 Mar 2017 06:19:28 -0700 (PDT) Received: by mail-it0-x230.google.com with SMTP id m27so42553234iti.1 for ; Thu, 16 Mar 2017 06:19:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=kP2rAOGnrsThpMhRs6/6A3gYV3wzQ/mWlEtIbX6V1mE=; b=HPZTEHMcxDPCA2YWgYXMui1GCVwtPp3d30G0AIB0Zpjc+HDdUPGthB57W+42ruoQro C4L/jiqN6UuU+j5g+sxNmvpNURvXCh2Q6gTG03RrSOIasYQTBQJBFVesq9zfaGslWSGy MJVaOCLY/NekEOdHZto78bxFn8HVz/jZePKAA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=kP2rAOGnrsThpMhRs6/6A3gYV3wzQ/mWlEtIbX6V1mE=; b=M7W3G/BC3zSijC2QOxv4NrvvrDflgEqpDl6SlkW/EWhuGV7z/LYyA4NsyHVHXudDhi SoVNxn6MuYPP3YmeufX6eT/p66EKl4b9Y0n5WcilkPhDX4NH8M6KZ2B8ro6IOSwdA4aM U8b4pFOp59Ds9UWkcQsOP+Sed8BiI1Go0Xtb+O3d9k07fDcMbLu9Pt4UFfvAigDwWTuX 7CUPmhiUeq6SxGIqdqOpg+c2L5MC3yxkIHeQV5l15Q+vzjw+sRhbSAzXuaRj9IAMX2Zz GQHcMHq9A5auz/tnq+2XG810mH2H+U2MVlvfzxRTuq90DWXRYgAmq9pxSn+LDZJaX1Ni S+HQ== X-Gm-Message-State: AFeK/H27FFfbNLcPEtshsxs8fZQtVttFM//mtRiCC9Zy7WAPdQ3uFlLJV2PpSgflNN9e5EeLlv9ThPLowl/q5JwD X-Received: by 10.107.132.155 with SMTP id o27mr9417859ioi.138.1489670366557; Thu, 16 Mar 2017 06:19:26 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Thu, 16 Mar 2017 06:19:26 -0700 (PDT) In-Reply-To: <1489664128-30700-1-git-send-email-ard.biesheuvel@linaro.org> References: <1489664128-30700-1-git-send-email-ard.biesheuvel@linaro.org> From: Ard Biesheuvel Date: Thu, 16 Mar 2017 13:19:26 +0000 Message-ID: To: "edk2-devel@lists.01.org" Cc: Leif Lindholm , "Cohen, Eugene" , "Tian, Feng" , "Zeng, Star" , Ard Biesheuvel Subject: Re: [PATCH] MdeModulePkg/DxeCore: deal with allocations spanning several memmap entries X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 16 Mar 2017 13:19:28 -0000 Content-Type: text/plain; charset=UTF-8 On 16 March 2017 at 11:35, Ard Biesheuvel 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 > --- > 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 >