[edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF
Laszlo Ersek
lersek at redhat.com
Fri Jul 26 10:10:14 UTC 2019
On 07/26/19 11:58, Laszlo Ersek wrote:
> Ray,
>
> On 07/18/19 08:58, Ni, Ray wrote:
>> Commit c60d36b4d1ee1f69b7cca897d3621dfa951895c2
>> * UefiCpuPkg/SmmCpu: Block access-out only when static paging is used
>>
>> updated page fault handler to treat SMM access-out as allowed
>> address when static paging is not used.
>>
>> But that commit is not complete because the page table is still
>> updated in SetUefiMemMapAttributes() for non-SMRAM memory. When SMM
>> code accesses non-SMRAM memory, page fault is still generated.
>>
>> This patch skips to update page table for non-SMRAM memory and
>> page table itself.
>>
>> 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>
>> ---
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 21 +++++++++++++++------
>> 1 file changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> index 2f7d777ee7..f75e75f55c 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> @@ -1103,6 +1103,9 @@ FindSmramInfo (
>> *SmrrBase = (UINT32)CurrentSmramRange->CpuStart;
>> *SmrrSize = (UINT32)CurrentSmramRange->PhysicalSize;
>>
>> + //
>> + // Extend *SmrrBase/*SmrrSize to include adjacent SMRAM ranges
>> + //
>> do {
>> Found = FALSE;
>> for (Index = 0; Index < mSmmCpuSmramRangeCount; Index++) {
>> @@ -1414,14 +1417,20 @@ PerformRemainingTasks (
>> SetMemMapAttributes ();
>>
>> //
>> - // For outside SMRAM, we only map SMM communication buffer or MMIO.
>> + // Do not protect memory outside SMRAM when SMM static page table is not enabled.
>> //
>> - SetUefiMemMapAttributes ();
>> + if (mCpuSmmStaticPageTable) {
>>
>> - //
>> - // Set page table itself to be read-only
>> - //
>> - SetPageTableAttributes ();
>> + //
>> + // For outside SMRAM, we only map SMM communication buffer or MMIO.
>> + //
>> + SetUefiMemMapAttributes ();
>> +
>> + //
>> + // Set page table itself to be read-only
>> + //
>> + SetPageTableAttributes ();
>> + }
>>
>> //
>> // Configure SMM Code Access Check feature if available.
>>
>
> Commit 30f6148546c7 causes a build failure, when building for IA32:
>
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c: In function 'PerformRemainingTasks':
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c:1440:9: error: 'mCpuSmmStaticPageTable' undeclared (first use in this function)
>> if (mCpuSmmStaticPageTable) {
>
> "mCpuSmmStaticPageTable" is an X64-only variable. It is defined in
> "X64/PageTbl.c", which is not linked into the IA32 binary. We must not
> reference the variable in such code that is linked into both IA32 and
> X64 builds, such as "PiSmmCpuDxeSmm.c".
>
> We have encountered the same challenge at least once in the past:
>
> - https://bugzilla.tianocore.org/show_bug.cgi?id=1593
> - commit 37f9fea5b88d ("UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand
> paging in SMM", 2019-04-04)
>
> The right approach is to declare a new function in "PiSmmCpuDxeSmm.h",
> and to provide two definitions for the function, one in
> "Ia32/PageTbl.c", and another in "X64/PageTbl.c". The IA32
> implementation should return a constant value. The X64 implementation
> should return "mCpuSmmStaticPageTable". (In the example named above, the
> functions were SaveCr2() and RestoreCr2().)
>
> --*--
>
> I'm going to revert commit 30f6148546c7 immediately, because it breaks
> the build, and because catching this issue in advance would have been
> trivial, if you had attempted to build for IA32. (To be sure, the
> reviewers on this patch are not responsible; reviewers are welcome, but
> not required, to catch such errors that the compiler/linker is supposed
> to catch.) With this build breakage, I wouldn't be able to test Eric's
> series
>
> [edk2-devel] [Patch v3 0/6] UefiCpuPkg: Enable Edkii Mp Services2 Ppi
>
> and I'd like to proceed with that.
The revert has commit hash d47b85a621ad.
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#44452): https://edk2.groups.io/g/devel/message/44452
Mute This Topic: https://groups.io/mt/32512605/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