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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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>