Re: [PATCH v2 2/5] UefiCpuPkg/CpuDxe: fix an infinite loop issue

[Laszlo Ersek <lersek@redhat.com>]

On 10/23/18 16:53, Jian J Wang wrote:
>> v2 changes:
>> a. Change the type of mPageTablePoolLock to be BOOLEAN. Related code
>>    logic is also updated and refined.
>> b. Add non-stop mode for freed-memory guard feature
>
> The freed-memory guard feature will cause an infinite calling
> of InitializePageTablePool().

(1) An important statement is missing from the commit message. Namely,
under the UAF guard feature, FreePages() will modify page attributes.

Similarly, the subject line is mostly useless. First, we're not "fixing"
an issue: the issue doesn't exist yet. We're preventing the issue before
the next patches could introduce it. Second, "infinite loop" is way too
generic. Here's a suggestion:

  UefiCpuPkg/CpuDxe: prevent inf. recursion from FreePages->SetMemoryAttributes

(77 characters. A bit longer than I'd like (which is 74), but given the
long function names, it's hard to compress down the subject better.

Also, I don't recommend "inf. loop", we have recursion here, not a plain
loop.)


> This is due to a fact that
> AllocateAlignedPages() is used to allocate page table pool memory.
> This function will most likely call gBS->FreePages to free unaligned
> pages and then cause another round of page attributes change, like
> below
>
>    FreePages() <===============|
> => SetMemoryAttributes()       |
> => <if out of page table>      |
> => InitializePageTablePool()   |
> => AllocateAlignedPages()      |
> => FreePages() ================|
>
> The solution is add a global variable as a lock in page table pool
> allocation function and fail any other requests if it has not been
> done.

(2) Please document what the practical consequences are when the global
variable prevents the infinite recursion. As far as I can see, we are
going to propagate various error codes as far as possible outwards.
Questions:

- Will this error reach the caller (for example, a 3rd party UEFI
driver) if it checks the return status of gBS->FreePages()?

- What is the consequence for the UAF guard? Is it only that the freed
pages will not be marked inaccessible, or something more serious (like
some inconsistency in internal platform firmware structures)?


> This patch also add non-stop mode for freed-memory guard.

(3) Please isolate the update to the non-stop macro
(HEAP_GUARD_NONSTOP_MODE) to a separate patch.

- Non-stop mode was introduced / explained in commit 8f2613628acf
("MdeModulePkg/MdeModulePkg.dec: add new settings for PCDs",
2018-08-30).

- The macro HEAP_GUARD_NONSTOP_MODE was added to CpuDxe in commit
dcc026217fdc ("UefiCpuPkg/CpuDxe: implement non-stop mode for uefi",
2018-08-30).

- Another instance of the macro HEAP_GUARD_NONSTOP_MODE was added to
PiSmmCpuDxeSmm -- with BIT1|BIT0 replaced with BIT3|BIT2 -- in commit
09afd9a42a7f ("UefiCpuPkg/PiSmmCpuDxeSmm: implement non-stop mode for
SMM", 2018-08-30)

- According to the first patch in the present series, the UAF guard is
UEFI-only. (Well, "DXE-only" would be more precise; the point is, it
does not cover SMM.)

Thus, the fact that we don't modify the macro definition in
PiSmmCpuDxeSmm, and also *why* we don't do that, should be mentioned
explicitly in the commit message of the patch that modifies
HEAP_GUARD_NONSTOP_MODE in "UefiCpuPkg/CpuDxe/CpuDxe.h":

  UefiCpuPkg/CpuDxe: consider Use After Free guard in non-stop mode

(65 characters)


> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  UefiCpuPkg/CpuDxe/CpuDxe.h       |  2 +-
>  UefiCpuPkg/CpuDxe/CpuPageTable.c | 19 +++++++++++++++++--
>  2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.h b/UefiCpuPkg/CpuDxe/CpuDxe.h
> index 064ea05bba..3183a3f7f4 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.h
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.h
> @@ -58,7 +58,7 @@
>                                         )
>
>  #define HEAP_GUARD_NONSTOP_MODE       \
> -        ((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT6|BIT1|BIT0)) > BIT6)
> +        ((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT6|BIT4|BIT1|BIT0)) > BIT6)
>
>  #define NULL_DETECTION_NONSTOP_MODE   \
>          ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT6|BIT0)) > BIT6)
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index 33e8ee2d2c..b7beaf935b 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -100,6 +100,7 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = {
>  };
>
>  PAGE_TABLE_POOL                   *mPageTablePool = NULL;
> +BOOLEAN                           mPageTablePoolLock = FALSE;
>  PAGE_TABLE_LIB_PAGING_CONTEXT     mPagingContext;
>  EFI_SMM_BASE2_PROTOCOL            *mSmmBase2 = NULL;
>
> @@ -1046,6 +1047,16 @@ InitializePageTablePool (
>    VOID                      *Buffer;
>    BOOLEAN                   IsModified;
>
> +  //
> +  // Do not allow re-entrance.
> +  //
> +  if (mPageTablePoolLock) {
> +    return FALSE;
> +  }
> +
> +  mPageTablePoolLock = TRUE;
> +  IsModified = FALSE;
> +
>    //
>    // Always reserve at least PAGE_TABLE_POOL_UNIT_PAGES, including one page for
>    // header.
> @@ -1056,7 +1067,9 @@ InitializePageTablePool (
>    Buffer = AllocateAlignedPages (PoolPages, PAGE_TABLE_POOL_ALIGNMENT);
>    if (Buffer == NULL) {
>      DEBUG ((DEBUG_ERROR, "ERROR: Out of aligned pages\r\n"));
> -    return FALSE;
> +    goto Done;
> +  } else {
> +    DEBUG ((DEBUG_INFO, "Paging: added %ld pages to page table pool\r\n", (UINT64)PoolPages));
>    }

(4) Please don't add an "else" branch right after a "return" or "goto".
(This is a very annoying, and regrettably frequent, anti-pattern in
edk2.)

(5) Please print UINT64 values with "%lu", not "%ld".

(6) I think this message should be logged at the DEBUG_VERBOSE level. I
don't insist, but please consider it at least.


>    //
> @@ -1092,7 +1105,9 @@ InitializePageTablePool (
>      );
>    ASSERT (IsModified == TRUE);
>
> -  return TRUE;
> +Done:
> +  mPageTablePoolLock = FALSE;
> +  return IsModified;
>  }
>
>  /**
>

Looks OK to me otherwise.

Thanks,
Laszlo