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

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

SmmAccessOut = SMM access memory outside SMRAM.

So, do we want to treat SMM access ACPI NVS, RSVD, Runtime, MMIO, to be SmmAccessOut? 

Thank you
Yao Jiewen

> -----Original Message-----
> From: Ni, Ray
> Sent: Thursday, August 1, 2019 10:24 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
> 
> Hi Jiewen,
> 
> >
> > 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.
> 
> SmmAccessOut = SMM access memory outside SMRAM.
> My experience is sometimes someone may think up a personally preferred
> name but may be very confusing to others. I also want to avoid that.
> 
> Do you have a name 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
> > > > >
> > > > > 
> > > > >