Re: [PATCH 3/4] UefiCpuPkg/MtrrLib: Update algorithm to calculate optimal settings

[Laszlo Ersek <lersek@redhat.com>]

Hi Ray,

On 10/12/17 10:48, Ruiyu Ni wrote:
> The new algorithm converts the problem calculating optimal
> MTRR settings (using least MTRR registers) to the problem finding
> the shortest path in a graph.
> The memory required in extreme but rare case can be up to 256KB,
> so using local stack buffer is impossible considering current
> DxeIpl only allocates 128KB stack.
> 
> The patch changes existing MtrrSetMemoryAttributeInMtrrSettings() and
> MtrrSetMemoryAttribute() to use the 4-page stack buffer for
> calculation. The two APIs return BUFFER_TOO_SMALL when the buffer
> is too small for calculation.

[snip]

> +#define SCRATCH_BUFFER_SIZE           (4 * SIZE_4KB)

[snip]

>  RETURN_STATUS
>  EFIAPI
> -MtrrSetMemoryAttribute (
> +MtrrSetMemoryAttributeInMtrrSettings (
> +  IN OUT MTRR_SETTINGS       *MtrrSetting,
>    IN PHYSICAL_ADDRESS        BaseAddress,
>    IN UINT64                  Length,
>    IN MTRR_MEMORY_CACHE_TYPE  Attribute
>    )
>  {
>    RETURN_STATUS              Status;
> +  UINT8                      Scratch[SCRATCH_BUFFER_SIZE];

[snip]

(This patch is now commit 2bbd7e2fbd4b.)

Today I managed to spend time on

  https://bugzilla.tianocore.org/show_bug.cgi?id=747

(which is in turn based on the earlier mailing list thread

  [edk2] dynamic PCD impact on temporary PEI memory
  https://lists.01.org/pipermail/edk2-devel/2017-October/016213.html
).

While writing the patches, I found the root cause of BZ#747:
"OvmfPkg/PlatformPei" calls MtrrLib APIs, and due to the above 16KB
stack allocation, MtrrLib overflow's OVMF's 16KB (total) temp SEC/PEI
stack. Because the temp SEC/PEI heap is just below the stack, and the
stack grows down, this overflow by to the large Scratch array corrupts
the heap (for example, various HOBs).

Now, I'm fixing this for OVMF by enlarging its temp SEC/PEI RAM (the
patches are mostly ready for posting), but I have a different concern:

MtrrLib is MP-safe, meaning that it can be called from APs as well, for
setting up MTRRs on APs. (The Intel SDM basically requires all
processors to use the same MTRR settings, which must be configured on
all APs in parallel.) Hence it seems to me that the above Stack array
(16KB in size) must fit into the stack of *each* AP.

In particular I'm concerned about the UefiCpuPkg/PiSmmCpuDxeSmm driver.
In OVMF we have seen SMM stack overflow before. The following two
commits were added back then:

- 509f8425b75d ("UefiCpuPkg: change PcdCpuSmmStackGuard default to
TRUE", 2016-06-01)

- 0d0c245dfb14 ("OvmfPkg: set SMM stack size to 16KB", 2016-06-01)

However: the default SMM stack size (in "UefiCpuPkg.dec") remains 8KB
(PcdCpuSmmStackSize). Furthermore, the guard page can only catch
accesses that are *slightly* below the stack base address. If an
overflow is several pages out of bounds, then the wrong access will skip
over the guard page.

The same worry might apply, via MpInitLib, and the PcdCpuApStackSize
PCD, to:

- UefiCpuPkg/CpuMpPei/CpuMpPei.inf (produces the MP services PPI),
- UefiCpuPkg/CpuDxe/CpuDxe.inf (produces the MP services protocol).

(Although the default value for PcdCpuApStackSize is larger: 32KB).

Do we need to audit all the AP stacks to see if they can accommodate the
Scratch array?

Thanks
Laszlo