| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 9 Feb 2024 16:15:14 -0800
From: Kees Cook <keescook@...omium.org>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
Wedson Almeida Filho <wedsonaf@...il.com>,
Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
Björn Roy Baron <bjorn3_gh@...tonmail.com>,
Benno Lossin <benno.lossin@...ton.me>,
Andreas Hindborg <a.hindborg@...sung.com>,
Al Viro <viro@...iv.linux.org.uk>,
Andrew Morton <akpm@...ux-foundation.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Arve Hjønnevåg <arve@...roid.com>,
Todd Kjos <tkjos@...roid.com>, Martijn Coenen <maco@...roid.com>,
Joel Fernandes <joel@...lfernandes.org>,
Carlos Llamas <cmllamas@...gle.com>,
Suren Baghdasaryan <surenb@...gle.com>,
Arnd Bergmann <arnd@...db.de>, linux-mm@...ck.org,
linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
Christian Brauner <brauner@...nel.org>
Subject: Re: [PATCH v2 2/4] uaccess: always export _copy_[from|to]_user with
CONFIG_RUST
On Thu, Feb 08, 2024 at 03:47:52PM +0000, Alice Ryhl wrote:
> From: Arnd Bergmann <arnd@...db.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@...db.de>
> Signed-off-by: Alice Ryhl <aliceryhl@...gle.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
Powered by blists - more mailing lists