On 2/5/21 10:28 AM, Jarkko Sakkinen 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
>
> There are two functions that drain encl->mm_list:
>
> - sgx_release() (i.e. VFS release) removes the remaining mm_list entries.
> - sgx_mmu_notifier_release() removes mm_list entry for the registered
> process, if it still exists.
Jarkko, I like your approach. This actually has the potential to be a
lot more understandable than the fix we settled on before.
But I think the explanation needs some tweaking, and I think I can take
it a step further to make it even more straightforward. The issue here
isn't *really* mm_list, it's this:
encl_mm->encl = encl;
That literally establishes a encl_mm to encl reference and needs a
reference count. That reference remains until 'encl_mm' is freed. I
don't think mm_list needs to even be taken into account.
The most straightforward way to fix this is to take a refcount at
"encl_mm->encl = encl" and release it at kfree(encl_mm). That makes a
*lot* of logical sense to me, and it's also trivial to audit.
Totally untested patch attached (adapted directly from yours).
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 <dave.hansen@xxxxxxxxxxxxxxx
Reported-by: Haitao Huang <haitao.huang@xxxxxxxxxxxxxxx>
Signed-off-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx>
---
b/arch/x86/kernel/cpu/sgx/driver.c | 3 +++
b/arch/x86/kernel/cpu/sgx/encl.c | 5 +++++
2 files changed, 8 insertions(+)
diff -puN arch/x86/kernel/cpu/sgx/driver.c~raw arch/x86/kernel/cpu/sgx/driver.c
--- a/arch/x86/kernel/cpu/sgx/driver.c~raw 2021-02-05 10:52:47.484545869 -0800
+++ b/arch/x86/kernel/cpu/sgx/driver.c 2021-02-05 10:59:06.497544923 -0800
@@ -72,6 +72,9 @@ static int sgx_release(struct inode *ino
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 -puN arch/x86/kernel/cpu/sgx/encl.c~raw arch/x86/kernel/cpu/sgx/encl.c
--- a/arch/x86/kernel/cpu/sgx/encl.c~raw 2021-02-05 10:52:47.486545869 -0800
+++ b/arch/x86/kernel/cpu/sgx/encl.c 2021-02-05 11:23:06.674541332 -0800
@@ -481,6 +481,9 @@ static void sgx_mmu_notifier_free(struct
{
struct sgx_encl_mm *encl_mm = container_of(mn, struct sgx_encl_mm, mmu_notifier);
+ /* 'encl_mm' is goin 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 *enc
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;
_
