Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] The `unsafe_copy_to_user` and `unsafe_copy_from_user` macros use dangerously common local variable names (`__dst`, `__src`, `__len`) which can silently shadow caller variables. --
--- Patch [19]: [RFC PATCH v1 19/25] x86/uaccess: Add unsafe_copy_from_user() implementation --- commit 83e5aa488f68698873050ec00671487d755924f5 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(). It also updates the local variables in the unsafe_copy_to_user() macro. > diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h > --- 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 these generic variable names (__dst, __src, __len) silently shadow variables in the caller's scope? If a caller passes a variable named __len as the _len argument, the macro will expand to size_t __len = (__len);. This declares a new block-scoped variable and initializes it with its own uninitialized garbage value from the stack. The copy loop would then use this uninitialized length, potentially resulting in an out-of-bounds memory copy. Does it make sense to retain a unique prefix like the previous __ucu_ variable names to prevent macro shadowing? > + 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(_dst, _src, _len, label) > \ > +do { > \ > + void *__dst = (_dst); > \ > + const void __user *__src = (_src); > \ > + size_t __len = (_len); > \ [Severity: Medium] Similarly, does the new unsafe_copy_from_user() macro share the same shadowing vulnerability? > + 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=19
