[edk2-devel] [PATCH v2 2/2] UefiCpuPkg/PiSmmCpu: PcdCpuSmmAccessOut controls SMM access-out policy
Laszlo Ersek
lersek at redhat.com
Wed Jul 31 23:46:15 UTC 2019
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 at intel.com>
>> Cc: Eric Dong <eric.dong at intel.com>
>> Cc: Jiewen Yao <jiewen.yao at intel.com>
>> Cc: Jian J Wang <jian.j.wang at intel.com>
>> Cc: Laszlo Ersek <lersek at 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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#44706): https://edk2.groups.io/g/devel/message/44706
Mute This Topic: https://groups.io/mt/32668874/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list