Re: [PATCH v1 04/15] StandaloneMmPkg: StandaloneMmMemLib: Extends support for X64 architecture

["Yao, Jiewen" <jiewen.yao@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 6695 bytes --]

I am not familiar with AArch64. So I will let ARM people comment it.

For X86, I think it is security hole, if we do not fill mMmMemLibInternalMmramRanges.

A partial fix with known security vulnerability is not the best idea.

I prefer to we give a full fix, or no fix it in V2.

Thank you
Yao Jiewen


From: Kun Qin <kun.q@outlook.com>
Sent: Monday, December 28, 2020 12:15 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>; Sami Mujawar <sami.mujawar@arm.com>; Supreeth Venkatesh <supreeth.venkatesh@arm.com>
Subject: RE: [PATCH v1 04/15] StandaloneMmPkg: StandaloneMmMemLib: Extends support for X64 architecture

Hi Jiewen,

I did not fill these 2 variables for this patch because I thought to follow up fix for this issue separately because the AARCH64 instance does not fill it either and I meant for this patch to solely extends the coverage to x64 (and IA32). But please let me know if you think otherwise, I can add the x64 fix in this patch as well. But I do not have fix for AARCH64 since I am not familiar with how it should work.

Thanks,
Kun

From: Yao, Jiewen<mailto:jiewen.yao@intel.com>
Sent: Sunday, December 27, 2020 16:18
To: Kun Qin<mailto:kun.q@outlook.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Ard Biesheuvel<mailto:ard.biesheuvel@arm.com>; Sami Mujawar<mailto:sami.mujawar@arm.com>; Supreeth Venkatesh<mailto:supreeth.venkatesh@arm.com>
Subject: RE: [PATCH v1 04/15] StandaloneMmPkg: StandaloneMmMemLib: Extends support for X64 architecture

May I know where is the code to fill below?
EFI_MMRAM_DESCRIPTOR *mMmMemLibInternalMmramRanges;
UINTN                mMmMemLibInternalMmramCount;



> -----Original Message-----
> From: Kun Qin <kun.q@outlook.com<mailto:kun.q@outlook.com>>
> Sent: Saturday, December 19, 2020 2:50 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com<mailto:ard.biesheuvel@arm.com>>; Sami Mujawar
> <sami.mujawar@arm.com<mailto:sami.mujawar@arm.com>>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Supreeth
> Venkatesh <supreeth.venkatesh@arm.com<mailto:supreeth.venkatesh@arm.com>>
> Subject: [PATCH v1 04/15] StandaloneMmPkg: StandaloneMmMemLib:
> Extends support for X64 architecture
>
> This change extends StandaloneMmMemLib library to support X64
> architecture. The implementation is ported from
> MdePkg/Library/SmmMemLib.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com<mailto:ard.biesheuvel@arm.com>>
> Cc: Sami Mujawar <sami.mujawar@arm.com<mailto:sami.mujawar@arm.com>>
> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
> Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com<mailto:supreeth.venkatesh@arm.com>>
>
> Signed-off-by: Kun Qin <kun.q@outlook.com<mailto:kun.q@outlook.com>>
> ---
>
> StandaloneMmPkg/Library/StandaloneMmMemLib/X64/StandaloneMmMe
> mLibInternal.c | 67 ++++++++++++++++++++
>
> StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib
> .inf           |  6 +-
>  2 files changed, 72 insertions(+), 1 deletion(-)
>
> diff --git
> a/StandaloneMmPkg/Library/StandaloneMmMemLib/X64/StandaloneMm
> MemLibInternal.c
> b/StandaloneMmPkg/Library/StandaloneMmMemLib/X64/StandaloneMm
> MemLibInternal.c
> new file mode 100644
> index 000000000000..c5e21c583f44
> --- /dev/null
> +++
> b/StandaloneMmPkg/Library/StandaloneMmMemLib/X64/StandaloneMm
> MemLibInternal.c
> @@ -0,0 +1,67 @@
> +/** @file
> +  Internal ARCH Specific file of MM memory check library.
> +
> +  MM memory check library implementation. This library consumes
> MM_ACCESS_PROTOCOL
> +  to get MMRAM information. In order to use this library instance, the
> platform should produce
> +  all MMRAM range via MM_ACCESS_PROTOCOL, including the range for
> firmware (like MM Core
> +  and MM driver) and/or specific dedicated hardware.
> +
> +  Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +#include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/HobLib.h>
> +//
> +// Maximum support address used to check input buffer
> +//
> +extern EFI_PHYSICAL_ADDRESS
> mMmMemLibInternalMaximumSupportAddress;
> +
> +/**
> +  Calculate and save the maximum support address.
> +
> +**/
> +VOID
> +MmMemLibInternalCalculateMaximumSupportAddress (
> +  VOID
> +  )
> +{
> +  VOID         *Hob;
> +  UINT32       RegEax;
> +  UINT8        PhysicalAddressBits;
> +
> +  //
> +  // Get physical address bits supported.
> +  //
> +  Hob = GetFirstHob (EFI_HOB_TYPE_CPU);
> +  if (Hob != NULL) {
> +    PhysicalAddressBits = ((EFI_HOB_CPU *) Hob)->SizeOfMemorySpace;
> +  } else {
> +    AsmCpuid (0x80000000, &RegEax, NULL, NULL, NULL);
> +    if (RegEax >= 0x80000008) {
> +      AsmCpuid (0x80000008, &RegEax, NULL, NULL, NULL);
> +      PhysicalAddressBits = (UINT8) RegEax;
> +    } else {
> +      PhysicalAddressBits = 36;
> +    }
> +  }
> +  //
> +  // IA-32e paging translates 48-bit linear addresses to 52-bit physical
> addresses.
> +  //
> +  ASSERT (PhysicalAddressBits <= 52);
> +  if (PhysicalAddressBits > 48) {
> +    PhysicalAddressBits = 48;
> +  }
> +
> +  //
> +  // Save the maximum support address in one global variable
> +  //
> +  mMmMemLibInternalMaximumSupportAddress =
> (EFI_PHYSICAL_ADDRESS)(UINTN)(LShiftU64 (1, PhysicalAddressBits) - 1);
> +  DEBUG ((DEBUG_INFO, "mMmMemLibInternalMaximumSupportAddress
> = 0x%lx\n", mMmMemLibInternalMaximumSupportAddress));
> +}
> +
> +
> diff --git
> a/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMem
> Lib.inf
> b/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMem
> Lib.inf
> index 49da02e54e6d..65ad0a48905c 100644
> ---
> a/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMem
> Lib.inf
> +++
> b/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMem
> Lib.inf
> @@ -26,12 +26,15 @@ [Defines]
>  #
>  # The following information is for reference only and not required by the
> build tools.
>  #
> -#  VALID_ARCHITECTURES           = AARCH64
> +#  VALID_ARCHITECTURES           = X64 AARCH64
>  #
>
>  [Sources.Common]
>    StandaloneMmMemLib.c
>
> +[Sources.X64]
> +  X64/StandaloneMmMemLibInternal.c
> +
>  [Sources.AARCH64]
>    AArch64/StandaloneMmMemLibInternal.c
>
> @@ -42,3 +45,4 @@ [Packages]
>  [LibraryClasses]
>    BaseMemoryLib
>    DebugLib
> +  HobLib
> --
> 2.28.0.windows.1


[-- Attachment #2: Type: text/html, Size: 11967 bytes --]