Re: [PATCH] ArmPkg/ArmMmuLib ARM: fix Mva to use idx instead of table base

[Chris Co <Christopher.Co@microsoft.com>]

Hi,

Just checking if there is anything needed on my end to get this patch merged in.

Chris

> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Sent: Thursday, April 19, 2018 5:30 AM
> To: Chris Co <Christopher.Co@microsoft.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>; edk2-devel@lists.01.org
> Subject: Re: [PATCH] ArmPkg/ArmMmuLib ARM: fix Mva to use idx instead
> of table base
> 
> On 16 April 2018 at 21:45, Chris Co <Christopher.Co@microsoft.com> wrote:
> > Hi Leif,
> >
> >> -----Original Message-----
> >> From: Leif Lindholm <leif.lindholm@linaro.org>
> >> Sent: Monday, April 16, 2018 3:44 AM
> >> To: Chris Co <Christopher.Co@microsoft.com>
> >> Cc: edk2-devel@lists.01.org; Ard Biesheuvel
> >> <ard.biesheuvel@linaro.org>
> >> 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 <leif.lindholm@linaro.org>
> >> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> > Contributed-under: TianoCore Contribution Agreement 1.1
> >> > Signed-off-by: Christopher Co <christopher.co@microsoft.com>
> >> > ---
> >> >  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 <ard.biesheuvel@linaro.org>