On Thu, Jan 25, 2024, at 13:37, Alice Ryhl wrote: > On Thu, Jan 25, 2024 at 1:27 PM Arnd Bergmann <arnd@xxxxxxxx> wrote: >> On Wed, Jan 24, 2024, at 12:20, Alice Ryhl wrote: >> > +EXPORT_SYMBOL_GPL(rust_helper_copy_to_user_unsafe_skip_check_object_size); >> >> These functions are almost identical to the ones in >> lib/usercopy.c for !defined(INLINE_COPY_TO_USER). >> >> That version has an extra memset() after a partial >> copy_from_user(), and you probably want to have the >> same thing here for consistency. >> >> I think ideally we should only have one out-of-line copy >> of these two functions and have that one shared between >> rust and architectures that want the C version out of line >> as well. > > I had a bit of trouble figuring out all of the copy_[to/from]_user > methods that are available. I was hoping that a better solution would > be available, and it sounds like one is. Is _copy_from_user always > available as an exported symbol? If it's always available and skips > the check, then I can just use that. I don't think the memset matters > for my case. At the moment, it appears that it's available on the few architectures that don't #define INLINE_COPY_FROM_USER: alpha, csky, powerpc, riscv and x86. On the other architectures it is always an inline function. > Otherwise, I can add a helper in rust/helpers.c that wraps > _copy_from_user only when INLINE_COPY_FROM_USER is defined, and call > the helper in those cases, and otherwise call the exported symbol > directly. (I need an exported symbol to call into C from Rust.) > > Would that make sense? I don't think we can have a perfect abstraction here, but rather than putting knowledge of INLINE_COPY_FROM_USER into the rust wrapper, I would suggest putting a bit of information about rust into lib/usercopy.c. I've tried to come up with an idea below, see if that works for you. Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx> 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..503a064d79e2 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