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

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

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.

Thanks,
Chris
> >
> >          // Clean/invalidate the cache for this section, but only
> >          // if we are modifying the memory type attributes
> > --
> > 2.15.1.gvfs.2.39.g03d366a
> >