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

["Ni, Ray" <ray.ni@intel.com>]

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