This has been shown in tests:
[ +0.000008] WARNING: CPU: 3 PID: 7620 at kernel/rcu/srcutree.c:374 cleanup_srcu_struct+0xed/0x100
This is essentially a use-after free, although SRCU notices it as
an SRCU cleanup in an invalid context.
== Background ==
SGX has a data structure (struct sgx_encl_mm) which keeps per-mm SGX
metadata. This is separate from 'struct sgx_encl' because, in theory,
an enclave can be mapped from more than one mm. sgx_encl_mm includes
a pointer back to the sgx_encl.
This means that sgx_encl must have a longer lifetime than all of the
sgx_encl_mm's that point to it. That's usually the case: sgx_encl_mm
is freed only after the mmu_notifier is unregistered in sgx_release().
However, there's a race. If the process is exiting,
sgx_mmu_notifier_release() can be called in parallel with sgx_release()
instead of being called *by* it. The mmu_notifier path keeps encl_mm
alive past when sgx_encl can be freed. This inverts the lifetime rules
and means that sgx_mmu_notifier_release() can access a freed sgx_encl.
== Fix ==
Increase encl->refcount when encl_mm->encl is established. Release
this reference encl_mm is freed. This ensures that 'encl' outlives
'encl_mm'.
Fixes: 1728ab54b4be ("x86/sgx: Add a page reclaimer")
Cc: Dave Hansen <[email protected]
Reported-by: Haitao Huang <[email protected]>
Signed-off-by: Jarkko Sakkinen <[email protected]>
---
v8:
- Slight adjustments on call sites suggested by Dave, to make things
more clear and obvious. Otherwise, semantically same as v7:
https://lore.kernel.org/linux-sgx/[email protected]/
Contains also long description written by Dave.
v7:
- No changes from v6. Resend of
https://patchwork.kernel.org/project/intel-sgx/patch/[email protected]/
v6:
- Maintain refcount for each encl->mm_list entry.
v5:
- To make sure that the instance does not get deleted use kref_get()
kref_put(). This also removes the need for additional
synchronize_srcu().
v4:
- Rewrite the commit message.
- Just change the call order. *_expedited() is out of scope for this
bug fix.
v3: Fine-tuned tags, and added missing change log for v2.
v2: Switch to synchronize_srcu_expedited().
arch/x86/kernel/cpu/sgx/driver.c | 3 +++
arch/x86/kernel/cpu/sgx/encl.c | 5 +++++
2 files changed, 8 insertions(+)
diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
index f2eac41bb4ff..8ce6d8371cfb 100644
--- a/arch/x86/kernel/cpu/sgx/driver.c
+++ b/arch/x86/kernel/cpu/sgx/driver.c
@@ -72,6 +72,9 @@ static int sgx_release(struct inode *inode, struct file *file)
synchronize_srcu(&encl->srcu);
mmu_notifier_unregister(&encl_mm->mmu_notifier, encl_mm->mm);
kfree(encl_mm);
+
+ /* 'encl_mm' is gone, put encl_mm->encl reference: */
+ kref_put(&encl->refcount, sgx_encl_release);
}
kref_put(&encl->refcount, sgx_encl_release);
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 20a2dd5ba2b4..7449ef33f081 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -473,6 +473,9 @@ static void sgx_mmu_notifier_free(struct mmu_notifier *mn)
{
struct sgx_encl_mm *encl_mm = container_of(mn, struct sgx_encl_mm, mmu_notifier);
+ /* 'encl_mm' is going away, put encl_mm->encl reference: */
+ kref_put(&encl_mm->encl->refcount, sgx_encl_release);
+
kfree(encl_mm);
}
@@ -526,6 +529,8 @@ int sgx_encl_mm_add(struct sgx_encl *encl, struct mm_struct *mm)
if (!encl_mm)
return -ENOMEM;
+ /* Grab a refcount for the encl_mm->encl reference: */
+ kref_get(&encl->refcount);
encl_mm->encl = encl;
encl_mm->mm = mm;
encl_mm->mmu_notifier.ops = &sgx_mmu_notifier_ops;
--
2.30.0
> This has been shown in tests:
>
> [ +0.000008] WARNING: CPU: 3 PID: 7620 at kernel/rcu/srcutree.c:374 cleanup_srcu_struct+0xed/0x100
>
> This is essentially a use-after free, although SRCU notices it as
> an SRCU cleanup in an invalid context.
...
Acked-by: Dave Hansen <[email protected]
The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: 2ade0d60939bcd54197c133b03b460fe62a4ec47
Gitweb: https://git.kernel.org/tip/2ade0d60939bcd54197c133b03b460fe62a4ec47
Author: Jarkko Sakkinen <[email protected]>
AuthorDate: Mon, 08 Feb 2021 00:14:01 +02:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Mon, 08 Feb 2021 19:11:30 +01:00
x86/sgx: Maintain encl->refcount for each encl->mm_list entry
This has been shown in tests:
[ +0.000008] WARNING: CPU: 3 PID: 7620 at kernel/rcu/srcutree.c:374 cleanup_srcu_struct+0xed/0x100
This is essentially a use-after free, although SRCU notices it as
an SRCU cleanup in an invalid context.
== Background ==
SGX has a data structure (struct sgx_encl_mm) which keeps per-mm SGX
metadata. This is separate from struct sgx_encl because, in theory,
an enclave can be mapped from more than one mm. sgx_encl_mm includes
a pointer back to the sgx_encl.
This means that sgx_encl must have a longer lifetime than all of the
sgx_encl_mm's that point to it. That's usually the case: sgx_encl_mm
is freed only after the mmu_notifier is unregistered in sgx_release().
However, there's a race. If the process is exiting,
sgx_mmu_notifier_release() can be called in parallel with sgx_release()
instead of being called *by* it. The mmu_notifier path keeps encl_mm
alive past when sgx_encl can be freed. This inverts the lifetime rules
and means that sgx_mmu_notifier_release() can access a freed sgx_encl.
== Fix ==
Increase encl->refcount when encl_mm->encl is established. Release
this reference when encl_mm is freed. This ensures that encl outlives
encl_mm.
[ bp: Massage commit message. ]
Fixes: 1728ab54b4be ("x86/sgx: Add a page reclaimer")
Reported-by: Haitao Huang <[email protected]>
Signed-off-by: Jarkko Sakkinen <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Acked-by: Dave Hansen <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
arch/x86/kernel/cpu/sgx/driver.c | 3 +++
arch/x86/kernel/cpu/sgx/encl.c | 5 +++++
2 files changed, 8 insertions(+)
diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
index f2eac41..8ce6d83 100644
--- a/arch/x86/kernel/cpu/sgx/driver.c
+++ b/arch/x86/kernel/cpu/sgx/driver.c
@@ -72,6 +72,9 @@ static int sgx_release(struct inode *inode, struct file *file)
synchronize_srcu(&encl->srcu);
mmu_notifier_unregister(&encl_mm->mmu_notifier, encl_mm->mm);
kfree(encl_mm);
+
+ /* 'encl_mm' is gone, put encl_mm->encl reference: */
+ kref_put(&encl->refcount, sgx_encl_release);
}
kref_put(&encl->refcount, sgx_encl_release);
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index ee50a50..f65564a 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -481,6 +481,9 @@ static void sgx_mmu_notifier_free(struct mmu_notifier *mn)
{
struct sgx_encl_mm *encl_mm = container_of(mn, struct sgx_encl_mm, mmu_notifier);
+ /* 'encl_mm' is going away, put encl_mm->encl reference: */
+ kref_put(&encl_mm->encl->refcount, sgx_encl_release);
+
kfree(encl_mm);
}
@@ -534,6 +537,8 @@ int sgx_encl_mm_add(struct sgx_encl *encl, struct mm_struct *mm)
if (!encl_mm)
return -ENOMEM;
+ /* Grab a refcount for the encl_mm->encl reference: */
+ kref_get(&encl->refcount);
encl_mm->encl = encl;
encl_mm->mm = mm;
encl_mm->mmu_notifier.ops = &sgx_mmu_notifier_ops;
On Sun, 07 Feb 2021 16:14:01 -0600, Jarkko Sakkinen <[email protected]>
wrote:
> This has been shown in tests:
>
> [ +0.000008] WARNING: CPU: 3 PID: 7620 at kernel/rcu/srcutree.c:374
> cleanup_srcu_struct+0xed/0x100
>
> This is essentially a use-after free, although SRCU notices it as
> an SRCU cleanup in an invalid context.
>
The comments in code around this warning indicate a potential memory leak.
Not sure how use-after-free come into play. Anyway, this fix seems to work
for the warning above.
However, I still have doubts on another potential race. See below.
> diff --git a/arch/x86/kernel/cpu/sgx/driver.c
> b/arch/x86/kernel/cpu/sgx/driver.c
> index f2eac41bb4ff..8ce6d8371cfb 100644
> --- a/arch/x86/kernel/cpu/sgx/driver.c
> +++ b/arch/x86/kernel/cpu/sgx/driver.c
> @@ -72,6 +72,9 @@ static int sgx_release(struct inode *inode, struct
> file *file)
> synchronize_srcu(&encl->srcu);
> mmu_notifier_unregister(&encl_mm->mmu_notifier, encl_mm->mm);
> kfree(encl_mm);
Note here you are freeing the encl_mm, outside protection of
encl->refcount.
> +
> + /* 'encl_mm' is gone, put encl_mm->encl reference: */
> + kref_put(&encl->refcount, sgx_encl_release);
> }
> kref_put(&encl->refcount, sgx_encl_release);
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c
> b/arch/x86/kernel/cpu/sgx/encl.c
> index 20a2dd5ba2b4..7449ef33f081 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -473,6 +473,9 @@ static void sgx_mmu_notifier_free(struct
> mmu_notifier *mn)
> {
> struct sgx_encl_mm *encl_mm = container_of(mn, struct sgx_encl_mm,
> mmu_notifier);
> + /* 'encl_mm' is going away, put encl_mm->encl reference: */
> + kref_put(&encl_mm->encl->refcount, sgx_encl_release);
> +
> kfree(encl_mm);
Could this access to and kfree of encl_mm possibly be after the
kfree(encl_mm) noted above?
Also is there a reason we do kfree(encl_mm) in notifier_free not directly
in notifier_release?
Thanks
Haitao
On Tue, Apr 13, 2021, Haitao Huang wrote:
> On Sun, 07 Feb 2021 16:14:01 -0600, Jarkko Sakkinen <[email protected]>
> wrote:
>
> > This has been shown in tests:
> >
> > [ +0.000008] WARNING: CPU: 3 PID: 7620 at kernel/rcu/srcutree.c:374
> > cleanup_srcu_struct+0xed/0x100
> >
> > This is essentially a use-after free, although SRCU notices it as
> > an SRCU cleanup in an invalid context.
> >
> The comments in code around this warning indicate a potential memory leak.
> Not sure how use-after-free come into play. Anyway, this fix seems to work
> for the warning above.
>
> However, I still have doubts on another potential race. See below.
>
>
> > diff --git a/arch/x86/kernel/cpu/sgx/driver.c
> > b/arch/x86/kernel/cpu/sgx/driver.c
> > index f2eac41bb4ff..8ce6d8371cfb 100644
> > --- a/arch/x86/kernel/cpu/sgx/driver.c
> > +++ b/arch/x86/kernel/cpu/sgx/driver.c
> > @@ -72,6 +72,9 @@ static int sgx_release(struct inode *inode, struct
> > file *file)
> > synchronize_srcu(&encl->srcu);
> > mmu_notifier_unregister(&encl_mm->mmu_notifier, encl_mm->mm);
> > kfree(encl_mm);
>
> Note here you are freeing the encl_mm, outside protection of encl->refcount.
>
> > +
> > + /* 'encl_mm' is gone, put encl_mm->encl reference: */
> > + kref_put(&encl->refcount, sgx_encl_release);
> > }
> > kref_put(&encl->refcount, sgx_encl_release);
> > diff --git a/arch/x86/kernel/cpu/sgx/encl.c
> > b/arch/x86/kernel/cpu/sgx/encl.c
> > index 20a2dd5ba2b4..7449ef33f081 100644
> > --- a/arch/x86/kernel/cpu/sgx/encl.c
> > +++ b/arch/x86/kernel/cpu/sgx/encl.c
> > @@ -473,6 +473,9 @@ static void sgx_mmu_notifier_free(struct
> > mmu_notifier *mn)
> > {
> > struct sgx_encl_mm *encl_mm = container_of(mn, struct sgx_encl_mm,
> > mmu_notifier);
> > + /* 'encl_mm' is going away, put encl_mm->encl reference: */
> > + kref_put(&encl_mm->encl->refcount, sgx_encl_release);
> > +
> > kfree(encl_mm);
>
> Could this access to and kfree of encl_mm possibly be after the
> kfree(encl_mm) noted above?
No, the mmu_notifier_unregister() ensures that all in-progress notifiers complete
before it returns, i.e. SGX's notifier call back is not reachable after it's
unregistered.
> Also is there a reason we do kfree(encl_mm) in notifier_free not directly in
> notifier_release?
Because encl_mm is the anchor to the enclave reference
/* 'encl_mm' is going away, put encl_mm->encl reference: */
kref_put(&encl_mm->encl->refcount, sgx_encl_release);
as well as the mmu notifier reference (the mmu_notifier_put(mn) call chain).
Freeing encl_mm immediately would prevent sgx_mmu_notifier_free() from dropping
the enclave reference. And the mmu notifier reference need to be dropped in
sgx_mmu_notifier_release() because the encl_mm has been taken off encl->mm_list.
On 4/14/21 8:51 AM, Sean Christopherson wrote:
>> Could this access to and kfree of encl_mm possibly be after the
>> kfree(encl_mm) noted above?
> No, the mmu_notifier_unregister() ensures that all in-progress notifiers complete
> before it returns, i.e. SGX's notifier call back is not reachable after it's
> unregistered.
>
>> Also is there a reason we do kfree(encl_mm) in notifier_free not directly in
>> notifier_release?
> Because encl_mm is the anchor to the enclave reference
>
> /* 'encl_mm' is going away, put encl_mm->encl reference: */
> kref_put(&encl_mm->encl->refcount, sgx_encl_release);
>
> as well as the mmu notifier reference (the mmu_notifier_put(mn) call chain).
> Freeing encl_mm immediately would prevent sgx_mmu_notifier_free() from dropping
> the enclave reference. And the mmu notifier reference need to be dropped in
> sgx_mmu_notifier_release() because the encl_mm has been taken off encl->mm_list.
Haitao, I think you've highlighted that this locking scheme is woefully
under-documented. Any patches to beef it up would be very welcome.