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

["Wang, Jian J" <jian.j.wang@intel.com>]

Laszlo,

Thanks for the comments.

Regards,
Jian


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, October 19, 2018 7:46 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu
> <ruiyu.ni@intel.com>
> Subject: Re: [PATCH 2/3] UefiCpuPkg/CpuDxe: fix an infinite loop issue
> 
> 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).
> 

You're right. I'll change it.

> > => <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.
> 

No, I don't think they're the similar. The issue I encountered here is that the code
tries to set freed memory as not-present but trapped in dead loop. The only
consequence here is that the freed pages in AllocateAlignedPages() cannot be
set as not-present. But it's ok because they're just allocated and haven't been
used by any other code.

> >
> > 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?
> 

Maybe you're right. Lock is a bit overkill. I'll try a global to find out if it's ok.

> 
> >  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".)
> 

Agree. I'll update this part of logic.

> >      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.
> 

You got me again. That's a shame:( But thanks for point it out.

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