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::241; helo=mail-it0-x241.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x241.google.com (mail-it0-x241.google.com [IPv6:2607:f8b0:4001:c0b::241]) (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 0667822637DDB for ; Thu, 19 Apr 2018 05:30:13 -0700 (PDT) Received: by mail-it0-x241.google.com with SMTP id l83-v6so5680198ita.5 for ; Thu, 19 Apr 2018 05:30:13 -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=Fg8UDD6e4x+n15D9+cpQ+V2P6iSDoCU4cPj5Bf52gls=; b=AJqT9JK6vTMbKbY4+5VGzDBTv044nubtoM2PNQtOWuUbCbh8F5GalM4ihmZcBYrQU0 AgpzrRDUwbQ06vK0agEzwZu9ax2liiYXnJ6V4ZlbilF2dQDrLPA5kGQGJRRi5rtPFUuy 6rZrgviZtswd0zFQdyCtdE9G6pKSJbHj2Oero= 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=Fg8UDD6e4x+n15D9+cpQ+V2P6iSDoCU4cPj5Bf52gls=; b=muVf81aQVDP+Kf9ZiMedVYKQjwjI3wqCCOJ4CUzETLgOThAu/YRuLK2pVN7UECOXG9 FAIJQBKT4tHS46Zn0gvLfwtJSc+HtSQg0+lweKypc+jxytm//xbHB6bNO4j12FtbmNRL 5xB0EUXzMNPI8rnKnmyYOhxD2B5tTa/dVY0wzeeJTz9zfviZYsBCw9rpMN72EVRE7PGs KJsRzpau0D19NSpi2+lgJFe49TGXVW5Azu70aFt/3fyb5TgO22yj4UZ1qzjCRngpMYkz IYUZCdWYZokCB1v8L+TCFv12qMdFWFKzdOsCbmHOiYLBcr/NANL0yWl+X4tgFB5KBE9x BhPg== X-Gm-Message-State: ALQs6tB0QeBKgK/G3X06C1ZVoQUufNo3GuyZDnlkW0E83D4ZhFCty/67 UbXE3xoMsVhBISwYYPEgIlJLzBzBKOmVo8snpUC93A== X-Google-Smtp-Source: AIpwx48EwDJ9mpYa6uGmBZFwQxtmftylLnj6l2IlSTzDfAFlCMsT2kjlNzc8PT/DP2R7oO12ijFRxm/tCPA6Ba38RfM= X-Received: by 2002:a24:9456:: with SMTP id j83-v6mr6261381ite.50.1524141012598; Thu, 19 Apr 2018 05:30:12 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.187.67 with HTTP; Thu, 19 Apr 2018 05:30:12 -0700 (PDT) In-Reply-To: References: <20180416104412.npzwcvl6zlrh426k@bivouac.eciton.net> From: Ard Biesheuvel Date: Thu, 19 Apr 2018 14:30:12 +0200 Message-ID: To: Chris Co Cc: Leif Lindholm , "edk2-devel@lists.01.org" Subject: Re: [PATCH] ArmPkg/ArmMmuLib ARM: fix Mva to use idx instead of table base X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 Apr 2018 12:30:14 -0000 Content-Type: text/plain; charset="UTF-8" On 16 April 2018 at 21:45, Chris Co wrote: > Hi Leif, > >> -----Original Message----- >> From: Leif Lindholm >> Sent: Monday, April 16, 2018 3:44 AM >> To: Chris Co >> Cc: edk2-devel@lists.01.org; Ard Biesheuvel >> Subject: Re: [PATCH] ArmPkg/ArmMmuLib ARM: fix Mva to use idx instead >> of table base >> >> On Fri, Apr 13, 2018 at 11:43:27PM +0000, Chris Co wrote: >> > Mva address calculation should use the left-shifted current section >> > index instead of the left-shifted table base address. >> > >> > Using the table base address here has the side-effect of potentially >> > causing an access violation depending on the base address value. >> > >> > Cc: Leif Lindholm >> > Cc: Ard Biesheuvel >> > Contributed-under: TianoCore Contribution Agreement 1.1 >> > Signed-off-by: Christopher Co >> > --- >> > ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c >> > b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c >> > index 774a7ccf59..9bf4ba03fd 100644 >> > --- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c >> > +++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c >> > @@ -716,7 +716,7 @@ UpdateSectionEntries ( >> > Descriptor |= EntryValue; >> > >> > if (CurrentDescriptor != Descriptor) { >> > - Mva = (VOID *)(UINTN)(((UINTN)FirstLevelTable) << >> TT_DESCRIPTOR_SECTION_BASE_SHIFT); >> > + Mva = (VOID *)(UINTN)(((UINTN)FirstLevelIdx + i) << >> > + TT_DESCRIPTOR_SECTION_BASE_SHIFT); >> >> So, this clearly looks like you've found a bug - thanks! >> >> But I am a little bit confused about the patch - should this not need to >> incorporate the descriptor size in some way? >> I.e. something like >> Mva = (VOID *)(UINTN)(((UINTN)FirstLevelIdx + (i * sizeof(UINTN))) << >> TT_DESCRIPTOR_SECTION_BASE_SHIFT); >> or >> ... &FirstLevelTable[FirstLevelIndex + i] ... >> >> ? >> >> Regards, >> >> Leif >> > I don't think descriptor size is needed here. > > My understanding is that Mva is the base address of the current section. > > FirstLevelidx is derived by the first section's BaseAddress >> 20. The current section > index is then (FirstLevelIdx + i), which makes the base address of the current > section (FirstLeveLidx + i) << 20. > Indeed. 'Index' is a bit misleading here, given that it is the top level index into the entire VA space, and so it is congruent with the virtual base address itself. The use of 'FirstLevelTable' in this context is obviously incorrect, given that it refers to the [physical] address of the page tables itself, not to the virtual region they describe. Reviewed-by: Ard Biesheuvel