archive mirror
 help / color / mirror / Atom feed
From: Dave Hansen <>
To: Jarkko Sakkinen <>,
Cc:, Sean Christopherson <>,
	Haitao Huang <>,
	Thomas Gleixner <>,
	Ingo Molnar <>, Borislav Petkov <>,, "H. Peter Anvin" <>,
	Jethro Beekman <>,
Subject: Re: [PATCH v5] x86/sgx: Fix use-after-free in sgx_mmu_notifier_release()
Date: Thu, 28 Jan 2021 08:33:55 -0800	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On 1/28/21 4:58 AM, Jarkko Sakkinen wrote:
> 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()

This is missing some key details including a clear, unambiguous, problem
statement.  To me, the patch should concentrate on the SRCU warning
since that's where we started.  Here's the detail that needs to be added
about the issue and the locking in general in this path:

sgx_release() also does this:

	mmu_notifier_unregister(&encl_mm->mmu_notifier, encl_mm->mm);

which does another synchronize_srcu() on the mmu_notifier's srcu_struct.
 *But*, it only does this if its own list_del_rcu() is successful.  It
does all of this before the kref_put().

In other words, sgx_release() can *only* get to this buggy path if
sgx_mmu_notifier_release() races with sgx_release and does a
list_del_rcu() first.

The key to this patch is that the sgx_mmu_notifier_release() will now
take an 'encl' reference in that case, which prevents kref_put() from
calling sgx_release() which cleans up and frees 'encl'.

I was actually also hoping to see some better comments about the new
refcount, and the locking in general.  There are *TWO* struct_srcu's in
play, a spinlock and a refcount.  I took me several days with Sean and
your help to identify the actual path and get a proper fix (versions 1-4
did *not* fix the race).

Also, the use-after-free is *fixed* in sgx_mmu_notifier_release() but
does not *occur* in sgx_mmu_notifier_release().  The subject here is a
bit misleading in that regard.

  reply	other threads:[~2021-01-28 16:36 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-28 12:58 [PATCH v5] x86/sgx: Fix use-after-free in sgx_mmu_notifier_release() Jarkko Sakkinen
2021-01-28 16:33 ` Dave Hansen [this message]
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:

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

  git send-email \ \ \ \ \ \ \ \ \ \ \ \ \ \ \

* 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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).