Re: [PATCH 0/6] Enable SMM page level protection.

[Laszlo Ersek <lersek@redhat.com>]

On 11/03/16 07:53, Jiewen Yao wrote:
> This series patch enables SMM page level protection.
> Features are:
> 1) PiSmmCore reports SMM PE image code/data information
> in EdkiiPiSmmMemoryAttributeTable, if the SMM image is page aligned.
> 2) PiSmmCpu consumes EdkiiPiSmmMemoryAttributeTable
> and set XD for data page and RO for code page.
> 3) PiSmmCpu enables Static Paging for X64 according to
> PcdCpuSmmStaticPageTable. If it is true, 1G paging for above 4G
> is used as long as it is supported.
> 4) PiSmmCpu sets importance data structure to be read only,
> such as Gdt, Idt, SmmEntrypoint, and PageTable itself.
> 
> tested platform:
> 1) Intel internal platform (X64).
> 2) EDKII Quark IA32
> 3) EDKII Vlv2  X64
> 4) EDKII OVMF IA32 and IA32X64.

Did you use a Windows QEMU binary for the OVMF tests?

Please find my test results below, all done on KVM
(3.10.0-514.el7.x86_64, if anyone is curious :)).

> 
> Cc: Jeff Fan <jeff.fan@intel.com>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
> 
> Jiewen Yao (6):
>   MdeModulePkg/Include: Add PiSmmMemoryAttributesTable.h
>   MdeModulePkg/dec: Add gEdkiiPiSmmMemoryAttributesTableGuid.
>   MdeModulePkg/PiSmmCore: Add MemoryAttributes support.
>   UefiCpuPkg/dec: Add PcdCpuSmmStaticPageTable.
>   UefiCpuPkg/PiSmmCpuDxeSmm: Add paging protection.
>   QuarkPlatformPkg/dsc: enable Smm paging protection.

Legend:

- "untested" means the test was not executed because the same test
  failed or proved unreliable in a less demanding configuration already,

- "n/a" means a setting or test case was impossible,

- "fail" and "unreliable" (lower case) are outside the scope of this
  series; they either capture the pre-series status, or are expected
  even with the series applied due to the pre-series status,

- "FAIL" and "UNRELIABLE" mean regressions caused (or exposed) by the
  series.

In all cases, 36 bits were used as address width in the CPU HOB (--> up
to 64GB guest-phys address space).

   series  OVMF                                                              VCPU     boot       S3 resume
 # applied platform PcdCpuMaxLogicalProcessorNumber PcdCpuSmmStaticPageTable topology result     result
-- ------- -------- ------------------------------- ------------------------ -------- ------     ---------
 1 no      Ia32      64                              n/a                     1x2x2    pass       unreliable
 2 no      Ia32     255                              n/a                     52x2x2   pass       untested
 3 no      Ia32     255                              n/a                     53x2x2   unreliable untested
 4 no      Ia32X64   64                              n/a                     1x2x2    pass       pass
 5 no      Ia32X64  255                              n/a                     52x2x2   pass       pass
 6 no      Ia32X64  255                              n/a                     54x2x2   fail       n/a
 7 yes     Ia32      64                              FALSE                   1x2x2    FAIL       n/a
 8 yes     Ia32      64                              TRUE                    1x2x2    FAIL       n/a
 9 yes     Ia32     255                              FALSE                   52x2x2   untested   untested
10 yes     Ia32     255                              FALSE                   53x2x2   untested   untested
11 yes     Ia32     255                              TRUE                    52x2x2   untested   untested
12 yes     Ia32     255                              TRUE                    53x2x2   untested   untested
13 yes     Ia32X64   64                              FALSE                   1x2x2    pass       UNRELIABLE
14 yes     Ia32X64   64                              TRUE                    1x2x2    FAIL       n/a
15 yes     Ia32X64  255                              FALSE                   52x2x2   pass       untested
16 yes     Ia32X64  255                              FALSE                   54x2x2   fail       untested
17 yes     Ia32X64  255                              TRUE                    52x2x2   untested   untested
18 yes     Ia32X64  255                              TRUE                    54x2x2   untested   untested

Notes for the baseline tests (1-6):

* test 3: boot unreliable due to out-of-SMRAM (depends on what we do
  during boot)

* test 5: S3 wakeup: 1 try, suspend takes very long, CPU intensive;
  wakeup is quick, works okay

* test 6: boot fail: out of SMRAM (expected)

Notes for the tests with the series applied:

* test 7: normal boot is regressed even with
  PcdCpuSmmStaticPageTable=FALSE, relative to test 1:

> SmmInstallProtocolInterface: [EfiSmmReadyToLockProtocol] 0
> Patch page table start ...
> Patch page table done!
> MemoryAttributesTable - NULL
> SetPageTableAttributes
> Start...
> ConvertPageEntryAttribute 0x7FF9A067->0x7FF9A065
>
> ASSERT_EFI_ERROR (Status = Invalid Parameter)
> ASSERT UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c(468): !EFI_ERROR (Status)

  Note that the processor model used in this test has no support for NX.

* test 8: results identical to those of test 7

* test 13: S3 resume is unreliable (regression relative to test 4). End
  of OVMF log (Fedora guest):

