All of lore.kernel.org
 help / color / mirror / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: stable@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	patches@lists.linux.dev, Jordy Zomer <jordyzomer@google.com>,
	Linus Torvalds <torvalds@linuxfoundation.org>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Daniel Borkmann <daniel@iogearbox.net>
Subject: [PATCH 4.14 6/7] uaccess: Add speculation barrier to copy_from_user()
Date: Thu, 23 Feb 2023 14:04:43 +0100	[thread overview]
Message-ID: <20230223130423.662749238@linuxfoundation.org> (raw)
In-Reply-To: <20230223130423.369876969@linuxfoundation.org>

From: Dave Hansen <dave.hansen@linux.intel.com>

commit 74e19ef0ff8061ef55957c3abd71614ef0f42f47 upstream.

The results of "access_ok()" can be mis-speculated.  The result is that
you can end speculatively:

	if (access_ok(from, size))
		// Right here

even for bad from/size combinations.  On first glance, it would be ideal
to just add a speculation barrier to "access_ok()" so that its results
can never be mis-speculated.

But there are lots of system calls just doing access_ok() via
"copy_to_user()" and friends (example: fstat() and friends).  Those are
generally not problematic because they do not _consume_ data from
userspace other than the pointer.  They are also very quick and common
system calls that should not be needlessly slowed down.

"copy_from_user()" on the other hand uses a user-controller pointer and
is frequently followed up with code that might affect caches.  Take
something like this:

	if (!copy_from_user(&kernelvar, uptr, size))
		do_something_with(kernelvar);

If userspace passes in an evil 'uptr' that *actually* points to a kernel
addresses, and then do_something_with() has cache (or other)
side-effects, it could allow userspace to infer kernel data values.

Add a barrier to the common copy_from_user() code to prevent
mis-speculated values which happen after the copy.

Also add a stub for architectures that do not define barrier_nospec().
This makes the macro usable in generic code.

Since the barrier is now usable in generic code, the x86 #ifdef in the
BPF code can also go away.

Reported-by: Jordy Zomer <jordyzomer@google.com>
Suggested-by: Linus Torvalds <torvalds@linuxfoundation.org>
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>   # BPF bits
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 include/linux/nospec.h |    4 ++++
 lib/usercopy.c         |    7 +++++++
 2 files changed, 11 insertions(+)

--- a/include/linux/nospec.h
+++ b/include/linux/nospec.h
@@ -9,6 +9,10 @@
 
 struct task_struct;
 
+#ifndef barrier_nospec
+# define barrier_nospec() do { } while (0)
+#endif
+
 /**
  * array_index_mask_nospec() - generate a ~0 mask when index < size, 0 otherwise
  * @index: array element index
--- a/lib/usercopy.c
+++ b/lib/usercopy.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <linux/uaccess.h>
+#include <linux/nospec.h>
 
 /* out-of-line parts */
 
@@ -9,6 +10,12 @@ unsigned long _copy_from_user(void *to,
 	unsigned long res = n;
 	might_fault();
 	if (likely(access_ok(VERIFY_READ, from, n))) {
+		/*
+		 * Ensure that bad access_ok() speculation will not
+		 * lead to nasty side effects *after* the copy is
+		 * finished:
+		 */
+		barrier_nospec();
 		kasan_check_write(to, n);
 		res = raw_copy_from_user(to, from, n);
 	}



  parent reply	other threads:[~2023-02-23 13:05 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-02-23 13:04 [PATCH 4.14 0/7] 4.14.307-rc1 review Greg Kroah-Hartman
2023-02-23 13:04 ` [PATCH 4.14 1/7] wifi: rtl8xxxu: gen2: Turn on the rate control Greg Kroah-Hartman
2023-02-23 13:04 ` [PATCH 4.14 2/7] powerpc: dts: t208x: Mark MAC1 and MAC2 as 10G Greg Kroah-Hartman
2023-02-23 13:04 ` [PATCH 4.14 3/7] random: always mix cycle counter in add_latent_entropy() Greg Kroah-Hartman
2023-02-23 13:04 ` [PATCH 4.14 4/7] powerpc: dts: t208x: Disable 10G on MAC1 and MAC2 Greg Kroah-Hartman
2023-02-23 13:04 ` [PATCH 4.14 5/7] alarmtimer: Prevent starvation by small intervals and SIG_IGN Greg Kroah-Hartman
2023-02-23 13:04 ` Greg Kroah-Hartman [this message]
2023-02-24  9:13   ` [PATCH 4.14 6/7] uaccess: Add speculation barrier to copy_from_user() Daniel Borkmann
2023-02-24  9:56     ` Greg Kroah-Hartman
2023-02-23 13:04 ` [PATCH 4.14 7/7] wifi: mwifiex: Add missing compatible string for SD8787 Greg Kroah-Hartman
2023-02-24  4:27 ` [PATCH 4.14 0/7] 4.14.307-rc1 review Guenter Roeck
2023-02-24 13:27 ` Naresh Kamboju
2023-02-24 17:50 ` Slade Watkins

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=20230223130423.662749238@linuxfoundation.org \
    --to=gregkh@linuxfoundation.org \
    --cc=daniel@iogearbox.net \
    --cc=dave.hansen@linux.intel.com \
    --cc=jordyzomer@google.com \
    --cc=patches@lists.linux.dev \
    --cc=stable@vger.kernel.org \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    --cc=torvalds@linuxfoundation.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.