From: Marco Elver <elver@google.com> To: Dmitry Vyukov <dvyukov@google.com> Cc: "Paul E. McKenney" <paulmck@kernel.org>, Andrey Konovalov <andreyknvl@google.com>, Alexander Potapenko <glider@google.com>, kasan-dev <kasan-dev@googlegroups.com>, LKML <linux-kernel@vger.kernel.org>, Mark Rutland <mark.rutland@arm.com>, Will Deacon <will@kernel.org>, Peter Zijlstra <peterz@infradead.org>, Boqun Feng <boqun.feng@gmail.com>, Arnd Bergmann <arnd@arndb.de>, Al Viro <viro@zeniv.linux.org.uk>, Christophe Leroy <christophe.leroy@c-s.fr>, Daniel Axtens <dja@axtens.net>, Michael Ellerman <mpe@ellerman.id.au>, Steven Rostedt <rostedt@goodmis.org>, Masami Hiramatsu <mhiramat@kernel.org>, Ingo Molnar <mingo@kernel.org>, Christian Brauner <christian.brauner@ubuntu.com>, Daniel Borkmann <daniel@iogearbox.net>, cyphar@cyphar.com, Kees Cook <keescook@chromium.org>, linux-arch <linux-arch@vger.kernel.org> Subject: Re: [PATCH 5/5] copy_to_user, copy_from_user: Use generic instrumented.h Date: Mon, 20 Jan 2020 16:05:42 +0100 [thread overview] Message-ID: <CANpmjNMZpLfNKLOs7JVxP-S7oWbkvyg=bt=uYGU30bMZXYtUHA@mail.gmail.com> (raw) In-Reply-To: <CACT4Y+bUvoePVPV+BqU-cwhF6bR41_eaYkr9WLLMYi-2q11JjQ@mail.gmail.com> On Mon, 20 Jan 2020 at 15:52, Dmitry Vyukov <dvyukov@google.com> wrote: > > On Mon, Jan 20, 2020 at 3:19 PM Marco Elver <elver@google.com> wrote: > > > > This replaces the KASAN instrumentation with generic instrumentation, > > implicitly adding KCSAN instrumentation support. > > > > For KASAN no functional change is intended. > > > > Suggested-by: Arnd Bergmann <arnd@arndb.de> > > Signed-off-by: Marco Elver <elver@google.com> > > --- > > include/linux/uaccess.h | 46 +++++++++++++++++++++++++++++------------ > > lib/usercopy.c | 14 ++++++++----- > > 2 files changed, 42 insertions(+), 18 deletions(-) > > > > diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h > > index 67f016010aad..d3f2d9a8cae3 100644 > > --- a/include/linux/uaccess.h > > +++ b/include/linux/uaccess.h > > @@ -2,9 +2,9 @@ > > #ifndef __LINUX_UACCESS_H__ > > #define __LINUX_UACCESS_H__ > > > > +#include <linux/instrumented.h> > > #include <linux/sched.h> > > #include <linux/thread_info.h> > > -#include <linux/kasan-checks.h> > > > > #define uaccess_kernel() segment_eq(get_fs(), KERNEL_DS) > > > > @@ -58,18 +58,26 @@ > > static __always_inline __must_check unsigned long > > __copy_from_user_inatomic(void *to, const void __user *from, unsigned long n) > > { > > - kasan_check_write(to, n); > > + unsigned long res; > > + > > check_object_size(to, n, false); > > - return raw_copy_from_user(to, from, n); > > + instrument_copy_from_user_pre(to, n); > > + res = raw_copy_from_user(to, from, n); > > + instrument_copy_from_user_post(to, n, res); > > + return res; > > } > > There is also something called strncpy_from_user() that has kasan > instrumentation now: > https://elixir.bootlin.com/linux/v5.5-rc6/source/lib/strncpy_from_user.c#L117 Yes, however, I think it's a special case for KASAN. The implementation is already instrumented by the compiler. In the original commit it says (1771c6e1a567e): "Note: Unlike others strncpy_from_user() is written mostly in C and KASAN sees memory accesses in it. However, it makes sense to add explicit check for all @count bytes that *potentially* could be written to the kernel." I don't think we want unconditional double-instrumentation here. Let me know if you think otherwise. Thanks, -- Marco > > static __always_inline __must_check unsigned long > > __copy_from_user(void *to, const void __user *from, unsigned long n) > > { > > + unsigned long res; > > + > > might_fault(); > > - kasan_check_write(to, n); > > check_object_size(to, n, false); > > - return raw_copy_from_user(to, from, n); > > + instrument_copy_from_user_pre(to, n); > > + res = raw_copy_from_user(to, from, n); > > + instrument_copy_from_user_post(to, n, res); > > + return res; > > } > > > > /** > > @@ -88,18 +96,26 @@ __copy_from_user(void *to, const void __user *from, unsigned long n) > > static __always_inline __must_check unsigned long > > __copy_to_user_inatomic(void __user *to, const void *from, unsigned long n) > > { > > - kasan_check_read(from, n); > > + unsigned long res; > > + > > check_object_size(from, n, true); > > - return raw_copy_to_user(to, from, n); > > + instrument_copy_to_user_pre(from, n); > > + res = raw_copy_to_user(to, from, n); > > + instrument_copy_to_user_post(from, n, res); > > + return res; > > } > > > > static __always_inline __must_check unsigned long > > __copy_to_user(void __user *to, const void *from, unsigned long n) > > { > > + unsigned long res; > > + > > might_fault(); > > - kasan_check_read(from, n); > > check_object_size(from, n, true); > > - return raw_copy_to_user(to, from, n); > > + instrument_copy_to_user_pre(from, n); > > + res = raw_copy_to_user(to, from, n); > > + instrument_copy_to_user_post(from, n, res); > > + return res; > > } > > > > #ifdef INLINE_COPY_FROM_USER > > @@ -109,8 +125,9 @@ _copy_from_user(void *to, const void __user *from, unsigned long n) > > unsigned long res = n; > > might_fault(); > > if (likely(access_ok(from, n))) { > > - kasan_check_write(to, n); > > + instrument_copy_from_user_pre(to, n); > > res = raw_copy_from_user(to, from, n); > > + instrument_copy_from_user_post(to, n, res); > > } > > if (unlikely(res)) > > memset(to + (n - res), 0, res); > > @@ -125,12 +142,15 @@ _copy_from_user(void *, const void __user *, unsigned long); > > static inline __must_check unsigned long > > _copy_to_user(void __user *to, const void *from, unsigned long n) > > { > > + unsigned long res = n; > > + > > might_fault(); > > if (access_ok(to, n)) { > > - kasan_check_read(from, n); > > - n = raw_copy_to_user(to, from, n); > > + instrument_copy_to_user_pre(from, n); > > + res = raw_copy_to_user(to, from, n); > > + instrument_copy_to_user_post(from, n, res); > > } > > - return n; > > + return res; > > } > > #else > > extern __must_check unsigned long > > diff --git a/lib/usercopy.c b/lib/usercopy.c > > index cbb4d9ec00f2..1c20d4423b86 100644 > > --- a/lib/usercopy.c > > +++ b/lib/usercopy.c > > @@ -1,6 +1,7 @@ > > // SPDX-License-Identifier: GPL-2.0 > > -#include <linux/uaccess.h> > > #include <linux/bitops.h> > > +#include <linux/instrumented.h> > > +#include <linux/uaccess.h> > > > > /* out-of-line parts */ > > > > @@ -10,8 +11,9 @@ unsigned long _copy_from_user(void *to, const void __user *from, unsigned long n > > unsigned long res = n; > > might_fault(); > > if (likely(access_ok(from, n))) { > > - kasan_check_write(to, n); > > + instrument_copy_from_user_pre(to, n); > > res = raw_copy_from_user(to, from, n); > > + instrument_copy_from_user_post(to, n, res); > > } > > if (unlikely(res)) > > memset(to + (n - res), 0, res); > > @@ -23,12 +25,14 @@ EXPORT_SYMBOL(_copy_from_user); > > #ifndef INLINE_COPY_TO_USER > > unsigned long _copy_to_user(void __user *to, const void *from, unsigned long n) > > { > > + unsigned long res = n; > > might_fault(); > > if (likely(access_ok(to, n))) { > > - kasan_check_read(from, n); > > - n = raw_copy_to_user(to, from, n); > > + instrument_copy_to_user_pre(from, n); > > + res = raw_copy_to_user(to, from, n); > > + instrument_copy_to_user_post(from, n, res); > > } > > - return n; > > + return res; > > } > > EXPORT_SYMBOL(_copy_to_user); > > #endif > > -- > > 2.25.0.341.g760bfbb309-goog > >
WARNING: multiple messages have this Message-ID (diff)
From: Marco Elver <elver@google.com> To: Dmitry Vyukov <dvyukov@google.com> Cc: "Paul E. McKenney" <paulmck@kernel.org>, Andrey Konovalov <andreyknvl@google.com>, Alexander Potapenko <glider@google.com>, kasan-dev <kasan-dev@googlegroups.com>, LKML <linux-kernel@vger.kernel.org>, Mark Rutland <mark.rutland@arm.com>, Will Deacon <will@kernel.org>, Peter Zijlstra <peterz@infradead.org>, Boqun Feng <boqun.feng@gmail.com>, Arnd Bergmann <arnd@arndb.de>, Al Viro <viro@zeniv.linux.org.uk>, Christophe Leroy <christophe.leroy@c-s.fr>, Daniel Axtens <dja@axtens.net>, Michael Ellerman <mpe@ellerman.id.au>, Steven Rostedt <rostedt@goodmis.org>, Masami Hiramatsu <mhiramat@kernel.org>, Ingo Molnar <mingo@kernel.org>, Christian Brauner <christian.brauner@ubuntu.com>, Daniel Borkmann <daniel@iogearbox.net>, cyphar@cyphar.comK Subject: Re: [PATCH 5/5] copy_to_user, copy_from_user: Use generic instrumented.h Date: Mon, 20 Jan 2020 16:05:42 +0100 [thread overview] Message-ID: <CANpmjNMZpLfNKLOs7JVxP-S7oWbkvyg=bt=uYGU30bMZXYtUHA@mail.gmail.com> (raw) In-Reply-To: <CACT4Y+bUvoePVPV+BqU-cwhF6bR41_eaYkr9WLLMYi-2q11JjQ@mail.gmail.com> On Mon, 20 Jan 2020 at 15:52, Dmitry Vyukov <dvyukov@google.com> wrote: > > On Mon, Jan 20, 2020 at 3:19 PM Marco Elver <elver@google.com> wrote: > > > > This replaces the KASAN instrumentation with generic instrumentation, > > implicitly adding KCSAN instrumentation support. > > > > For KASAN no functional change is intended. > > > > Suggested-by: Arnd Bergmann <arnd@arndb.de> > > Signed-off-by: Marco Elver <elver@google.com> > > --- > > include/linux/uaccess.h | 46 +++++++++++++++++++++++++++++------------ > > lib/usercopy.c | 14 ++++++++----- > > 2 files changed, 42 insertions(+), 18 deletions(-) > > > > diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h > > index 67f016010aad..d3f2d9a8cae3 100644 > > --- a/include/linux/uaccess.h > > +++ b/include/linux/uaccess.h > > @@ -2,9 +2,9 @@ > > #ifndef __LINUX_UACCESS_H__ > > #define __LINUX_UACCESS_H__ > > > > +#include <linux/instrumented.h> > > #include <linux/sched.h> > > #include <linux/thread_info.h> > > -#include <linux/kasan-checks.h> > > > > #define uaccess_kernel() segment_eq(get_fs(), KERNEL_DS) > > > > @@ -58,18 +58,26 @@ > > static __always_inline __must_check unsigned long > > __copy_from_user_inatomic(void *to, const void __user *from, unsigned long n) > > { > > - kasan_check_write(to, n); > > + unsigned long res; > > + > > check_object_size(to, n, false); > > - return raw_copy_from_user(to, from, n); > > + instrument_copy_from_user_pre(to, n); > > + res = raw_copy_from_user(to, from, n); > > + instrument_copy_from_user_post(to, n, res); > > + return res; > > } > > There is also something called strncpy_from_user() that has kasan > instrumentation now: > https://elixir.bootlin.com/linux/v5.5-rc6/source/lib/strncpy_from_user.c#L117 Yes, however, I think it's a special case for KASAN. The implementation is already instrumented by the compiler. In the original commit it says (1771c6e1a567e): "Note: Unlike others strncpy_from_user() is written mostly in C and KASAN sees memory accesses in it. However, it makes sense to add explicit check for all @count bytes that *potentially* could be written to the kernel." I don't think we want unconditional double-instrumentation here. Let me know if you think otherwise. Thanks, -- Marco > > static __always_inline __must_check unsigned long > > __copy_from_user(void *to, const void __user *from, unsigned long n) > > { > > + unsigned long res; > > + > > might_fault(); > > - kasan_check_write(to, n); > > check_object_size(to, n, false); > > - return raw_copy_from_user(to, from, n); > > + instrument_copy_from_user_pre(to, n); > > + res = raw_copy_from_user(to, from, n); > > + instrument_copy_from_user_post(to, n, res); > > + return res; > > } > > > > /** > > @@ -88,18 +96,26 @@ __copy_from_user(void *to, const void __user *from, unsigned long n) > > static __always_inline __must_check unsigned long > > __copy_to_user_inatomic(void __user *to, const void *from, unsigned long n) > > { > > - kasan_check_read(from, n); > > + unsigned long res; > > + > > check_object_size(from, n, true); > > - return raw_copy_to_user(to, from, n); > > + instrument_copy_to_user_pre(from, n); > > + res = raw_copy_to_user(to, from, n); > > + instrument_copy_to_user_post(from, n, res); > > + return res; > > } > > > > static __always_inline __must_check unsigned long > > __copy_to_user(void __user *to, const void *from, unsigned long n) > > { > > + unsigned long res; > > + > > might_fault(); > > - kasan_check_read(from, n); > > check_object_size(from, n, true); > > - return raw_copy_to_user(to, from, n); > > + instrument_copy_to_user_pre(from, n); > > + res = raw_copy_to_user(to, from, n); > > + instrument_copy_to_user_post(from, n, res); > > + return res; > > } > > > > #ifdef INLINE_COPY_FROM_USER > > @@ -109,8 +125,9 @@ _copy_from_user(void *to, const void __user *from, unsigned long n) > > unsigned long res = n; > > might_fault(); > > if (likely(access_ok(from, n))) { > > - kasan_check_write(to, n); > > + instrument_copy_from_user_pre(to, n); > > res = raw_copy_from_user(to, from, n); > > + instrument_copy_from_user_post(to, n, res); > > } > > if (unlikely(res)) > > memset(to + (n - res), 0, res); > > @@ -125,12 +142,15 @@ _copy_from_user(void *, const void __user *, unsigned long); > > static inline __must_check unsigned long > > _copy_to_user(void __user *to, const void *from, unsigned long n) > > { > > + unsigned long res = n; > > + > > might_fault(); > > if (access_ok(to, n)) { > > - kasan_check_read(from, n); > > - n = raw_copy_to_user(to, from, n); > > + instrument_copy_to_user_pre(from, n); > > + res = raw_copy_to_user(to, from, n); > > + instrument_copy_to_user_post(from, n, res); > > } > > - return n; > > + return res; > > } > > #else > > extern __must_check unsigned long > > diff --git a/lib/usercopy.c b/lib/usercopy.c > > index cbb4d9ec00f2..1c20d4423b86 100644 > > --- a/lib/usercopy.c > > +++ b/lib/usercopy.c > > @@ -1,6 +1,7 @@ > > // SPDX-License-Identifier: GPL-2.0 > > -#include <linux/uaccess.h> > > #include <linux/bitops.h> > > +#include <linux/instrumented.h> > > +#include <linux/uaccess.h> > > > > /* out-of-line parts */ > > > > @@ -10,8 +11,9 @@ unsigned long _copy_from_user(void *to, const void __user *from, unsigned long n > > unsigned long res = n; > > might_fault(); > > if (likely(access_ok(from, n))) { > > - kasan_check_write(to, n); > > + instrument_copy_from_user_pre(to, n); > > res = raw_copy_from_user(to, from, n); > > + instrument_copy_from_user_post(to, n, res); > > } > > if (unlikely(res)) > > memset(to + (n - res), 0, res); > > @@ -23,12 +25,14 @@ EXPORT_SYMBOL(_copy_from_user); > > #ifndef INLINE_COPY_TO_USER > > unsigned long _copy_to_user(void __user *to, const void *from, unsigned long n) > > { > > + unsigned long res = n; > > might_fault(); > > if (likely(access_ok(to, n))) { > > - kasan_check_read(from, n); > > - n = raw_copy_to_user(to, from, n); > > + instrument_copy_to_user_pre(from, n); > > + res = raw_copy_to_user(to, from, n); > > + instrument_copy_to_user_post(from, n, res); > > } > > - return n; > > + return res; > > } > > EXPORT_SYMBOL(_copy_to_user); > > #endif > > -- > > 2.25.0.341.g760bfbb309-goog > >
next prev parent reply other threads:[~2020-01-20 15:05 UTC|newest] Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-01-20 14:19 [PATCH 1/5] include/linux: Add instrumented.h infrastructure Marco Elver 2020-01-20 14:19 ` [PATCH 2/5] asm-generic, atomic-instrumented: Use generic instrumented.h Marco Elver 2020-01-20 14:19 ` [PATCH 3/5] asm-generic, kcsan: Add KCSAN instrumentation for bitops Marco Elver 2020-01-20 14:40 ` Peter Zijlstra 2020-01-20 16:27 ` Paul E. McKenney 2020-01-20 16:52 ` Peter Zijlstra 2020-01-20 20:23 ` Paul E. McKenney 2020-01-21 9:15 ` Peter Zijlstra 2020-01-21 14:21 ` Paul E. McKenney 2020-01-21 14:47 ` Peter Zijlstra 2020-01-21 15:07 ` Marco Elver 2020-01-21 15:07 ` Marco Elver 2020-01-21 16:16 ` Paul E. McKenney 2020-01-20 14:19 ` [PATCH 4/5] iov_iter: Use generic instrumented.h Marco Elver 2020-01-20 14:19 ` [PATCH 5/5] copy_to_user, copy_from_user: " Marco Elver 2020-01-20 14:51 ` Dmitry Vyukov 2020-01-20 14:51 ` Dmitry Vyukov 2020-01-20 15:05 ` Marco Elver [this message] 2020-01-20 15:05 ` Marco Elver 2020-01-20 14:25 ` [PATCH 1/5] include/linux: Add instrumented.h infrastructure Alexander Potapenko 2020-01-20 14:34 ` Dmitry Vyukov 2020-01-20 14:34 ` Dmitry Vyukov 2020-01-20 15:53 ` Marco Elver 2020-01-20 15:53 ` Marco Elver 2020-01-20 14:45 ` Dmitry Vyukov 2020-01-20 14:45 ` Dmitry Vyukov 2020-01-20 14:58 ` Dmitry Vyukov 2020-01-20 14:58 ` Dmitry Vyukov 2020-01-20 15:09 ` Dmitry Vyukov 2020-01-20 15:09 ` Dmitry Vyukov 2020-01-20 15:40 ` Marco Elver 2020-01-20 15:40 ` Marco Elver 2020-01-20 16:06 ` Dmitry Vyukov 2020-01-20 16:06 ` Dmitry Vyukov 2020-01-20 16:25 ` Marco Elver 2020-01-20 16:25 ` Marco Elver 2020-01-20 16:39 ` Dmitry Vyukov 2020-01-20 16:39 ` Dmitry Vyukov 2020-01-21 9:44 ` Marco Elver 2020-01-21 9:44 ` Marco Elver 2020-01-21 13:01 ` Dmitry Vyukov 2020-01-21 13:01 ` Dmitry Vyukov 2020-01-21 16:14 ` Marco Elver 2020-01-21 16:14 ` Marco Elver
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='CANpmjNMZpLfNKLOs7JVxP-S7oWbkvyg=bt=uYGU30bMZXYtUHA@mail.gmail.com' \ --to=elver@google.com \ --cc=andreyknvl@google.com \ --cc=arnd@arndb.de \ --cc=boqun.feng@gmail.com \ --cc=christian.brauner@ubuntu.com \ --cc=christophe.leroy@c-s.fr \ --cc=cyphar@cyphar.com \ --cc=daniel@iogearbox.net \ --cc=dja@axtens.net \ --cc=dvyukov@google.com \ --cc=glider@google.com \ --cc=kasan-dev@googlegroups.com \ --cc=keescook@chromium.org \ --cc=linux-arch@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=mhiramat@kernel.org \ --cc=mingo@kernel.org \ --cc=mpe@ellerman.id.au \ --cc=paulmck@kernel.org \ --cc=peterz@infradead.org \ --cc=rostedt@goodmis.org \ --cc=viro@zeniv.linux.org.uk \ --cc=will@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: linkBe 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.