public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Laszlo Ersek <lersek@redhat.com>
To: edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Jordan Justen <jordan.l.justen@intel.com>,
	Ruiyu Ni <ruiyu.ni@intel.com>
Subject: [PATCH 4/4] OvmfPkg: restore temporary SEC/PEI RAM size to 64KB
Date: Fri, 10 Nov 2017 16:49:08 +0100	[thread overview]
Message-ID: <20171110154908.306-5-lersek@redhat.com> (raw)
In-Reply-To: <20171110154908.306-1-lersek@redhat.com>

(1) In the PEI phase, the PCD database is maintained in a GUID HOB. In
    OVMF, we load the PCD PEIM before any other PEIMs (using APRIORI PEI),
    so that all other PEIMs can use dynamic PCDs. Consequently,

    - the PCD GUID HOB is initially allocated from the temporary SEC/PEI
      heap,

    - whenever we introduce a dynamic PCD to a PEIM built into OVMF such
      that the PCD is new to OVMF's whole PEI phase, the PCD GUID HOB (and
      its temporary heap footprint) grow.

    I've noticed that, if we add just one more dynamic PCD to the PEI
    phase, then in the X64 build,

    - we get very close to the half of the temporary heap (i.e., 8192
      bytes),

    - obscure PEI phase hangs or DXE core initialization failures
      (ASSERTs) occur. The symptoms vary between the FD_SIZE_2MB and
      FD_SIZE_4MB builds of X64 OVMF.

(2) I've found that commit

      2bbd7e2fbd4b ("UefiCpuPkg/MtrrLib: Update algorithm to calculate
                    optimal settings", 2017-09-27)

    introduced a large (16KB) stack allocation:

> The patch changes existing MtrrSetMemoryAttributeInMtrrSettings() and
> MtrrSetMemoryAttribute() to use the 4-page stack buffer for calculation.
> ...
> +#define SCRATCH_BUFFER_SIZE           (4 * SIZE_4KB)
> ...
> @@ -2207,17 +2462,66 @@ MtrrSetMemoryAttributeInMtrrSettings (
> ...
> +  UINT8                      Scratch[SCRATCH_BUFFER_SIZE];

(3) OVMF's temp SEC/PEI RAM size has been 32KB ever since commit

      7cb6b0e06809 ("OvmfPkg: Move SEC/PEI Temporary RAM from 0x70000 to
                    0x810000", 2014-01-21)

    Of that, the upper 16KB half is stack (growing down), and the lower
    16KB half is heap.

    Thus, OvmfPkg/PlatformPei's calls to "UefiCpuPkg/Library/MtrrLib", in
    QemuInitializeRam(), cause the Scratch array to overflow the entire
    stack (heading towards lower addresses), and corrupt the heap below
    the stack. It turns out that the total stack demand is about 24KB, so
    the overflow is able to corrupt the upper 8KB of the heap. If that
    part of the heap is actually used (for example because we grow the PCD
    GUID HOB sufficiently), mayhem ensues.

(4) Right after commit 7cb6b0e06809 (see above), there would be no room
    left above the 32KB temp SEC/PEI RAM. However, given more recent
    commits

      45d870815156 ("OvmfPkg/PlatformPei: rebase and resize the permanent
                    PEI memory for S3", 2016-07-13)

      6b04cca4d697 ("OvmfPkg: remove PcdS3AcpiReservedMemoryBase,
                    PcdS3AcpiReservedMemorySize", 2016-07-12)

    we can now restore the temp SEC/PEI RAM size to the original
    (pre-7cb6b0e06809) 64KB. This will allow for a 32KB temp SEC/PEI
    stack, which accommodates the ~24KB demand mentioned in (3).

    (Prior patches in this series will let us monitor the stack usage in
    the future.)

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=747
Ref: http://mid.mail-archive.com/a49cc089-12ae-a887-a4d6-4dc509233a74@redhat.com
Ref: http://mid.mail-archive.com/03e369bb-77c4-0134-258f-bdae62cbc8c5@redhat.com
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/OvmfPkgIa32.fdf    | 2 +-
 OvmfPkg/OvmfPkgIa32X64.fdf | 2 +-
 OvmfPkg/OvmfPkgX64.fdf     | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
index 751522411857..06a439f8cba5 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -82,7 +82,7 @@ [FD.MEMFD]
 0x007000|0x001000
 gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
 
-0x010000|0x008000
+0x010000|0x010000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
 
 0x020000|0x0E0000
diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
index f1a2044fb716..ced4c5639f39 100644
--- a/OvmfPkg/OvmfPkgIa32X64.fdf
+++ b/OvmfPkg/OvmfPkgIa32X64.fdf
@@ -82,7 +82,7 @@ [FD.MEMFD]
 0x007000|0x001000
 gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
 
-0x010000|0x008000
+0x010000|0x010000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
 
 0x020000|0x0E0000
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index 32000a3b934c..62dd58f6e47a 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -82,7 +82,7 @@ [FD.MEMFD]
 0x007000|0x001000
 gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
 
-0x010000|0x008000
+0x010000|0x010000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
 
 0x020000|0x0E0000
-- 
2.14.1.3.gb7cf6e02401b



  parent reply	other threads:[~2017-11-10 15:45 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-10 15:49 [PATCH 0/4] OvmfPkg: measure temp stack usage, restore temp RAM to 64KB Laszlo Ersek
2017-11-10 15:49 ` [PATCH 1/4] OvmfPkg/Sec/Ia32: free up EAX for other uses while setting up the stack Laszlo Ersek
2017-11-13 18:08   ` Jordan Justen
2017-11-13 18:30     ` Laszlo Ersek
2017-11-10 15:49 ` [PATCH 2/4] OvmfPkg/Sec/Ia32: seed the temporary RAM with PcdInitValueInTempStack Laszlo Ersek
2017-11-10 15:56   ` Ard Biesheuvel
2017-11-10 18:11     ` Laszlo Ersek
2017-11-10 18:27       ` Laszlo Ersek
2017-11-11  9:10       ` Ard Biesheuvel
2017-11-13 18:25   ` Jordan Justen
2017-11-13 18:36     ` Laszlo Ersek
2017-11-13 19:02       ` Jordan Justen
2017-11-13 20:58         ` Laszlo Ersek
2017-11-10 15:49 ` [PATCH 3/4] OvmfPkg/Sec/X64: " Laszlo Ersek
2017-11-10 15:49 ` Laszlo Ersek [this message]
2017-11-11  9:14 ` [PATCH 0/4] OvmfPkg: measure temp stack usage, restore temp RAM to 64KB Ard Biesheuvel
2017-11-11 20:38 ` Jordan Justen
2017-11-11 22:04   ` Jordan Justen
2017-11-12 10:58     ` Ard Biesheuvel
2017-11-13  9:08       ` Jordan Justen
2017-11-13 10:09         ` Ard Biesheuvel
2017-11-13 12:34           ` Laszlo Ersek
2017-11-13 13:09             ` Laszlo Ersek
2017-11-13 18:05               ` Jordan Justen
2017-11-13 18:04             ` Jordan Justen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20171110154908.306-5-lersek@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox