All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jarkko Sakkinen <jarkko@kernel.org>
To: linux-sgx@vger.kernel.org
Cc: dave.hansen@intel.com, Jarkko Sakkinen <jarkko@kernel.org>,
	stable@vger.kernel.org, Sean Christopherson <seanjc@google.com>,
	Haitao Huang <haitao.huang@linux.intel.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
	Jethro Beekman <jethro@fortanix.com>,
	linux-kernel@vger.kernel.org
Subject: [PATCH v5] x86/sgx: Fix use-after-free in sgx_mmu_notifier_release()
Date: Thu, 28 Jan 2021 14:58:23 +0200	[thread overview]
Message-ID: <20210128125823.18660-1-jarkko@kernel.org> (raw)

The most trivial example of a race condition can be demonstrated by this
sequence where mm_list contains just one entry:

CPU A                           CPU B
-> sgx_release()
                                -> sgx_mmu_notifier_release()
                                -> list_del_rcu()
                                <- list_del_rcu()
-> kref_put()
-> sgx_encl_release()
                                -> synchronize_srcu()
-> cleanup_srcu_struct()

A sequence similar to this has also been spotted in tests under high
stress:

[  +0.000008] WARNING: CPU: 3 PID: 7620 at kernel/rcu/srcutree.c:374 cleanup_srcu_struct+0xed/0x100

Albeit not spotted in the tests, it's also entirely possible that the
following scenario could happen:

CPU A                           CPU B
-> sgx_release()
                                -> sgx_mmu_notifier_release()
                                -> list_del_rcu()
-> kref_put()
-> sgx_encl_release()
-> cleanup_srcu_struct()
<- cleanup_srcu_struct()
                                -> synchronize_srcu()

This scenario would lead into use-after free in cleaup_srcu_struct().

Fix this by taking a reference to the enclave in
sgx_mmu_notifier_release().

Cc: stable@vger.kernel.org
Fixes: 1728ab54b4be ("x86/sgx: Add a page reclaimer")
Suggested-by: Sean Christopherson <seanjc@google.com>
Reported-by: Haitao Huang <haitao.huang@linux.intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
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/encl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index ee50a5010277..5ecbcf94ec2a 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -465,6 +465,7 @@ static void sgx_mmu_notifier_release(struct mmu_notifier *mn,
 	spin_lock(&encl_mm->encl->mm_lock);
 	list_for_each_entry(tmp, &encl_mm->encl->mm_list, list) {
 		if (tmp == encl_mm) {
+			kref_get(&encl_mm->encl->refcount);
 			list_del_rcu(&encl_mm->list);
 			break;
 		}
@@ -474,6 +475,7 @@ static void sgx_mmu_notifier_release(struct mmu_notifier *mn,
 	if (tmp == encl_mm) {
 		synchronize_srcu(&encl_mm->encl->srcu);
 		mmu_notifier_put(mn);
+		kref_put(&encl_mm->encl->refcount, sgx_encl_release);
 	}
 }
 
-- 
2.30.0


             reply	other threads:[~2021-01-28 12:59 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-28 12:58 Jarkko Sakkinen [this message]
2021-01-28 16:33 ` [PATCH v5] x86/sgx: Fix use-after-free in sgx_mmu_notifier_release() Dave Hansen
2021-01-30 19:20   ` Jarkko Sakkinen
2021-01-30 19:26     ` Jarkko Sakkinen
2021-02-03 15:46     ` Dave Hansen
2021-02-03 21:54       ` Jarkko Sakkinen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210128125823.18660-1-jarkko@kernel.org \
    --to=jarkko@kernel.org \
    --cc=bp@alien8.de \
    --cc=dave.hansen@intel.com \
    --cc=haitao.huang@linux.intel.com \
    --cc=hpa@zytor.com \
    --cc=jethro@fortanix.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sgx@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=seanjc@google.com \
    --cc=stable@vger.kernel.org \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.