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

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

Thanks. :)
Pushed this patch at https://github.com/tianocore/edk2/commit/714c2603018a99a514c42c2b511c821f30ba9cdf.
And sent the patch at https://lists.01.org/pipermail/edk2-devel/2017-August/013768.html for the definition movement.

Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Monday, August 28, 2017 1:10 AM
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/26/17 04:24, Zeng, Star wrote:
> Laszlo,
> 
> I am ok to centralize the definition in the patch(V2 will cover it), how about PiSmmCpuDxeSmm.c?

Because I have now tested this patch (see below), at this point my preference would be that you commit v1 as-is, and then post a separate patch to centralize the variable. (I wouldn't like to re-test because of the variable movement.)

I would have preferred to test v2 only, but the above works too I guess.

> Have you helped get the test result with the patch?

The crash is gone for the IA32 build, and I found no regressions in the
IA32X64 build.

Tested-by: Laszlo Ersek <lersek@redhat.com>

Thanks,
Laszlo

> -----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
>>
> 
> _______________________________________________
> 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