| Message ID | 20240208-alice-mm-v2-2-d821250204a6@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Memory management patches needed by Rust Binder | expand |
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
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
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(); 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