Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] Macro variable shadowing risk in `unsafe_copy_to_user` and `unsafe_copy_from_user`. --
--- Patch [18]: [RFC PATCH v2 18/24] x86/uaccess: Add unsafe_copy_from_user() implementation --- commit 6d0ae2cda144449266a35cb0d45ee49364b75b97 Author: Josh Poimboeuf <[email protected]> x86/uaccess: Add unsafe_copy_from_user() implementation This commit adds an x86 implementation of unsafe_copy_from_user() similar to the existing unsafe_copy_to_user() macro. It also updates the existing unsafe_copy_to_user() macro to use shorter internal variable names. > diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h > index 3a0dd3c2b233e..235886106f31e 100644 > --- a/arch/x86/include/asm/uaccess.h > +++ b/arch/x86/include/asm/uaccess.h [ ... ] > -#define unsafe_copy_to_user(_dst,_src,_len,label) \ > -do { \ > - char __user *__ucu_dst = (_dst); \ > - const char *__ucu_src = (_src); \ > - size_t __ucu_len = (_len); \ > - unsafe_copy_loop(__ucu_dst, __ucu_src, __ucu_len, u64, label); \ > - unsafe_copy_loop(__ucu_dst, __ucu_src, __ucu_len, u32, label); \ > - unsafe_copy_loop(__ucu_dst, __ucu_src, __ucu_len, u16, label); \ > - unsafe_copy_loop(__ucu_dst, __ucu_src, __ucu_len, u8, label); \ > +#define unsafe_copy_to_user(_dst, _src, _len, label) > \ > +do { > \ > + void __user *__dst = (_dst); > \ > + const void *__src = (_src); > \ > + size_t __len = (_len); > \ [Severity: Medium] Could this macro change cause dangerous variable shadowing? By renaming the internal variables from __ucu_dst to generic names like __dst, __src, and __len, if a caller passes arguments with those exact names, the macro will expand to something like void __user *__dst = (__dst);. This results in self-initialization using uninitialized stack memory, which could lead to arbitrary memory reads or writes when used with SMAP disabled. > + unsafe_copy_to_user_loop(__dst, __src, __len, u64, label); > \ > + unsafe_copy_to_user_loop(__dst, __src, __len, u32, label); > \ > + unsafe_copy_to_user_loop(__dst, __src, __len, u16, label); > \ > + unsafe_copy_to_user_loop(__dst, __src, __len, u8, label); > \ > +} while (0) > + > +#define unsafe_copy_from_user_loop(dst, src, len, type, label) > \ > + while (len >= sizeof(type)) { > \ > + unsafe_get_user(*(type *)(dst), (type __user *)(src), label); > \ > + dst += sizeof(type); > \ > + src += sizeof(type); > \ > + len -= sizeof(type); > \ > + } > + > +#define unsafe_copy_from_user(_dst, _src, _len, label) > \ > +do { > \ > + void *__dst = (_dst); > \ > + const void __user *__src = (_src); > \ > + size_t __len = (_len); > \ [Severity: Medium] Does this new macro suffer from the same variable shadowing risk? Using generic internal names like __dst, __src, and __len might cause accidental self-initialization if a caller happens to use the same variable names for the arguments. Should we use more uniquely prefixed names here similar to the old __ucu_dst? > + unsafe_copy_from_user_loop(__dst, __src, __len, u64, label); > \ > + unsafe_copy_from_user_loop(__dst, __src, __len, u32, label); > \ > + unsafe_copy_from_user_loop(__dst, __src, __len, u16, label); > \ > + unsafe_copy_from_user_loop(__dst, __src, __len, u8, label); > \ > } while (0) -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=18
