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

[Leif Lindholm <leif.lindholm@linaro.org>]

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

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