Re: [RFC PATCH v2 18/24] x86/uaccess: Add unsafe_copy_from_user() implementation

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