From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.4 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D9101C2D0DB for ; Mon, 20 Jan 2020 15:05:57 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 945A2217F4 for ; Mon, 20 Jan 2020 15:05:57 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="tojSwQu1" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729108AbgATPF4 (ORCPT ); Mon, 20 Jan 2020 10:05:56 -0500 Received: from mail-ot1-f67.google.com ([209.85.210.67]:38649 "EHLO mail-ot1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729076AbgATPF4 (ORCPT ); Mon, 20 Jan 2020 10:05:56 -0500 Received: by mail-ot1-f67.google.com with SMTP id z9so10033oth.5 for ; Mon, 20 Jan 2020 07:05:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=smLCZaIYcQF0/52Z19iYa5thG5yG//BBZdb/jdacqzg=; b=tojSwQu1bHYhbIog279hFH4kK/1efQmq3Nw5UqFpyhOHvgdGZIQwaKootMm732MoaR 5uvPwP9TZbXXRqp2YYGWklkJjC1I7wBT5aHBrLDAysGHrTKeZXC4sO+3etTTIOHrkpTI q0hqHd/50k0CXZ9Y0L3+sebhGigshfjrvYhpcrBkYdcnm7/H9JZzGN4YusJQFW4a4vHY WJl0PVj/Dx2oSsTd1mOPgKK6CzhHZle3m0T5dhljPmXpV+blTBDtH7oao5wAKidwgKxI FhHPlmJ7dE2yq2Rc02bDoOhbJGPJcoPpiH6xBAQmFBEvk+AozgxVtu1+/n9vbxVA/CX1 OAkw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=smLCZaIYcQF0/52Z19iYa5thG5yG//BBZdb/jdacqzg=; b=B2gejILCyNZiZAYNknR/d+BkfZ4HUM/oFf3uRhKNHq+CulJ7GiedxjyHoEFRRclaL+ /0A0kMb0HzgUtg//nGXG4a5MZyzjAETqRFJbFDX6DaPvF5xch0Q8EyncXqdqbkHS0wkL J260ze9ExYGvkHKyxsFzEwKUCSIFqUyLUKq2zi2EO3l1+KEYaUf90+3rgmvDr3O7Cbxb ykijAlkWZ6bmJAQVMJqRva4U01bfejbIDPzLDQs8HPoildWxjX/o8n5+9qCZPTxqTCL+ daU6XxwSyreXDunnDo5DSgw1jO9RRWfYoO4c+t24PSfunT9rAO9rk4maGmZVpBpEKBG6 TLaA== X-Gm-Message-State: APjAAAV3JWhro9I8UtKXCiN6z8eAoJslYRZBwqTOTESgchwoKa2KJjze PDBZyRNDJNfxnAN+3mQYGhC/6mcUCYhBExYePOYRHg== X-Google-Smtp-Source: APXvYqwz9M64ESVRwDt7JHftJjj3plesO1W/y3LCrXrSk8oCjrGQ/XZgT2g/2FVrI4NH9xp+JPV1btnfepl5wjcr37Y= X-Received: by 2002:a9d:7410:: with SMTP id n16mr16771057otk.23.1579532754151; Mon, 20 Jan 2020 07:05:54 -0800 (PST) MIME-Version: 1.0 References: <20200120141927.114373-1-elver@google.com> <20200120141927.114373-5-elver@google.com> In-Reply-To: From: Marco Elver Date: Mon, 20 Jan 2020 16:05:42 +0100 Message-ID: Subject: Re: [PATCH 5/5] copy_to_user, copy_from_user: Use generic instrumented.h To: Dmitry Vyukov Cc: "Paul E. McKenney" , Andrey Konovalov , Alexander Potapenko , kasan-dev , LKML , Mark Rutland , Will Deacon , Peter Zijlstra , Boqun Feng , Arnd Bergmann , Al Viro , Christophe Leroy , Daniel Axtens , Michael Ellerman , Steven Rostedt , Masami Hiramatsu , Ingo Molnar , Christian Brauner , Daniel Borkmann , cyphar@cyphar.com, Kees Cook , linux-arch Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 20 Jan 2020 at 15:52, Dmitry Vyukov wrote: > > On Mon, Jan 20, 2020 at 3:19 PM Marco Elver 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 > > Signed-off-by: Marco Elver > > --- > > 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 > > #include > > #include > > -#include > > > > #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 > > #include > > +#include > > +#include > > > > /* 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 > > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marco Elver Subject: Re: [PATCH 5/5] copy_to_user, copy_from_user: Use generic instrumented.h Date: Mon, 20 Jan 2020 16:05:42 +0100 Message-ID: References: <20200120141927.114373-1-elver@google.com> <20200120141927.114373-5-elver@google.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: Received: from mail-ot1-f67.google.com ([209.85.210.67]:40373 "EHLO mail-ot1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729073AbgATPF4 (ORCPT ); Mon, 20 Jan 2020 10:05:56 -0500 Received: by mail-ot1-f67.google.com with SMTP id w21so28906747otj.7 for ; Mon, 20 Jan 2020 07:05:55 -0800 (PST) In-Reply-To: Sender: linux-arch-owner@vger.kernel.org List-ID: To: Dmitry Vyukov Cc: "Paul E. McKenney" , Andrey Konovalov , Alexander Potapenko , kasan-dev , LKML , Mark Rutland , Will Deacon , Peter Zijlstra , Boqun Feng , Arnd Bergmann , Al Viro , Christophe Leroy , Daniel Axtens , Michael Ellerman , Steven Rostedt , Masami Hiramatsu , Ingo Molnar , Christian Brauner , Daniel Borkmann , cyphar@cyphar.comK On Mon, 20 Jan 2020 at 15:52, Dmitry Vyukov wrote: > > On Mon, Jan 20, 2020 at 3:19 PM Marco Elver 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 > > Signed-off-by: Marco Elver > > --- > > 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 > > #include > > #include > > -#include > > > > #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 > > #include > > +#include > > +#include > > > > /* 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 > >