> S3_BOOT_SCRIPT_LIB_TERMINATE_OPCODE
> S3BootScriptDone - Success
> Call AsmDisablePaging64() to return to S3 Resume in PEI Phase
> Install PPI: [PeiPostScriptTablePpi]
> Install PPI: [EfiEndOfPeiSignalPpi]
> Notify: PPI Guid: [EfiEndOfPeiSignalPpi], Peim notify entry point: 857895
> PeiMpInitLib: CpuMpEndOfPeiCallback () invoked
> Transfer to 16bit OS waking vector - 9A1D0

  QEMU log:

> KVM internal error. Suberror: 1
> KVM internal error. Suberror: 1
> emulation failure
> emulation failure
> RAX=0000000000000001 RBX=0000000000000000 RCX=000000007ffdbf48 RDX=000000007fede058
> RSI=0000000000000004 RDI=000000007fede040 RBP=0000000000000000 RSP=000000007e19f000
> R8 =0000000000000000 R9 =0000000000000000 R10=0000000000000000 R11=0000000000000000
> R12=0000000000000000 R13=0000000000000000 R14=0000000000000000 R15=0000000000000000
> RIP=000000000009f0fd RFL=00010006 [-----P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> ES =0030 0000000000000000 ffffffff 00c09300 DPL=0 DS   [-WA]
> CS =0038 0000000000000000 ffffffff 00a09b00 DPL=0 CS64 [-RA]
> SS =0030 0000000000000000 ffffffff 00c09300 DPL=0 DS   [-WA]
> DS =0030 0000000000000000 ffffffff 00c09300 DPL=0 DS   [-WA]
> FS =0018 0000000000000000 ffffffff 00c09300 DPL=0 DS   [-WA]
> GS =0018 0000000000000000 ffffffff 00c09300 DPL=0 DS   [-WA]
> LDT=0000 0000000000000000 0000ffff 00008200 DPL=0 LDT
> TR =0000 0000000000000000 0000ffff 00008b00 DPL=0 TSS64-busy
> GDT=     000000007f294000 00000047
> IDT=     000000007f294048 00000fff
> CR0=e0000011 CR2=0000000000000000 CR3=000000007ff80000 CR4=00000220
> DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000
> DR6=00000000ffff0ff0 DR7=0000000000000400
> EFER=0000000000000500
> Code=?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? <??> ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??
> RAX=0000000000000002 RBX=0000000000000000 RCX=000000007ffdbf48 RDX=000000007fede070
> RSI=0000000000000004 RDI=000000007fede058 RBP=0000000000000000 RSP=000000007e1a7000
> R8 =0000000000000000 R9 =0000000000000000 R10=0000000000000000 R11=0000000000000000
> R12=0000000000000000 R13=0000000000000000 R14=0000000000000000 R15=0000000000000000
> RIP=000000000009f0fd RFL=00010006 [-----P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> ES =0030 0000000000000000 ffffffff 00c09300 DPL=0 DS   [-WA]
> CS =0038 0000000000000000 ffffffff 00a09b00 DPL=0 CS64 [-RA]
> SS =0030 0000000000000000 ffffffff 00c09300 DPL=0 DS   [-WA]
> DS =0030 0000000000000000 ffffffff 00c09300 DPL=0 DS   [-WA]
> FS =0018 0000000000000000 ffffffff 00c09300 DPL=0 DS   [-WA]
> GS =0018 0000000000000000 ffffffff 00c09300 DPL=0 DS   [-WA]
> LDT=0000 0000000000000000 0000ffff 00008200 DPL=0 LDT
> TR =0000 0000000000000000 0000ffff 00008b00 DPL=0 TSS64-busy
> GDT=     000000007f294000 00000047
> IDT=     000000007f294048 00000fff
> CR0=e0000011 CR2=0000000000000000 CR3=000000007ff80000 CR4=00000220
> DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000
> DR6=00000000ffff0ff0 DR7=0000000000000400
> EFER=0000000000000500
> Code=?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? <??> ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??

  This happened 1 time out of the 2 times I ran the test.

* test 14: boot regression relative to test 4. The symptoms are
  identical to those seen in test 7, that is:

> SmmInstallProtocolInterface: [EfiSmmReadyToLockProtocol] 0
> Patch page table start ...
> Patch page table done!
> MemoryAttributesTable - NULL
> SetPageTableAttributes
> Start...
> ConvertPageEntryAttribute 0x7FF98067->0x7FF98065
>
> ASSERT_EFI_ERROR (Status = Invalid Parameter)
> ASSERT UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c(468): !EFI_ERROR (Status)


Summary:

- this series seems to break the boot with the Ia32 build of OVMF,
  regardless of PcdCpuSmmStaticPageTable (cases 7 and 8),

- it seems to break the boot with the Ia32X64 build of OVMF, with
  PcdCpuSmmStaticPageTable=TRUE (case 14),

- for the Ia32X64 build, with PcdCpuSmmStaticPageTable=FALSE, boot
  works, but S3 becomes unreliable (case 13),

- because there was no successful boot with
  PcdCpuSmmStaticPageTable=TRUE, I couldn't check if and how the
  increased SMRAM demand would impact the maximum count of bootable
  VCPUs. (For this test, I should likely increase the guest-phys address
  width from 36 bits anyway.)

Thanks
Laszlo