Re: [RFC PATCH v1 19/25] x86/uaccess: Add unsafe_copy_from_user() implementation

[sashiko-bot@kernel.org]

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 <jpoimboe@kernel.org>

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/20260818144954.2320378-1-jremus@linux.ibm.com?part=19