Re: [PATCH v4 0/7] Implement heap guard feature

[Laszlo Ersek <lersek@redhat.com>]

Hi Jian,

On 10/27/17 08:11, Jian J Wang wrote:
>> Path V4 changes:
>> a. Change names of gEdkiiSmmMemoryAttributeProtocolGuid related
>>    definitions from EFI_ to EDKII_
>> b. Coding style cleanup
>> c. Split patches in a more reasonable order and groups
> 
>> Patch V3 changes:
>> a. Add new protocol gEdkiiSmmMemoryAttributeProtocolGuid to do
>>    memory attributes update instead of doing it directly in SmmCore
>> b. Fix GCC build error
> 
>> Patch V2 changes:
>> a. Remove local variable initializer with memory copy from globals
>> b. Change map table dump code to use DEBUG_PAGE|DEBUG_POOL level
>>    message 
>> c. Fix malfunction in 32-bit boot mode
>> d. Add comment for the use of mOnGuarding
>> e. Change name of function InitializePageTableLib to 
>>    InitializePageTableGlobals
>> f. Add code in 32-bit code to bypass setting page table to read-only
>> g. Coding style clean-up
>>
> 
> This feature makes use of paging mechanism to add a hidden (not present)
> page just before and after the allocated memory block. If the code tries
> to access memory outside of the allocated part, page fault exception will
> be triggered.
> 
> This feature is disabled by default and is not recommended to enable it
> in production build of BIOS.
> 
> This patch has passed following validations:
> 
>   a. Boot to shell (OVMF, Intel real platform)(32/64)
>   b. Boot to Fedora 25 (64)
> 
> NT32 emulation platform was not validated with this feature enabled
> due to the fact that it doesn't support paging which is needed for
> this feature to work. But all are validated with feature is disabled.
> 
> Suggested-by: Ayellet Wolman <ayellet.wolman@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> 
> Jian J Wang (7):
>   MdeModulePkg/MdeModulePkg.dec,.uni: Add Protocol, PCDs and string
>     tokens
>   MdeModulePkg/SmmMemoryAttribute.h: Add new protocol definitions
>   UefiCpuPkg/CpuDxe: Reduce debug message
>   MdeModulePkg/DxeIpl: Enable paging for heap guard
>   MdeModulePkg/DxeCore: Implement heap guard feature for UEFI
>   UefiCpuPkg/PiSmmCpuDxeSmm: Add SmmMemoryAttribute protocol
>   MdeModulePkg/PiSmmCore: Implement heap guard feature for SMM mode
> 
>  MdeModulePkg/Core/Dxe/DxeMain.inf                  |    4 +
>  MdeModulePkg/Core/Dxe/Mem/HeapGuard.c              | 1182 ++++++++++++++++
>  MdeModulePkg/Core/Dxe/Mem/HeapGuard.h              |  394 ++++++
>  MdeModulePkg/Core/Dxe/Mem/Imem.h                   |   38 +-
>  MdeModulePkg/Core/Dxe/Mem/Page.c                   |  130 +-
>  MdeModulePkg/Core/Dxe/Mem/Pool.c                   |  154 +-
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf            |    1 +
>  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c    |   36 +-
>  MdeModulePkg/Core/PiSmmCore/HeapGuard.c            | 1467 ++++++++++++++++++++
>  MdeModulePkg/Core/PiSmmCore/HeapGuard.h            |  398 ++++++
>  MdeModulePkg/Core/PiSmmCore/Page.c                 |   52 +-
>  MdeModulePkg/Core/PiSmmCore/PiSmmCore.c            |    7 +-
>  MdeModulePkg/Core/PiSmmCore/PiSmmCore.h            |   81 +-
>  MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf          |    8 +
>  MdeModulePkg/Core/PiSmmCore/Pool.c                 |   81 +-
>  MdeModulePkg/Include/Protocol/SmmMemoryAttribute.h |  136 ++
>  MdeModulePkg/MdeModulePkg.dec                      |   60 +
>  MdeModulePkg/MdeModulePkg.uni                      |   58 +
>  UefiCpuPkg/CpuDxe/CpuPageTable.c                   |    5 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c           |   10 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c         |   20 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |   98 ++
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf       |    2 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c |  163 +++
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c            |   10 +-
>  25 files changed, 4496 insertions(+), 99 deletions(-)
>  create mode 100644 MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
>  create mode 100644 MdeModulePkg/Core/Dxe/Mem/HeapGuard.h
>  create mode 100644 MdeModulePkg/Core/PiSmmCore/HeapGuard.c
>  create mode 100644 MdeModulePkg/Core/PiSmmCore/HeapGuard.h
>  create mode 100644 MdeModulePkg/Include/Protocol/SmmMemoryAttribute.h
> 

I applied your patches on top of edk2 master (76fd5a660d70,
"MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at
ExitBootServices()", 2017-10-26), and regression-tested the following
platforms:

* ArmVirtQemu, AARCH64, QEMU TCG (x86_64) host, Fedora 25 Server guest,
normal boot. (I figured I'd test this because the DXE_CORE modifications
affect aarch64 too.)

* OVMF, IA32, QEMU/KVM, Fedora 25 guest, Q35 machine type, SMM. Normal
boot and S3. UEFI variable access test.

* OVMF, IA32X64, QEMU/KVM, Fedora 26 guest, Q35 machine type, SMM.
Normal boot and S3. UEFI variable access test. "multiprocessing" test.

* OVMF, IA32X64, QEMU/KVM, Windows 10 guest, Q35 machine type, SMM.
Normal boot and S3.

* OVMF, X64, QEMU/KVM, Fedora 26 guest, i440fx machine type, no SMM.
Normal boot and S3. "multiprocessing" test.

The test case references are from
<https://github.com/tianocore/tianocore.github.io/wiki/Testing-SMM-with-QEMU,-KVM-and-libvirt>.

I didn't encounter any regressions.

For the series:

Regression-tested-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo