* Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF
2019-07-18 6:58 [PATCH] UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF Ni, Ray
@ 2019-07-18 15:14 ` Laszlo Ersek
2019-07-19 7:51 ` Dong, Eric
2019-07-26 9:58 ` Laszlo Ersek
2 siblings, 0 replies; 5+ messages in thread
From: Laszlo Ersek @ 2019-07-18 15:14 UTC (permalink / raw)
To: devel, ray.ni; +Cc: Eric Dong, Jiewen Yao, Jian J Wang
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@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>
> ---
> 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.
>
This patch is a no-op for platforms with PcdCpuSmmStaticPageTable=TRUE
(which is both the UefiCpuPkg default and what OVMF uses), so, from my side:
Acked-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF
2019-07-18 6:58 [PATCH] UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF Ni, Ray
2019-07-18 15:14 ` [edk2-devel] " Laszlo Ersek
@ 2019-07-19 7:51 ` Dong, Eric
2019-07-26 9:58 ` Laszlo Ersek
2 siblings, 0 replies; 5+ messages in thread
From: Dong, Eric @ 2019-07-19 7:51 UTC (permalink / raw)
To: devel@edk2.groups.io, Ni, Ray; +Cc: Yao, Jiewen, Wang, Jian J
Reviewed-by: Eric Dong <eric.dong@intel.com>
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ni,
> Ray
> Sent: Thursday, July 18, 2019 2:58 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Wang, Jian J <jian.j.wang@intel.com>
> Subject: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Allow SMM access-
> out when static paging is OFF
>
> 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@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>
> ---
> 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.
> --
> 2.21.0.windows.1
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF
2019-07-18 6:58 [PATCH] UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF Ni, Ray
2019-07-18 15:14 ` [edk2-devel] " Laszlo Ersek
2019-07-19 7:51 ` Dong, Eric
@ 2019-07-26 9:58 ` Laszlo Ersek
2019-07-26 10:10 ` Laszlo Ersek
2 siblings, 1 reply; 5+ messages in thread
From: Laszlo Ersek @ 2019-07-26 9:58 UTC (permalink / raw)
To: devel, ray.ni; +Cc: Eric Dong, Jiewen Yao, Jian J Wang
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@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>
> ---
> 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.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF
2019-07-26 9:58 ` Laszlo Ersek
@ 2019-07-26 10:10 ` Laszlo Ersek
0 siblings, 0 replies; 5+ messages in thread
From: Laszlo Ersek @ 2019-07-26 10:10 UTC (permalink / raw)
To: devel, ray.ni; +Cc: Eric Dong, Jiewen Yao, Jian J Wang
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@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>
>> ---
>> 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
^ permalink raw reply [flat|nested] 5+ messages in thread