All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: torvalds@linuxfoundation.org
Cc: x86@kernel.org, davem@davemloft.net, daniel@iogearbox.net,
	andrii@kernel.org, peterz@infradead.org, keescook@chromium.org,
	tglx@linutronix.de, hsinweih@uci.edu, rostedt@goodmis.org,
	vegard.nossum@oracle.com, gregkh@linuxfoundation.org,
	alan.maguire@oracle.com, dylany@meta.com, riel@surriel.com,
	bpf@vger.kernel.org, kernel-team@fb.com
Subject: [PATCH bpf 1/2] mm: Fix copy_from_user_nofault().
Date: Tue, 17 Jan 2023 21:14:42 -0800	[thread overview]
Message-ID: <20230118051443.78988-1-alexei.starovoitov@gmail.com> (raw)

From: Alexei Starovoitov <ast@kernel.org>

There are several issues with copy_from_user_nofault():

- access_ok() is designed for user context only and for that reason
it has WARN_ON_IN_IRQ() which triggers when bpf, kprobe, eprobe
and perf on ppc are calling it from irq.

- it's missing nmi_uaccess_okay() which is a nop on all architectures
except x86 where it's required.
The comment in arch/x86/mm/tlb.c explains the details why it's necessary.
Calling copy_from_user_nofault() from bpf, [ke]probe without this check is not safe.

- __copy_from_user_inatomic() under CONFIG_HARDENED_USERCOPY is calling
check_object_size()->__check_object_size()->check_heap_object()->find_vmap_area()->spin_lock()
which is not safe to do from bpf, [ke]probe and perf due to potential deadlock.

Fix all three issues. At the end the copy_from_user_nofault() becomes
equivalent to copy_from_user_nmi() from safety point of view with
a difference in the return value.

Reported-by: Hsin-Wei Hung <hsinweih@uci.edu>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 mm/maccess.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/mm/maccess.c b/mm/maccess.c
index 074f6b086671..6ee9b337c501 100644
--- a/mm/maccess.c
+++ b/mm/maccess.c
@@ -5,6 +5,7 @@
 #include <linux/export.h>
 #include <linux/mm.h>
 #include <linux/uaccess.h>
+#include <asm/tlb.h>
 
 bool __weak copy_from_kernel_nofault_allowed(const void *unsafe_src,
 		size_t size)
@@ -113,11 +114,18 @@ long strncpy_from_kernel_nofault(char *dst, const void *unsafe_addr, long count)
 long copy_from_user_nofault(void *dst, const void __user *src, size_t size)
 {
 	long ret = -EFAULT;
-	if (access_ok(src, size)) {
-		pagefault_disable();
-		ret = __copy_from_user_inatomic(dst, src, size);
-		pagefault_enable();
-	}
+
+	if (!__access_ok(src, size))
+		return ret;
+
+	if (!nmi_uaccess_okay())
+		return ret;
+
+	pagefault_disable();
+	instrument_copy_from_user_before(dst, src, size);
+	ret = raw_copy_from_user(dst, src, size);
+	instrument_copy_from_user_after(dst, src, size, ret);
+	pagefault_enable();
 
 	if (ret)
 		return -EFAULT;
-- 
2.30.2


             reply	other threads:[~2023-01-18  5:14 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-18  5:14 Alexei Starovoitov [this message]
2023-01-18  5:14 ` [PATCH bpf 2/2] perf: Fix arch_perf_out_copy_user() Alexei Starovoitov
2023-01-18 21:32 ` [PATCH bpf 1/2] mm: Fix copy_from_user_nofault() Hsin-Wei Hung
2023-01-19 16:52 ` Kees Cook
2023-01-19 19:21   ` Alexei Starovoitov
2023-01-19 20:08     ` Kees Cook
2023-01-19 20:14       ` Alexei Starovoitov
2023-01-19 20:28       ` Alexei Starovoitov
2023-03-25 14:55 ` Florian Lehner
2023-03-25 19:47   ` Alexei Starovoitov
2023-04-06 20:17     ` Salvatore Bonaccorso
2023-04-06 20:24       ` Alexei Starovoitov

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=20230118051443.78988-1-alexei.starovoitov@gmail.com \
    --to=alexei.starovoitov@gmail.com \
    --cc=alan.maguire@oracle.com \
    --cc=andrii@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=dylany@meta.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hsinweih@uci.edu \
    --cc=keescook@chromium.org \
    --cc=kernel-team@fb.com \
    --cc=peterz@infradead.org \
    --cc=riel@surriel.com \
    --cc=rostedt@goodmis.org \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linuxfoundation.org \
    --cc=vegard.nossum@oracle.com \
    --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.