linux-sgx.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jarkko Sakkinen <jarkko@kernel.org>
To: Borislav Petkov <bp@alien8.de>
Cc: x86@kernel.org, linux-kernel@vger.kernel.org,
	linux-sgx@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>, "H. Peter Anvin" <hpa@zytor.com>,
	Jethro Beekman <jethro@fortanix.com>
Subject: Re: [PATCH v3] x86/sgx: Synchronize encl->srcu in sgx_encl_release().
Date: Tue, 12 Jan 2021 02:08:10 +0200	[thread overview]
Message-ID: <X/zoarV7gd/LNo4A@kernel.org> (raw)
In-Reply-To: <20210105145749.GF28649@zn.tnic>

On Tue, Jan 05, 2021 at 03:57:49PM +0100, Borislav Petkov wrote:
> On Wed, Dec 16, 2020 at 03:49:20PM +0200, Jarkko Sakkinen wrote:
> > Add synchronize_srcu_expedited() to sgx_encl_release() to catch a grace
> > period initiated by sgx_mmu_notifier_release().
> > 
> > A trivial example of a failing sequence with tasks A and B:
> > 
> > 1. A: -> sgx_release()
> > 2. B: -> sgx_mmu_notifier_release()
> > 3. B: -> list_del_rcu()
> > 3. A: -> sgx_encl_release()
> > 4. A: -> cleanup_srcu_struct()
> > 
> > The loop in sgx_release() observes an empty list because B has removed its
> > entry in the middle, and calls cleanup_srcu_struct() before B has a chance
> > to calls synchronize_srcu().
> 
> Leading to what? NULL ptr?
> 
> https://lkml.kernel.org/r/X9e2jOWz1hfXVpQ5@google.com
> 
> already suggested that you should explain the bug better and add the
> splat but I'm still missing that explanation.

OK, I'll try to explain it how I understand the issue.

Consider this loop in the VFS release hook (sgx_release):

	/*
	 * Drain the remaining mm_list entries. At this point the list contains
	 * entries for processes, which have closed the enclave file but have
	 * not exited yet. The processes, which have exited, are gone from the
	 * list by sgx_mmu_notifier_release().
	 */
	for ( ; ; )  {
		spin_lock(&encl->mm_lock);

		if (list_empty(&encl->mm_list)) {
			encl_mm = NULL;
		} else {
			encl_mm = list_first_entry(&encl->mm_list,
						   struct sgx_encl_mm, list);
			list_del_rcu(&encl_mm->list);
		}

		spin_unlock(&encl->mm_lock);

		/* The enclave is no longer mapped by any mm. */
		if (!encl_mm)
			break;

		synchronize_srcu(&encl->srcu);
		mmu_notifier_unregister(&encl_mm->mmu_notifier, encl_mm->mm);
		kfree(encl_mm);
	}


At this point all processes have closed the enclave file, but that doesn't
mean that they all have exited yet.

Now, let's imagine that there is exactly one entry in the encl->mm_list.
and sgx_release() execution gets scheduled right after returning from
synchronize_srcu().

With some bad luck, some process comes and removes that last entry befoe
sgx_release() acquires mm_lock. The loop in sgx_release() just leaves

		/* The enclave is no longer mapped by any mm. */
		if (!encl_mm)
			break;

No synchronize_srcu().

After writing this, I think that the placement for synchronize_srcu()
in this patch is not best possible. It should be rather that the
above loop would also call synchronize_srcu() when leaving.

I.e. the code change would result:

	for ( ; ; )  {
		spin_lock(&encl->mm_lock);

		if (list_empty(&encl->mm_list)) {
			encl_mm = NULL;
		} else {
			encl_mm = list_first_entry(&encl->mm_list,
						   struct sgx_encl_mm, list);
			list_del_rcu(&encl_mm->list);
		}

		spin_unlock(&encl->mm_lock);

                /* 
                 * synchronize_srcu() is mandatory *even* when the list was
                 * empty, in order make sure that grace periods stays in
                 * sync even when another task took away the last entry
                 * (i.e. exiting process when it deletes its mm_list).
                 */
		synchronize_srcu(&encl->srcu);

		/* The enclave is no longer mapped by any mm. */
		if (!encl_mm)
			break;

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

What do you think? Does this start to make more sense now?
I don't have logs for this but the bug can be also reasoned.

/Jarkko

  reply	other threads:[~2021-01-12  0:27 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-16 13:49 [PATCH v3] x86/sgx: Synchronize encl->srcu in sgx_encl_release() Jarkko Sakkinen
2021-01-04 20:22 ` Haitao Huang
2021-01-11 23:41   ` Jarkko Sakkinen
2021-01-05 14:57 ` Borislav Petkov
2021-01-12  0:08   ` Jarkko Sakkinen [this message]
2021-01-12 18:35     ` Borislav Petkov
2021-01-12 20:11       ` Paul E. McKenney
2021-01-13 17:18       ` Jarkko Sakkinen
2021-01-13 17:46         ` Paul E. McKenney
2021-01-13 18:00           ` Borislav Petkov
2021-01-13 18:22             ` Paul E. McKenney
2021-01-15  1:49           ` Jarkko Sakkinen
2021-01-14  4:42     ` Haitao Huang
2021-01-15  9:58       ` 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=X/zoarV7gd/LNo4A@kernel.org \
    --to=jarkko@kernel.org \
    --cc=bp@alien8.de \
    --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=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 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).