Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF

["Laszlo Ersek" <lersek@redhat.com>]

On 07/26/19 11:58, Laszlo Ersek wrote:
> Ray,
> 
> On 07/18/19 08:58, Ni, Ray wrote:
>> Commit c60d36b4d1ee1f69b7cca897d3621dfa951895c2
>> * UefiCpuPkg/SmmCpu: Block access-out only when static paging is used
>>
>> updated page fault handler to treat SMM access-out as allowed
>> address when static paging is not used.
>>
>> But that commit is not complete because the page table is still
>> updated in SetUefiMemMapAttributes() for non-SMRAM memory. When SMM
>> code accesses non-SMRAM memory, page fault is still generated.
>>
>> This patch skips to update page table for non-SMRAM memory and
>> page table itself.
>>
>> Signed-off-by: Ray Ni <ray.ni@intel.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> ---
>>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 21 +++++++++++++++------
>>  1 file changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> index 2f7d777ee7..f75e75f55c 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> @@ -1103,6 +1103,9 @@ FindSmramInfo (
>>    *SmrrBase = (UINT32)CurrentSmramRange->CpuStart;
>>    *SmrrSize = (UINT32)CurrentSmramRange->PhysicalSize;
>>
>> +  //
>> +  // Extend *SmrrBase/*SmrrSize to include adjacent SMRAM ranges
>> +  //
>>    do {
>>      Found = FALSE;
>>      for (Index = 0; Index < mSmmCpuSmramRangeCount; Index++) {
>> @@ -1414,14 +1417,20 @@ PerformRemainingTasks (
>>      SetMemMapAttributes ();
>>
>>      //
>> -    // For outside SMRAM, we only map SMM communication buffer or MMIO.
>> +    // Do not protect memory outside SMRAM when SMM static page table is not enabled.
>>      //
>> -    SetUefiMemMapAttributes ();
>> +    if (mCpuSmmStaticPageTable) {
>>
>> -    //
>> -    // Set page table itself to be read-only
>> -    //
>> -    SetPageTableAttributes ();
>> +      //
>> +      // For outside SMRAM, we only map SMM communication buffer or MMIO.
>> +      //
>> +      SetUefiMemMapAttributes ();
>> +
>> +      //
>> +      // Set page table itself to be read-only
>> +      //
>> +      SetPageTableAttributes ();
>> +    }
>>
>>      //
>>      // Configure SMM Code Access Check feature if available.
>>
> 
> Commit 30f6148546c7 causes a build failure, when building for IA32:
> 
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c: In function 'PerformRemainingTasks':
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c:1440:9: error: 'mCpuSmmStaticPageTable' undeclared (first use in this function)
>>      if (mCpuSmmStaticPageTable) {
> 
> "mCpuSmmStaticPageTable" is an X64-only variable. It is defined in
> "X64/PageTbl.c", which is not linked into the IA32 binary. We must not
> reference the variable in such code that is linked into both IA32 and
> X64 builds, such as "PiSmmCpuDxeSmm.c".
> 
> We have encountered the same challenge at least once in the past:
> 
> - https://bugzilla.tianocore.org/show_bug.cgi?id=1593
> - commit 37f9fea5b88d ("UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand
>   paging in SMM", 2019-04-04)
> 
> The right approach is to declare a new function in "PiSmmCpuDxeSmm.h",
> and to provide two definitions for the function, one in
> "Ia32/PageTbl.c", and another in "X64/PageTbl.c". The IA32
> implementation should return a constant value. The X64 implementation
> should return "mCpuSmmStaticPageTable". (In the example named above, the
> functions were SaveCr2() and RestoreCr2().)
> 
> --*--
> 
> I'm going to revert commit 30f6148546c7 immediately, because it breaks
> the build, and because catching this issue in advance would have been
> trivial, if you had attempted to build for IA32. (To be sure, the
> reviewers on this patch are not responsible; reviewers are welcome, but
> not required, to catch such errors that the compiler/linker is supposed
> to catch.) With this build breakage, I wouldn't be able to test Eric's
> series
> 
>   [edk2-devel] [Patch v3 0/6] UefiCpuPkg: Enable Edkii Mp Services2 Ppi
> 
> and I'd like to proceed with that.

The revert has commit hash d47b85a621ad.

Thanks
Laszlo