Re: [edk2] [PATCH 2/3] UefiCpuPkg/CpuDxe: fix an infinite loop issue
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 ; edk2-devel@lists.01.org
> Cc: Zeng, Star ; Kinney, Michael D
> ; Yao, Jiewen ; Ni, Ruiyu
>
> 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.
> > => |
> > => 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
> > Cc: Star Zeng
> > Cc: Michael D Kinney
> > Cc: Jiewen Yao
> > Cc: Ruiyu Ni
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang
> > ---
> > 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_STATUSStatus;
> > +
> > + Status = EfiAcquireLockOrFail ();
> > + 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 ();
>
> 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
Re: [edk2] [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).
> => |
> => 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
> Cc: Star Zeng
> Cc: Michael D Kinney
> Cc: Jiewen Yao
> Cc: Ruiyu Ni
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang
> ---
> 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_STATUSStatus;
> +
> + Status = EfiAcquireLockOrFail ();
> + 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 ();
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 ();
> +
>return TRUE;
> }
>
>
Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH 2/3] UefiCpuPkg/CpuDxe: fix an infinite loop issue
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() |
=> |
=> 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.
Cc: Laszlo Ersek
Cc: Star Zeng
Cc: Michael D Kinney
Cc: Jiewen Yao
Cc: Ruiyu Ni
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang
---
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);
PAGE_TABLE_LIB_PAGING_CONTEXT mPagingContext;
EFI_SMM_BASE2_PROTOCOL*mSmmBase2 = NULL;
@@ -1045,6 +1046,12 @@ InitializePageTablePool (
{
VOID *Buffer;
BOOLEAN IsModified;
+ EFI_STATUSStatus;
+
+ Status = EfiAcquireLockOrFail ();
+ 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 ();
return FALSE;
+ } else {
+DEBUG ((DEBUG_INFO, "Paging: added %d pages to page table pool\r\n",
PoolPages));
}
//
@@ -1092,6 +1102,8 @@ InitializePageTablePool (
);
ASSERT (IsModified == TRUE);
+ EfiReleaseLock ();
+
return TRUE;
}
--
2.16.2.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
