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

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

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

Sounds good. Thank you!

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Kun Qin
Sent: Monday, December 28, 2020 2:36 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: [edk2-devel] [PATCH v1 04/15] StandaloneMmPkg: StandaloneMmMemLib: Extends support for X64 architecture

Understood, not including this library for x86 will fail CI build on some new drivers in this patch series, so we will need to give it a full fix. I will add the fix at least for x86 in patch v2.

Thanks,
Kun

From: Yao, Jiewen<mailto:jiewen.yao@intel.com>
Sent: Sunday, December 27, 2020 22:24
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

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<mailto:kun.q@outlook.com>>
Sent: Monday, December 28, 2020 12:15 PM
To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; 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>>; Supreeth Venkatesh <supreeth.venkatesh@arm.com<mailto: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: 14461 bytes --]