Laszlo, Thanks for the comments.
Regards, Jian > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Friday, October 19, 2018 7:46 PM > To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org > Cc: Zeng, Star <star.z...@intel.com>; Kinney, Michael D > <michael.d.kin...@intel.com>; Yao, Jiewen <jiewen....@intel.com>; Ni, Ruiyu > <ruiyu...@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 <ler...@redhat.com> > > Cc: Star Zeng <star.z...@intel.com> > > Cc: Michael D Kinney <michael.d.kin...@intel.com> > > Cc: Jiewen Yao <jiewen....@intel.com> > > Cc: Ruiyu Ni <ruiyu...@intel.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Jian J Wang <jian.j.w...@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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
