Re: [edk2-devel] [PATCH v2 2/2] UefiCpuPkg/PiSmmCpu: PcdCpuSmmAccessOut controls SMM access-out policy

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

On 08/01/19 01:13, Laszlo Ersek wrote:
> Hi Ray, Jiewen,
> 
> I've got several comments / questions:
> 
> On 07/31/19 18:38, Ni, Ray wrote:
>> This patch skips to update page table for non-SMRAM memory when
>> PcdCpuSmmAccessOut is TRUE.
>> So that when SMM accesses out, no page fault is triggered at all.
>>
>> 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>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 21 +++++++++++++++++----
>>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c    |  2 +-
>>  2 files changed, 18 insertions(+), 5 deletions(-)
>>
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> index 69a04dfb23..427c33fb01 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> @@ -130,6 +130,11 @@ UINT8                    mPhysicalAddressBits;
>>  UINT32                   mSmmCr0;
>>  UINT32                   mSmmCr4;
>>  
>> +//
>> +// Cache of PcdCpuSmmAccessOut
>> +//
>> +BOOLEAN                  mSmmAccessOut;
>> +
>>  /**
>>    Initialize IDT to setup exception handlers for SMM.
>>  
>> @@ -610,6 +615,12 @@ PiCpuSmmEntry (
>>    mSmmCodeAccessCheckEnable = PcdGetBool (PcdCpuSmmCodeAccessCheckEnable);
>>    DEBUG ((EFI_D_INFO, "PcdCpuSmmCodeAccessCheckEnable = %d\n", mSmmCodeAccessCheckEnable));
>>  
>> +  //
>> +  // Save the PcdCpuSmmAccessOut value into a global variable.
>> +  //
>> +  mSmmAccessOut = PcdGetBool (PcdCpuSmmAccessOut);
>> +  DEBUG ((DEBUG_INFO, "PcdCpuSmmAccessOut = %d\n", mSmmAccessOut));
>> +
>>    //
>>    // Save the PcdPteMemoryEncryptionAddressOrMask value into a global variable.
>>    // Make sure AddressEncMask is contained to smallest supported address field.
> 
> The above looks correct; however, the PcdGetBool() call depends on the
> INF file listing PcdCpuSmmAccessOut.
> 
> (1) Ray, did you forget to stage the INF file update for this patch commit?
> 
> 
>> @@ -1431,10 +1442,12 @@ PerformRemainingTasks (
>>      //
>>      SetMemMapAttributes ();
>>  
>> -    //
>> -    // For outside SMRAM, we only map SMM communication buffer or MMIO.
>> -    //
>> -    SetUefiMemMapAttributes ();
>> +    if (!mSmmAccessOut) {
>> +      //
>> +      // For outside SMRAM, we only map SMM communication buffer or MMIO.
>> +      //
>> +      SetUefiMemMapAttributes ();
>> +    }
> 
> This change looks OK. It seems to conditionalize the logic added in
> commit d2fc7711136a ("UefiCpuPkg/PiSmmCpu: Add SMM Comm Buffer Paging
> Protection.", 2016-12-19).
> 
> However, the comment confuses me a bit; it says "SMM communication
> buffer *or MMIO*".
> 
> I assume "SMM communication buffer" can be in "reserved, runtime and
> ACPI NVS" RAM, so that part likely matches the new PCD's explanation
> from patch#1. But MMIO is not mentioned in patch#1.
> 
> (2) Should we extend the description of the PCD in patch#1, with MMIO?
> 
> Or is MMIO considered *runtime* MMIO (and so covered by "runtime")?
> 
>>  
>>      //
>>      // Set page table itself to be read-only
> 
> In the previous version of the patch series, namely
> 
>   [edk2-devel] [PATCH 3/3]
>   UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF
> 
>   http://mid.mail-archive.com/20190727032850.337840-4-ray.ni@intel.com
>   https://edk2.groups.io/g/devel/message/44470
> 
> the next operation (namely SetPageTableAttributes()) was conditionalized
> too. Is that no longer necessary, with the new PCD? Shouldn't we still
> conditionalize SetPageTableAttributes() on the *other* (static paging) PCD?
> 
> Ah wait, we already do it! SetPageTableAttributes() has two
> implementations, one for IA32 and another for X64.
> 
> - The X64 variant checks for static page tables internally, and if they
> are disabled, then SetPageTableAttributes() does nothing.
> 
> - The IA32 variant does not contain that check, because on IA32 the page
> tables are always built statically.
> 
> So SetPageTableAttributes() already depends on static paging
> *internally*, hence gating the SetPageTableAttributes() call
> *externally*, like it was done in the previous version of the patch
> series, is superfluous in the first place.
> 
> Good!
> 
> 
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
>> index a3b62f7787..6699aac65d 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
>> @@ -1029,7 +1029,7 @@ SmiPFHandler (
>>        goto Exit;
>>      }
>>  
>> -    if (mCpuSmmStaticPageTable && IsSmmCommBufferForbiddenAddress (PFAddress)) {
>> +    if (IsSmmCommBufferForbiddenAddress (PFAddress)) {
>>        DumpCpuContext (InterruptType, SystemContext);
>>        DEBUG ((DEBUG_ERROR, "Access SMM communication forbidden address (0x%lx)!\n", PFAddress));
>>        DEBUG_CODE (
>>
> 
> This hunk looks good. Because:
> 
> - we ensure that there is no page fault at all, in the "access-out
> permitted" case, so there is no need to consider "access-out" in the
> fault handler at all;
> 
> - the "mSmmAccessOut" logic in PerformRemainingTasks() applies to both
> IA32 and X64 (commonly), and the SmiPFHandler() function is implemented
> for both IA32 and X64 (separately), and after this patch, both fault
> handlers check IsSmmCommBufferForbiddenAddress() identically. So this is
> nice and symmetric;
> 
> - we don't interfere with on-demand page table building (when it is
> enabled on X64) -- page faults should still be triggered by RAM pages
> not yet mapped at all, and the X64 variant of SmiDefaultPFHandler()
> should fix up *those* faults like before.
> 
> 
> However: given that this hunk practically undes commit c60d36b4, I would
> suggest that we revert commit c60d36b4 in a separate patch. So the
> series would go like:
> 
> - patch#1: commit c60d36b4 is not good enough, we're going to implement
> a separate approach, so revert commit c60d36b4, at first.
> - patch#2: introduce the new PCD
> - patch#3: disable page fault generation for non-SMRAM addresses (= keep
> them mapped as normal) to which access-out is permitted.
> 
> (3) Ray, do you agree to structure the patch series like that?

Hmmm wait a bit!

Just because we skip SetUefiMemMapAttributes() in
PerformRemainingTasks(), that doesn't mean we cannot end up in
SmiPFHandler()! Because other faults are possible too.

Whenever we skip SetUefiMemMapAttributes() in PerformRemainingTasks(),
we should *also* skip the IsSmmCommBufferForbiddenAddress() check in
SmiPFHandler().

Both SetUefiMemMapAttributes() and IsSmmCommBufferForbiddenAddress()
come from commit d2fc7711136a -- they are two parts of the same
protection feature. If we disable the protection for the sake of
access-out, we should disable all parts.

Furthermore, if we don't call either of
IsSmmCommBufferForbiddenAddress() and SetUefiMemMapAttributes(), then we
shouldn't call GetUefiMemoryMap() either!


So ultimately, I would argue for the following patch series:

- patch#1: Revert commit c60d36b4, and explain why, in the commit
  message.

- patch#2: Introduce the new PCD, also mentioning MMIO.

- Patch#3: modify *all* of the following functions, internally, to
  return immediately, if "mSmmAccessOut" is TRUE:

  - GetUefiMemoryMap()
  - SetUefiMemMapAttributes()
  - IsSmmCommBufferForbiddenAddress()

  Basically, the new PCD should short-circuit all three functions
  declared in "PiSmmCpuDxeSmm.h" by commit d2fc7711136a.

Thanks!
Laszlo



> (4) Jiewen, are you OK with the general approach, namely to relax
> access-out by eliminating *such* page faults, rather than fixing them up
> in the fault handler?
> 
> (I certainly agree with this approach: the fixup in the fault handler,
> namely SmiDefaultPFHandler(), only covers the X64 case; in the IA32
> case, it just hangs. That shows that the fault fixup is inherently tied
> to on-demand page table building, whereas the access-out feature should
> be possible to permit on IA32 too.)
> 
> Thanks,
> Laszlo
>