[v2,2/4] uaccess: always export _copy_[from|to]_user with CONFIG_RUST
Commit Message
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(-)
Comments
> -#else
> extern __must_check unsigned long
> _copy_from_user(void *, const void __user *, unsigned long);
> -#endif
This function is now unconditionally declared, but only defined if
`!defined(INLINE_COPY_FROM_USER) || defined(CONFIG_RUST)`, i.e., in the
common case where it is inlined and Rust is disabled this can lead to
link-time problems if someone decides to use it.
On Thu, Feb 8, 2024, at 23:56, Valentin Obst wrote:
>> -#else
>> extern __must_check unsigned long
>> _copy_from_user(void *, const void __user *, unsigned long);
>> -#endif
>
> This function is now unconditionally declared, but only defined if
> `!defined(INLINE_COPY_FROM_USER) || defined(CONFIG_RUST)`, i.e., in the
> common case where it is inlined and Rust is disabled this can lead to
> link-time problems if someone decides to use it.
Yes, that is intentional.
If someone tries to use it when the declaration is not there,
they just get a compile-time error, which is not all that
different from a link-time error in practice.
It's unlikely to make a difference here, but enclosing
declarations in an #ifdef is annoying when you want to
reference it from somewhere that is parsed by the compiler
but not called without the respective options.
The if(IS_ENABLED()) and PTR_IF() constructs in particular
only work when the unused functions are still declared.
Arnd
>>> -#else
>>> extern __must_check unsigned long
>>> _copy_from_user(void *, const void __user *, unsigned long);
>>> -#endif
>>
>> This function is now unconditionally declared, but only defined if
>> `!defined(INLINE_COPY_FROM_USER) || defined(CONFIG_RUST)`, i.e., in the
>> common case where it is inlined and Rust is disabled this can lead to
>> link-time problems if someone decides to use it.
>
> Yes, that is intentional.
>
> If someone tries to use it when the declaration is not there,
> they just get a compile-time error, which is not all that
> different from a link-time error in practice.
>
> It's unlikely to make a difference here, but enclosing
> declarations in an #ifdef is annoying when you want to
> reference it from somewhere that is parsed by the compiler
> but not called without the respective options.
>
> The if(IS_ENABLED()) and PTR_IF() constructs in particular
> only work when the unused functions are still declared.
I see, thanks for the explanation.
- Best Valentin
>
> Arnd
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
>
On Sat, Feb 10, 2024, at 01:15, Kees Cook wrote:
> On Thu, Feb 08, 2024 at 03:47:52PM +0000, Alice Ryhl wrote:
>> 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.
My bad, I probably should have explained it when I did the
patch as this is very subtle:
The barrier_nospec() definition is a nop on everything other
than x86 and powerpc, but those two were using the out-of-line
version that did in fact use it.
After this patch, the out-of-line function calls the inline
function, so it needs to be added here to keep the behavior
unchanged on the architectures that need it. For the rest,
this change has no effect.
Arnd
On Sat, Feb 10, 2024 at 1:15 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Feb 08, 2024 at 03:47:52PM +0000, Alice Ryhl wrote:
> > 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.
I just ran this with CONFIG_TEST_USER_COPY on x86 using the Android
cuttlefish emulator and it passed there. I also verified that it fails
if I remove the access_ok check. However, the tests succeed even if
the barrier_nospec() call is removed.
That said, it seems like it fails to compile on some other platforms.
It seems like we need to add #include <linux/nospec.h> to uaccess.h to
fix it.
Alice
@@ -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();
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
@@ -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