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

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

I agree that we only need support #2 and #3.

We need one PCD:
1) if it is set to TRUE, it locks SMM paging (make it static), only allows SMM access RSVD, ACPINVS, Runtime, as comm buffer, plus MMIO for device access. That is secure configuration.
2) it is FALSE, it allows SMM to access OS memory. It could be static or dynamic paging.


PcdCpuSmmAccessOut seems also confusing.
	What "Out" means ???
	What Out=False means? Only allow inside SMRAM access?
Anyway, I am open for the naming proposal.



Thank you
Yao Jiewen

> -----Original Message-----
> From: Ni, Ray
> Sent: Thursday, August 1, 2019 9:28 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;
> lersek@redhat.com
> Cc: Dong, Eric <eric.dong@intel.com>; Wang, Jian J <jian.j.wang@intel.com>
> Subject: RE: [edk2-devel] [PATCH v2 2/2] UefiCpuPkg/PiSmmCpu:
> PcdCpuSmmAccessOut controls SMM access-out policy
> 
> > Below is my thought, please correct if I am wrong.
> > 1) StaticPaging=false, AccessOut=false: it seems invalid. If we don’t
> support access out, why we need dynamic paging?
> > 2) StaticPaging=false, AccessOut=true: it seems valid. We need access out,
> but we only want a small paging in the beginning. As
> > such we use dynamic paging. This is to support Hotplug memory.
> > 3) StaticPaging=true, AccessOut=false: it seems valid. The is secure
> configuration.
> > 4) StaticPaging=true, AccessOut=true: it seems valid, but I do not see the
> value to support this. If we always allow access out,
> > what is the value to set static paging. Or why we care the paging is static
> or dynamic?
> 
> 
> Jiewen,
> The value of static paging is to reduce page table SMRAM consumption. We
> could create the page table in advance and that
> page table permits smm access out.
> 
> >
> > As such I recommend we only support #2 and #3.
> 
> Supporting #2 and #3 is based on real requirement, not from above
> discussion of 4 combinations.
> 
> >
> > Again, if the naming is confusing, I agree we should clarify or even rename.
> 
> I also agree that having fewer PCDs to provide fewer configurations. It also
> reduces validation effort.
> 
> Jiewen & Laszlo,
> Do you agree that with only #2 and #3 supported, the existing PCD can be
> renamed to PcdCpuSmmAccessOut?
> If AccessOut=true, it implies static paging is not used.
> If AccessOut=false, it implies static paging is used.
> 
> My goal is to have a way to allow RAS code access out from SMM after
> ReadyToLock.
> Any solution that can meet this goal is ok to me. I don't want to add
> confusing/unnecessary-complexity due to this goal.
> 
> > What I am trying to achieve is to limit the number of supported
> combination to reduce the effort of validation and maintenance.
> >
> > thank you!
> > Yao, Jiewen
> >
> >
> > > 在 2019年8月1日，上午7:13，Laszlo Ersek <lersek@redhat.com> 写
> 道：
> > >
> > > 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?
> > >
> > > (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
> > >
> > > 
> > >