Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220701142310.2188015-1-glider@google.com> <20220701142310.2188015-4-glider@google.com>
In-Reply-To: <20220701142310.2188015-4-glider@google.com>
From:   Marco Elver <elver@google.com>
Date:   Tue, 12 Jul 2022 16:17:05 +0200
Message-ID: <CANpmjNM9RkiXnqqdVSmpBJ0aw2hjZfmXGPQLgxAwWw+UfRHd7Q@mail.gmail.com>
Subject: Re: [PATCH v4 03/45] instrumented.h: allow instrumenting both sides
 of copy_from_user()
To:     Alexander Potapenko <glider@google.com>
Cc:     Alexander Viro <viro@zeniv.linux.org.uk>,
        Alexei Starovoitov <ast@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Andrey Konovalov <andreyknvl@google.com>,
        Andy Lutomirski <luto@kernel.org>,
        Arnd Bergmann <arnd@arndb.de>, Borislav Petkov <bp@alien8.de>,
        Christoph Hellwig <hch@lst.de>,
        Christoph Lameter <cl@linux.com>,
        David Rientjes <rientjes@google.com>,
        Dmitry Vyukov <dvyukov@google.com>,
        Eric Dumazet <edumazet@google.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        Ilya Leoshkevich <iii@linux.ibm.com>,
        Ingo Molnar <mingo@redhat.com>, Jens Axboe <axboe@kernel.dk>,
        Joonsoo Kim <iamjoonsoo.kim@lge.com>,
        Kees Cook <keescook@chromium.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Matthew Wilcox <willy@infradead.org>,
        "Michael S. Tsirkin" <mst@redhat.com>,
        Pekka Enberg <penberg@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Petr Mladek <pmladek@suse.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Vasily Gorbik <gor@linux.ibm.com>,
        Vegard Nossum <vegard.nossum@oracle.com>,
        Vlastimil Babka <vbabka@suse.cz>, kasan-dev@googlegroups.com,
        linux-mm@kvack.org, linux-arch@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, 1 Jul 2022 at 16:23, Alexander Potapenko <glider@google.com> wrote:
>
> Introduce instrument_copy_from_user_before() and
> instrument_copy_from_user_after() hooks to be invoked before and after
> the call to copy_from_user().
>
> KASAN and KCSAN will be only using instrument_copy_from_user_before(),
> but for KMSAN we'll need to insert code after copy_from_user().
>
> Signed-off-by: Alexander Potapenko <glider@google.com>

Reviewed-by: Marco Elver <elver@google.com>


