Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection crash

["Zeng, Star" <star.zeng@intel.com>]

Laszlo,

I am ok to centralize the definition in the patch(V2 will cover it), how about PiSmmCpuDxeSmm.c?

Have you helped get the test result with the patch?


Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Friday, August 25, 2017 7:52 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
Subject: Re: [edk2] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection crash

On 08/25/17 11:53, Zeng, Star wrote:
> Laszlo,
> 
> X64 defined mPhysicalAddressBits already before the patch, and has the code below to assign it.
> 
>   mPhysicalAddressBits = CalculateMaximumSupportAddress ();

Thanks.

Do you think it would make sense to centralize the definition (i.e., the
allocation) of the mPhysicalAddressBits variable in this patch?

That is,
- instead of adding mPhysicalAddressBits to "Ia32/PageTbl.c",
- you could move it from "X64/PageTbl.c" to "SmmCpuMemoryManagement.c"
  (or another C source file that is built into both Ia32 and X64).

Thanks,
Laszlo

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Laszlo Ersek
> Sent: Friday, August 25, 2017 5:50 PM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric 
> <eric.dong@intel.com>
> Subject: Re: [edk2] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory 
> protection crash
> 
> Star,
> 
> On 08/24/17 05:20, Star Zeng wrote:
>> https://bugzilla.tianocore.org/show_bug.cgi?id=624 reports memory 
>> protection crash in PiSmmCpuDxeSmm, Ia32 build with RAM above 4GB (of 
>> which 2GB are placed in 64-bit address).
>> It is because UEFI builds identity mapping page tables,
>>> 4G address is not supported at Ia32 build.
>>
>> This patch is to get the PhysicalAddressBits that is used to build in 
>> PageTbl.c(Ia32/X64), and use it to check whether the address is 
>> supported or not in ConvertMemoryPageAttributes().
>>
>> With this patch, the debug messages will be like below.
>> UefiMemory protection: 0x0 - 0x9F000 Success UefiMemory protection: 
>> 0x100000 - 0x807000 Success UefiMemory protection: 0x808000 - 
>> 0x810000 Success UefiMemory protection: 0x818000 - 0x820000 Success 
>> UefiMemory
>> protection: 0x1510000 - 0x7B798000 Success UefiMemory protection: 
>> 0x7B79B000 - 0x7E538000 Success UefiMemory protection: 0x7E539000 -
>> 0x7E545000 Success UefiMemory protection: 0x7E55A000 - 0x7E61F000 
>> Success UefiMemory protection: 0x7E62B000 - 0x7F6AB000 Success 
>> UefiMemory protection: 0x7F703000 - 0x7F70B000 Success UefiMemory
>> protection: 0x7F70F000 - 0x7F778000 Success UefiMemory protection: 
>> 0x100000000 - 0x180000000 Unsupported
>>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Originally-suggested-by: Jiewen Yao <jiewen.yao@intel.com>
>> Reported-by: Laszlo Ersek <lersek@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Star Zeng <star.zeng@intel.com>
>> ---
>>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c           |  4 +++
>>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  1 +
>>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 31
>> +++++++++++++++++-----
>>  3 files changed, 30 insertions(+), 6 deletions(-)
>>
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
>> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
>> index 32ce5958c59c..e88b42d73343 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
>> @@ -16,6 +16,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>>  
>>  #include "PiSmmCpuDxeSmm.h"
>>  
>> +UINT8                               mPhysicalAddressBits;
>> +
>>  /**
>>    Create PageTable for SMM use.
>>  
>> @@ -36,6 +38,8 @@ SmmInitPageTable (
>>    //
>>    InitializeSpinLock (mPFLock);
>>  
>> +  mPhysicalAddressBits = 32;
>> +
>>    if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
>>      //
>>      // Set own Page Fault entry instead of the default one, because 
>> SMM Profile diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>> index dbce9ec520fe..1cf85c1481a7 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>> @@ -419,6 +419,7 @@ extern SPIN_LOCK                           *mConfigSmmCodeAccessCheckLock;
>>  extern SPIN_LOCK                           *mMemoryMappedLock;
>>  extern EFI_SMRAM_DESCRIPTOR                *mSmmCpuSmramRanges;
>>  extern UINTN                               mSmmCpuSmramRangeCount;
>> +extern UINT8                               mPhysicalAddressBits;
>>  
>>  //
>>  // Copy of the PcdPteMemoryEncryptionAddressOrMask
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>> index a535389c26ce..3ad5256f1e03 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>> @@ -1,6 +1,6 @@
>>  /** @file
>>  
>> -Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
>> +Copyright (c) 2016 - 2017, Intel Corporation. All rights 
>> +reserved.<BR>
>>  This program and the accompanying materials  are licensed and made 
>> available under the terms and conditions of the BSD License  which 
>> accompanies this distribution.  The full text of the license may be 
>> found at @@ -380,6 +380,7 @@ ConvertMemoryPageAttributes (
>>    PAGE_ATTRIBUTE                    SplitAttribute;
>>    RETURN_STATUS                     Status;
>>    BOOLEAN                           IsEntryModified;
>> +  EFI_PHYSICAL_ADDRESS              MaximumSupportMemAddress;
>>  
>>    ASSERT (Attributes != 0);
>>    ASSERT ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO |
>> EFI_MEMORY_XP)) == 0); @@ -391,6 +392,17 @@ ConvertMemoryPageAttributes (
>>      return RETURN_INVALID_PARAMETER;
>>    }
>>  
>> +  MaximumSupportMemAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)(LShiftU64
>> + (1, mPhysicalAddressBits) - 1);  if (BaseAddress > MaximumSupportMemAddress) {
>> +    return RETURN_UNSUPPORTED;
>> +  }
>> +  if (Length > MaximumSupportMemAddress) {
>> +    return RETURN_UNSUPPORTED;
>> +  }
>> +  if ((Length != 0) && (BaseAddress > MaximumSupportMemAddress - (Length - 1))) {
>> +    return RETURN_UNSUPPORTED;
>> +  }
>> +
>>  //  DEBUG ((DEBUG_ERROR, "ConvertMemoryPageAttributes(%x) - %016lx, 
>> %016lx, %02lx\n", IsSet, BaseAddress, Length, Attributes));
>>  
>>    if (IsSplitted != NULL) {
>> @@ -1037,6 +1049,7 @@ SetUefiMemMapAttributes (
>>    VOID
>>    )
>>  {
>> +  EFI_STATUS            Status;
>>    EFI_MEMORY_DESCRIPTOR *MemoryMap;
>>    UINTN                 MemoryMapEntryCount;
>>    UINTN                 Index;
>> @@ -1052,12 +1065,18 @@ SetUefiMemMapAttributes (
>>    MemoryMap = mUefiMemoryMap;
>>    for (Index = 0; Index < MemoryMapEntryCount; Index++) {
>>      if (IsUefiPageNotPresent(MemoryMap)) {
>> -      DEBUG ((DEBUG_INFO, "UefiMemory protection: 0x%lx - 0x%lx\n", MemoryMap->PhysicalStart, MemoryMap->PhysicalStart + (UINT64)EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages)));
>> -      SmmSetMemoryAttributes (
>> +      Status = SmmSetMemoryAttributes (
>> +                 MemoryMap->PhysicalStart,
>> +                 EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
>> +                 EFI_MEMORY_RP
>> +                 );
>> +      DEBUG ((
>> +        DEBUG_INFO,
>> +        "UefiMemory protection: 0x%lx - 0x%lx %r\n",
>>          MemoryMap->PhysicalStart,
>> -        EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
>> -        EFI_MEMORY_RP
>> -        );
>> +        MemoryMap->PhysicalStart + (UINT64)EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
>> +        Status
>> +        ));
>>      }
>>      MemoryMap = NEXT_MEMORY_DESCRIPTOR(MemoryMap, mUefiDescriptorSize);
>>    }
>>
> 
> before applying this patch for local testing, I figured I'd look it over quickly.
> 
> I think that you missed adding the X64 changes to the commit, with 
> "git add". Because, the "mPhysicalAddressBits" variable is declared in 
> common code, it is also consumed in common code, but it is only 
> defined (i.e.,
> allocated) and set in Ia32 code. I believe that applying this exact patch would prevent PiSmmCpuDxeSmm even from linking.
> 
> I think for X64 you likely have a change similar to the Ia32 one (defining the variable and setting it to the actual physical address bits, likely from the CPU HOB), but it's not part of the patch.
> 
> If I'm right and you decide to post v2, then I suggest another (very
> small) improvement: I think the definition (=allocation) of "mPhysicalAddressBits" could also be moved to common code; only the assignments have to be architecture-specific.
> 
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>