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

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

Jiewen, Laszlo,
Firstly let's assume we are aligned to use single/one PCD to control both static page table creation and SMM access out.

With this assumption, we need more discussion about what proper PCD name to choose.

For now there are two candidates: PcdCpuSmmStaticPageTable and PcdCpuSmmAccessOut.
  Laszlo doesn't like PcdCpuSmmStaticPageTable (Jiewen proposed. I don't like this name either).
  Jiewen doesn't like PcdCpuSmmAccessOut (I proposed. Not sure if Laszlo likes it or not).

When using PcdCpuSmmStaticPageTable, the straightforward meaning is whether the page table
to cover all memory that needs access/forbidden is created in advance. When page table is created
in advance, it's a bit hard to deduce that access out is forbidden. When page table is dynamically
created in PF handler, it's also a bit hard to deduce that access out is allowed.

When using PcdCpuSmmAccessOut, the straightforward meaning is whether to allow SMM code
to access non-SMRAM memory. Exception is ACPI NVS/ RSVD, Runtime and MMIO can
always be accessible to SMM code. When PcdCpuSmmAccessOut is TRUE, whether to use static
page table is PiSmmCpu's implementation choice, but since dynamic page table creation in
PF handler saves the SMRAM, the PiSmmCpu can choose to use the optimal solution which means
to create page table in PF handler (Identical to Jiewen's case #2). When PcdCpuSmmAccessOut is
FALSE, it's more secure to have a static page table (Identical to Jiewen's case #3).
What I want to say here is SmmAccessOut is the policy that PiSmmCpu driver wants to get from
platform side, whether to use static page table is its own implementation choice to meet platform
SmmAccessOut requirement.

So I think PcdCpuSmmAccessOut is better than PcdCpuSmmStaticPageTable.

I agree with Jiewen's concern this name is not very precise to describe the behavior.

Laszlo, Jiewen,
Do you have some proposals?

Thanks,
Ray


> -----Original Message-----
> From: Yao, Jiewen
> Sent: Thursday, August 1, 2019 11:11 AM
> To: Ni, Ray <ray.ni@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
> 
> 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
> > > > > >
> > > > > > 
> > > > > >