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
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.