> ---
> v4:
>  -- fix _copy_from_user_key() in arch/s390/lib/uaccess.c (Reported-by:
>     kernel test robot <lkp@intel.com>)
>
> Link: https://linux-review.googlesource.com/id/I855034578f0b0f126734cbd734fb4ae1d3a6af99
> ---
>  arch/s390/lib/uaccess.c      |  3 ++-
>  include/linux/instrumented.h | 21 +++++++++++++++++++--
>  include/linux/uaccess.h      | 19 ++++++++++++++-----
>  lib/iov_iter.c               |  9 ++++++---
>  lib/usercopy.c               |  3 ++-
>  5 files changed, 43 insertions(+), 12 deletions(-)
>
> diff --git a/arch/s390/lib/uaccess.c b/arch/s390/lib/uaccess.c
> index d7b3b193d1088..58033dfcb6d45 100644
> --- a/arch/s390/lib/uaccess.c
> +++ b/arch/s390/lib/uaccess.c
> @@ -81,8 +81,9 @@ unsigned long _copy_from_user_key(void *to, const void __user *from,
>
>         might_fault();
>         if (!should_fail_usercopy()) {
> -               instrument_copy_from_user(to, from, n);
> +               instrument_copy_from_user_before(to, from, n);
>                 res = raw_copy_from_user_key(to, from, n, key);
> +               instrument_copy_from_user_after(to, from, n, res);
>         }
>         if (unlikely(res))
>                 memset(to + (n - res), 0, res);
> diff --git a/include/linux/instrumented.h b/include/linux/instrumented.h
> index 42faebbaa202a..ee8f7d17d34f5 100644
> --- a/include/linux/instrumented.h
> +++ b/include/linux/instrumented.h
> @@ -120,7 +120,7 @@ instrument_copy_to_user(void __user *to, const void *from, unsigned long n)
>  }
>
>  /**
> - * instrument_copy_from_user - instrument writes of copy_from_user
> + * instrument_copy_from_user_before - add instrumentation before copy_from_user
>   *
>   * Instrument writes to kernel memory, that are due to copy_from_user (and
>   * variants). The instrumentation should be inserted before the accesses.
> @@ -130,10 +130,27 @@ instrument_copy_to_user(void __user *to, const void *from, unsigned long n)
>   * @n number of bytes to copy
>   */
>  static __always_inline void
> -instrument_copy_from_user(const void *to, const void __user *from, unsigned long n)
> +instrument_copy_from_user_before(const void *to, const void __user *from, unsigned long n)
>  {
>         kasan_check_write(to, n);
>         kcsan_check_write(to, n);
>  }
>
> +/**
> + * instrument_copy_from_user_after - add instrumentation after copy_from_user
> + *
> + * Instrument writes to kernel memory, that are due to copy_from_user (and
> + * variants). The instrumentation should be inserted after the accesses.
> + *
> + * @to destination address
> + * @from source address
> + * @n number of bytes to copy
> + * @left number of bytes not copied (as returned by copy_from_user)
> + */
> +static __always_inline void
> +instrument_copy_from_user_after(const void *to, const void __user *from,
> +                               unsigned long n, unsigned long left)
> +{
> +}
> +
>  #endif /* _LINUX_INSTRUMENTED_H */
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index 5a328cf02b75e..da16e96680cf1 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -58,20 +58,28 @@
>  static __always_inline __must_check unsigned long
>  __copy_from_user_inatomic(void *to, const void __user *from, unsigned long n)
>  {
> -       instrument_copy_from_user(to, from, n);
> +       unsigned long res;
> +
> +       instrument_copy_from_user_before(to, from, n);
>         check_object_size(to, n, false);
> -       return raw_copy_from_user(to, from, n);
> +       res = raw_copy_from_user(to, from, n);
> +       instrument_copy_from_user_after(to, from, n, res);
> +       return res;
>  }
>
>  static __always_inline __must_check unsigned long
>  __copy_from_user(void *to, const void __user *from, unsigned long n)
>  {
> +       unsigned long res;
> +
>         might_fault();
> +       instrument_copy_from_user_before(to, from, n);
>         if (should_fail_usercopy())
>                 return n;
> -       instrument_copy_from_user(to, from, n);
>         check_object_size(to, n, false);
> -       return raw_copy_from_user(to, from, n);
> +       res = raw_copy_from_user(to, from, n);
> +       instrument_copy_from_user_after(to, from, n, res);
> +       return res;
>  }
>
>  /**
> @@ -115,8 +123,9 @@ _copy_from_user(void *to, const void __user *from, unsigned long n)
>         unsigned long res = n;
>         might_fault();
>         if (!should_fail_usercopy() && likely(access_ok(from, n))) {
> -               instrument_copy_from_user(to, from, n);
> +               instrument_copy_from_user_before(to, from, n);
>                 res = raw_copy_from_user(to, from, n);
> +               instrument_copy_from_user_after(to, from, n, res);
>         }
>         if (unlikely(res))
>                 memset(to + (n - res), 0, res);
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index 0b64695ab632f..fe5d169314dbf 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -159,13 +159,16 @@ static int copyout(void __user *to, const void *from, size_t n)
>
>  static int copyin(void *to, const void __user *from, size_t n)
>  {
> +       size_t res = n;
> +
>         if (should_fail_usercopy())
>                 return n;
>         if (access_ok(from, n)) {
> -               instrument_copy_from_user(to, from, n);
> -               n = raw_copy_from_user(to, from, n);
> +               instrument_copy_from_user_before(to, from, n);
> +               res = raw_copy_from_user(to, from, n);
> +               instrument_copy_from_user_after(to, from, n, res);
>         }
> -       return n;
> +       return res;
>  }
>
>  static size_t copy_page_to_iter_iovec(struct page *page, size_t offset, size_t bytes,
> diff --git a/lib/usercopy.c b/lib/usercopy.c
> index 7413dd300516e..1505a52f23a01 100644
> --- a/lib/usercopy.c
> +++ b/lib/usercopy.c
> @@ -12,8 +12,9 @@ unsigned long _copy_from_user(void *to, const void __user *from, unsigned long n
>         unsigned long res = n;
>         might_fault();
>         if (!should_fail_usercopy() && likely(access_ok(from, n))) {
> -               instrument_copy_from_user(to, from, n);
> +               instrument_copy_from_user_before(to, from, n);
>                 res = raw_copy_from_user(to, from, n);
> +               instrument_copy_from_user_after(to, from, n, res);
>         }
>         if (unlikely(res))
>                 memset(to + (n - res), 0, res);
> --
> 2.37.0.rc0.161.g10f37bed90-goog
>