From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::242; helo=mail-it0-x242.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x242.google.com (mail-it0-x242.google.com [IPv6:2607:f8b0:4001:c0b::242]) (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 96A792219BC8F for ; Wed, 20 Dec 2017 13:26:17 -0800 (PST) Received: by mail-it0-x242.google.com with SMTP id x28so8372685ita.0 for ; Wed, 20 Dec 2017 13:31:05 -0800 (PST) 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=yG9OkQwpzVUt/fRS52myipATIcy2Pni+ij8r2gX+yp8=; b=AkwgDVIeZ2AQsfBCw4XYNP4vWbbyiEjqz8miHSKGCP62wJ2AYnAioQk1kzePmRH49x RyoeUe0fzgKFnuNoroNCyrmailtaCq9838l8At8AiUPUHqDVpiB4i4cpoPTpQfBemybv oi7jsvPf35l6TYw1iEozFSrFgawxrRXV/Hmf8= 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=yG9OkQwpzVUt/fRS52myipATIcy2Pni+ij8r2gX+yp8=; b=KxzixCx4K+tXvk0yuspVNcavNaAodB6TPei/vo3n8JlKf1HuXjv/mpW0fx5ctl4WGC g6p4FbmZGoKbqRDzZOhTPyztGAyH1ixHciujh4IcXxh529pvN8hc18FlEWlk2NEx1+b9 0gOdpsHnzPq9kpY23VLIlkfvcakMz/WUXUlV29stFEFOkV5ZtF6CO8sfyUUvdl3MEj/D HSG8vn2MhPDOloDR4r9cAJBeecmp9wvls64xTwmYJlsAG7G3aCZIk+qanbBrhmedfOyy AodvFt+t6XnJN0s6wYZCSdD1+T2nnAyzZ2ceUMBNlPdYssAGtxfbBx7q6BNDqAJ0VDqj spUw== X-Gm-Message-State: AKGB3mJjdTtWZ6sn4AVhSjuy3uGRvm6pMyw5iv4UfjNlvFFjnQ/WQD/J g8He70ya/XVk41WroIgc5IxcjCcTvvig4R8ntUvQ9A== X-Google-Smtp-Source: ACJfBosZpp+EshinyFfFMEjeUWZsfeWnh7/ho5FnBE4lNfqSkjRLY5zfpDzLAJOz986ilEGRCnogW8ibL0nNIyutB8U= X-Received: by 10.36.55.138 with SMTP id r132mr9885553itr.34.1513805464810; Wed, 20 Dec 2017 13:31:04 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.52.14 with HTTP; Wed, 20 Dec 2017 13:31:04 -0800 (PST) In-Reply-To: <20171220192858.1901-1-sigmaepsilon92@gmail.com> References: <20171220192858.1901-1-sigmaepsilon92@gmail.com> From: Ard Biesheuvel Date: Wed, 20 Dec 2017 21:31:04 +0000 Message-ID: To: M1cha Cc: "edk2-devel@lists.01.org" , Leif Lindholm Subject: Re: [PATCH v2] ArmPkg/ArmMmuLib ARM: fix page size granularity in initial MMU setting X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 Dec 2017 21:26:18 -0000 Content-Type: text/plain; charset="UTF-8" On 20 December 2017 at 19:28, M1cha wrote: > From what I can see this bug dates back to the commit from 2011 where > support for this was added: 2cf4b60895f8a > > The first problem is that PopulateLevel2PageTable overflows the > translation table buffer because it doesn't verify that the size > actually fits within one level 2 page table. > > The second problem is that the loop in FillTranslationTable doesn't > care about the PhysicalBase or the RemainLength and always substracts > one section size from RemainLength. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Michael Zimmermann > --- > > v2: implement changes as requested by Ard. > Thanks for the fix, and thanks for reworking it. Reviewed-by: Ard Biesheuvel Pushed as 889c7ca1b5e8 > ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 34 ++++++++++---------- > 1 file changed, 17 insertions(+), 17 deletions(-) > > diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c > index b02f6d7fc590..8f85cf678c98 100644 > --- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c > +++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c > @@ -128,6 +128,7 @@ PopulateLevel2PageTable ( > UINT32 SectionDescriptor; > UINT32 TranslationTable; > UINT32 BaseSectionAddress; > + UINT32 FirstPageOffset; > > switch (Attributes) { > case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK: > @@ -199,9 +200,12 @@ PopulateLevel2PageTable ( > TT_DESCRIPTOR_SECTION_TYPE_PAGE_TABLE; > } > > - PageEntry = ((UINT32 *)(TranslationTable) + ((PhysicalBase & TT_DESCRIPTOR_PAGE_INDEX_MASK) >> TT_DESCRIPTOR_PAGE_BASE_SHIFT)); > + FirstPageOffset = ((PhysicalBase & TT_DESCRIPTOR_PAGE_INDEX_MASK) >> TT_DESCRIPTOR_PAGE_BASE_SHIFT); > + PageEntry = ((UINT32 *)(TranslationTable) + FirstPageOffset); > Pages = RemainLength / TT_DESCRIPTOR_PAGE_SIZE; > > + ASSERT (FirstPageOffset + Pages <= TRANSLATION_TABLE_PAGE_COUNT); > + > for (Index = 0; Index < Pages; Index++) { > *PageEntry++ = TT_DESCRIPTOR_PAGE_BASE_ADDRESS(PhysicalBase) | PageAttributes; > PhysicalBase += TT_DESCRIPTOR_PAGE_SIZE; > @@ -220,6 +224,7 @@ FillTranslationTable ( > UINT32 Attributes; > UINT32 PhysicalBase; > UINT64 RemainLength; > + UINT32 PageMapLength; > > ASSERT(MemoryRegion->Length > 0); > > @@ -268,30 +273,25 @@ FillTranslationTable ( > SectionEntry = TRANSLATION_TABLE_ENTRY_FOR_VIRTUAL_ADDRESS(TranslationTable, MemoryRegion->VirtualBase); > > while (RemainLength != 0) { > - if (PhysicalBase % TT_DESCRIPTOR_SECTION_SIZE == 0) { > - if (RemainLength >= TT_DESCRIPTOR_SECTION_SIZE) { > - // Case: Physical address aligned on the Section Size (1MB) && the length is greater than the Section Size > - *SectionEntry++ = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(PhysicalBase) | Attributes; > - PhysicalBase += TT_DESCRIPTOR_SECTION_SIZE; > - } else { > - // Case: Physical address aligned on the Section Size (1MB) && the length does not fill a section > - PopulateLevel2PageTable (SectionEntry++, PhysicalBase, RemainLength, MemoryRegion->Attributes); > - > - // It must be the last entry > - break; > - } > + if ((PhysicalBase % TT_DESCRIPTOR_SECTION_SIZE == 0) && RemainLength >= TT_DESCRIPTOR_SECTION_SIZE) { > + // Case: Physical address aligned on the Section Size (1MB) && the length is greater than the Section Size > + *SectionEntry++ = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(PhysicalBase) | Attributes; > + PhysicalBase += TT_DESCRIPTOR_SECTION_SIZE; > + RemainLength -= TT_DESCRIPTOR_SECTION_SIZE; > } else { > + PageMapLength = MIN (RemainLength, TT_DESCRIPTOR_SECTION_SIZE) - (PhysicalBase % TT_DESCRIPTOR_SECTION_SIZE); > + > // Case: Physical address NOT aligned on the Section Size (1MB) > - PopulateLevel2PageTable (SectionEntry++, PhysicalBase, RemainLength, MemoryRegion->Attributes); > - // Aligned the address > - PhysicalBase = (PhysicalBase + TT_DESCRIPTOR_SECTION_SIZE) & ~(TT_DESCRIPTOR_SECTION_SIZE-1); > + PopulateLevel2PageTable (SectionEntry++, PhysicalBase, PageMapLength, MemoryRegion->Attributes); > > // If it is the last entry > if (RemainLength < TT_DESCRIPTOR_SECTION_SIZE) { > break; > } > + > + PhysicalBase += PageMapLength; > + RemainLength -= PageMapLength; > } > - RemainLength -= TT_DESCRIPTOR_SECTION_SIZE; > } > } > > -- > 2.15.1 >