Re: [PATCH v14 04/19] x86/uaccess: Add unsafe_copy_from_user() implementation

Re: [PATCH v14 04/19] x86/uaccess: Add unsafe_copy_from_user() implementation

[Jens Remus]

On 5/5/2026 8:22 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Variable shadowing hazard in unsafe_copy_to_user and unsafe_copy_from_user macros.
> - [Low] Missing const qualifier on the source pointer in unsafe_copy_from_user.
> --
> 
> Note: The format of this report is altered due to recitation restrictions.
> The original patch code is not quoted directly, and a free-form summary
> of the review findings is provided instead.
> 
> commit b652061f302d029392e67f46699e5a2db0abfaca
> Author: Josh Poimboeuf <jpoimboe@kernel.org>
> 
> x86/uaccess: Add unsafe_copy_from_user() implementation
> 
> This commit introduces the unsafe_copy_from_user() macro for x86 architectures.
> It also refactors the existing unsafe_copy_to_user() macro to use updated
> internal variable names.
> 
> Regarding the variable naming in the updated unsafe_copy_to_user() and the
> new unsafe_copy_from_user() macros, is there a risk of variable shadowing?
> 
> The macros declare local variables with very common names, specifically __dst,
> __src, and __len. If a caller passes an argument that happens to share one of
> these names, the local variable declaration will shadow the caller's argument.
> For instance, if a caller passes __len, the initialization size_t __len =
> (_len); will evaluate the right-hand side as the newly declared uninitialized
> local variable itself. Could this lead to the loop copying a random length of
> memory based on stack garbage instead of the intended length? The previous
> implementation seemed to avoid this by using uniquely prefixed names like
> __ucu_len.

Please advise.

> Additionally, looking at the unsafe_copy_from_user() macro, the user-space
> source pointer is initialized as a void pointer without a const qualifier.
> 
> Since memory read operations typically expect the source buffer to be
> read-only, callers might pass a const void pointer. Does this implicit
> dropping of the const qualifier trigger a discarded-qualifiers compiler
> warning? Could the source pointer be declared as a const void pointer
> to prevent potential build failures when warnings are treated as errors?

Makes sense.
Thanks and regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303)
jremus@de.ibm.com / jremus@linux.ibm.com

IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Ehningen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/

Re: [PATCH v14 04/19] x86/uaccess: Add unsafe_copy_from_user() implementation

[Steven Rostedt]

On Wed, 6 May 2026 16:13:25 +0200
Jens Remus <jremus@linux.ibm.com> wrote:

> > Regarding the variable naming in the updated unsafe_copy_to_user() and the
> > new unsafe_copy_from_user() macros, is there a risk of variable shadowing?
> > 
> > The macros declare local variables with very common names, specifically __dst,
> > __src, and __len. If a caller passes an argument that happens to share one of
> > these names, the local variable declaration will shadow the caller's argument.
> > For instance, if a caller passes __len, the initialization size_t __len =
> > (_len); will evaluate the right-hand side as the newly declared uninitialized
> > local variable itself. Could this lead to the loop copying a random length of
> > memory based on stack garbage instead of the intended length? The previous
> > implementation seemed to avoid this by using uniquely prefixed names like
> > __ucu_len.  
> 
> Please advise.

There is an effort to get rid of common variable names in macros that
start with underscores. But honestly, that can be dealt with later, as
it's a problem with other macros in this header.

Feel free to ignore this suggestion.

-- Steve