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

[Laszlo Ersek <lersek@redhat.com>]

On 10/19/18 03:50, Jian J Wang wrote:
> The UAF (Use-After-Free) memory detection feature will cause an
> infinite calling of InitializePageTablePool(). 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()       |

This should likely be "SetMemorySpaceAttributes" (the DXE service), or else "CpuSetMemoryAttributes" (the underlying CpuDxe function name).

> => <out of page table>         |
> => InitializePageTablePool()   |
> => AllocateAlignedPages()      |
> => FreePages() ================|
> 
> The solution is add a lock in page table pool allocation function
> and fail any other requests if it has not been done.

OK, but what is the end result? InitializePageTablePool() will return FALSE. How far back up is that error propagated? To what components will the error be visible?

BTW, I've found the following comment in CpuSetMemoryAttributes():

  //
  // During memory attributes updating, new pages may be allocated to setup
  // smaller granularity of page table. Page allocation action might then cause
  // another calling of CpuSetMemoryAttributes() recursively, due to memory
  // protection policy configured (such as PcdDxeNxMemoryProtectionPolicy).
  // Since this driver will always protect memory used as page table by itself,
  // there's no need to apply protection policy requested from memory service.
  // So it's safe to just return EFI_SUCCESS if this time of calling is caused
  // by page table memory allocation.
  //

Is the current argument similar? I think it should be documented somehow.

> 
> 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/CpuPageTable.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index 33e8ee2d2c..2145e623fa 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -100,6 +100,7 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = {
>  };
>  
>  PAGE_TABLE_POOL                   *mPageTablePool = NULL;
> +EFI_LOCK                          mPageTablePoolLock = EFI_INITIALIZE_LOCK_VARIABLE (TPL_NOTIFY);

Why does this have to be an "EFI_LOCK"? Can't we just use a global variable? (I don't understand why messing with the TPL is necessary.)

In fact, I totally don't understand the point of EfiAcquireLock(). If we have two independent resources, each protected with its own separate lock, then why do both locks share the system-wide TPL between each other?


>  PAGE_TABLE_LIB_PAGING_CONTEXT     mPagingContext;
>  EFI_SMM_BASE2_PROTOCOL            *mSmmBase2 = NULL;
>  
> @@ -1045,6 +1046,12 @@ InitializePageTablePool (
>  {
>    VOID                      *Buffer;
>    BOOLEAN                   IsModified;
> +  EFI_STATUS                Status;
> +
> +  Status = EfiAcquireLockOrFail (&mPageTablePoolLock);
> +  if (EFI_ERROR (Status)) {
> +    return FALSE;
> +  }
>  
>    //
>    // Always reserve at least PAGE_TABLE_POOL_UNIT_PAGES, including one page for
> @@ -1056,7 +1063,10 @@ InitializePageTablePool (
>    Buffer = AllocateAlignedPages (PoolPages, PAGE_TABLE_POOL_ALIGNMENT);
>    if (Buffer == NULL) {
>      DEBUG ((DEBUG_ERROR, "ERROR: Out of aligned pages\r\n"));
> +    EfiReleaseLock (&mPageTablePoolLock);

I feel that it would be safer to introduce a "Done" label at the bottom of the function, and release the lock there.

(Again, I'm not sure why this has to be a "lock".)

>      return FALSE;
> +  } else {
> +    DEBUG ((DEBUG_INFO, "Paging: added %d pages to page table pool\r\n", PoolPages));

Please don't print UINTN values with %d. Cast them to UINT64 and log them with %lu.

>    }
>  
>    //
> @@ -1092,6 +1102,8 @@ InitializePageTablePool (
>      );
>    ASSERT (IsModified == TRUE);
>  
> +  EfiReleaseLock (&mPageTablePoolLock);
> +
>    return TRUE;
>  }
>  
> 

Thanks
Laszlo