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.lindh...@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Christopher Co <christopher...@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
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
