Received-SPF: pass (google.com: domain of linux-kernel+bounces-60171-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 9 Feb 2024 16:15:14 -0800
From: Kees Cook <keescook@chromium.org>
To: Alice Ryhl <aliceryhl@google.com>
Cc: Miguel Ojeda <ojeda@kernel.org>, Alex Gaynor <alex.gaynor@gmail.com>,
	Wedson Almeida Filho <wedsonaf@gmail.com>,
	Boqun Feng <boqun.feng@gmail.com>, Gary Guo <gary@garyguo.net>,
	=?iso-8859-1?Q?Bj=F6rn?= Roy Baron <bjorn3_gh@protonmail.com>,
	Benno Lossin <benno.lossin@proton.me>,
	Andreas Hindborg <a.hindborg@samsung.com>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Andrew Morton <akpm@linux-foundation.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Arve =?iso-8859-1?B?SGr4bm5lduVn?= <arve@android.com>,
	Todd Kjos <tkjos@android.com>, Martijn Coenen <maco@android.com>,
	Joel Fernandes <joel@joelfernandes.org>,
	Carlos Llamas <cmllamas@google.com>,
	Suren Baghdasaryan <surenb@google.com>,
	Arnd Bergmann <arnd@arndb.de>, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org,
	Christian Brauner <brauner@kernel.org>
Subject: Re: [PATCH v2 2/4] uaccess: always export _copy_[from|to]_user with
 CONFIG_RUST
Message-ID: <202402091606.A181673F0A@keescook>
References: <20240208-alice-mm-v2-0-d821250204a6@google.com>
 <20240208-alice-mm-v2-2-d821250204a6@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240208-alice-mm-v2-2-d821250204a6@google.com>

On Thu, Feb 08, 2024 at 03:47:52PM +0000, Alice Ryhl wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> Rust code needs to be able to access _copy_from_user and _copy_to_user
> so that it can skip the check_copy_size check in cases where the length
> is known at compile-time, mirroring the logic for when C code will skip
> check_copy_size. To do this, we ensure that exported versions of these
> methods are available when CONFIG_RUST is enabled.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  include/linux/uaccess.h | 37 +++++++++++++++++++++++--------------
>  lib/usercopy.c          | 30 ++++--------------------------
>  2 files changed, 27 insertions(+), 40 deletions(-)
> 
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index 3064314f4832..835aa175d0ee 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -138,13 +138,18 @@ __copy_to_user(void __user *to, const void *from, unsigned long n)
>  	return raw_copy_to_user(to, from, n);
>  }
>  
> -#ifdef INLINE_COPY_FROM_USER
>  static inline __must_check unsigned long
> -_copy_from_user(void *to, const void __user *from, unsigned long n)
> +_inline_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))) {
> +		/*
> +		 * Ensure that bad access_ok() speculation will not
> +		 * lead to nasty side effects *after* the copy is
> +		 * finished:
> +		 */
> +		barrier_nospec();

This means all callers just gained this barrier. That's a behavioral
change -- is it intentional here? I don't see it mentioned in the commit
log.

Also did this get tested with the CONFIG_TEST_USER_COPY tests? I would
expect it to be fine, but better to check and mention it in the commit
log.

-Kees

>  		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);
> @@ -153,14 +158,11 @@ _copy_from_user(void *to, const void __user *from, unsigned long n)
>  		memset(to + (n - res), 0, res);
>  	return res;
>  }
> -#else
>  extern __must_check unsigned long
>  _copy_from_user(void *, const void __user *, unsigned long);
> -#endif
>  
> -#ifdef INLINE_COPY_TO_USER
>  static inline __must_check unsigned long
> -_copy_to_user(void __user *to, const void *from, unsigned long n)
> +_inline_copy_to_user(void __user *to, const void *from, unsigned long n)
>  {
>  	might_fault();
>  	if (should_fail_usercopy())
> @@ -171,25 +173,32 @@ _copy_to_user(void __user *to, const void *from, unsigned long n)
>  	}
>  	return n;
>  }
> -#else
>  extern __must_check unsigned long
>  _copy_to_user(void __user *, const void *, unsigned long);
> -#endif
>  
>  static __always_inline unsigned long __must_check
>  copy_from_user(void *to, const void __user *from, unsigned long n)
>  {
> -	if (check_copy_size(to, n, false))
> -		n = _copy_from_user(to, from, n);
> -	return n;
> +	if (!check_copy_size(to, n, false))
> +		return n;
> +#ifdef INLINE_COPY_FROM_USER
> +	return _inline_copy_from_user(to, from, n);
> +#else
> +	return _copy_from_user(to, from, n);
> +#endif
>  }
>  
>  static __always_inline unsigned long __must_check
>  copy_to_user(void __user *to, const void *from, unsigned long n)
>  {
> -	if (check_copy_size(from, n, true))
> -		n = _copy_to_user(to, from, n);
> -	return n;
> +	if (!check_copy_size(from, n, true))
> +		return n;
> +
> +#ifdef INLINE_COPY_TO_USER
> +	return _inline_copy_to_user(to, from, n);
> +#else
> +	return _copy_to_user(to, from, n);
> +#endif
>  }
>  
>  #ifndef copy_mc_to_kernel
> diff --git a/lib/usercopy.c b/lib/usercopy.c
> index d29fe29c6849..de7f30618293 100644
> --- a/lib/usercopy.c
> +++ b/lib/usercopy.c
> @@ -7,40 +7,18 @@
>  
>  /* out-of-line parts */
>  
> -#ifndef INLINE_COPY_FROM_USER
> +#if !defined(INLINE_COPY_FROM_USER) || defined(CONFIG_RUST)
>  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))) {
> -		/*
> -		 * Ensure that bad access_ok() speculation will not
> -		 * lead to nasty side effects *after* the copy is
> -		 * finished:
> -		 */
> -		barrier_nospec();
> -		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);
> -	return res;
> +	return _inline_copy_from_user(to, from, n);
>  }
>  EXPORT_SYMBOL(_copy_from_user);
>  #endif
>  
> -#ifndef INLINE_COPY_TO_USER
> +#if !defined(INLINE_COPY_TO_USER) || defined(CONFIG_RUST)
>  unsigned long _copy_to_user(void __user *to, const void *from, unsigned long n)
>  {
> -	might_fault();
> -	if (should_fail_usercopy())
> -		return n;
> -	if (likely(access_ok(to, n))) {
> -		instrument_copy_to_user(to, from, n);
> -		n = raw_copy_to_user(to, from, n);
> -	}
> -	return n;
> +	return _inline_copy_to_user(to, from, n);
>  }
>  EXPORT_SYMBOL(_copy_to_user);
>  #endif
> 
> -- 
> 2.43.0.594.gd9cf4e227d-goog
> 

-- 
Kees Cook