linux-kernel - Re: [patch V3 07/12] uaccess: Provide scoped masked user access regions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8ff80802-33af-4432-a783-50c4ad7fe984@efficios.com>
Date: Fri, 17 Oct 2025 09:23:44 -0400
From: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To: Thomas Gleixner <tglx@...utronix.de>, LKML <linux-kernel@...r.kernel.org>
Cc: Christophe Leroy <christophe.leroy@...roup.eu>,
 Andrew Cooper <andrew.cooper3@...rix.com>,
 Linus Torvalds <torvalds@...ux-foundation.org>,
 kernel test robot <lkp@...el.com>, Russell King <linux@...linux.org.uk>,
 linux-arm-kernel@...ts.infradead.org, x86@...nel.org,
 Madhavan Srinivasan <maddy@...ux.ibm.com>,
 Michael Ellerman <mpe@...erman.id.au>, Nicholas Piggin <npiggin@...il.com>,
 linuxppc-dev@...ts.ozlabs.org, Paul Walmsley <pjw@...nel.org>,
 Palmer Dabbelt <palmer@...belt.com>, linux-riscv@...ts.infradead.org,
 Heiko Carstens <hca@...ux.ibm.com>,
 Christian Borntraeger <borntraeger@...ux.ibm.com>,
 Sven Schnelle <svens@...ux.ibm.com>, linux-s390@...r.kernel.org,
 Julia Lawall <Julia.Lawall@...ia.fr>, Nicolas Palix <nicolas.palix@...g.fr>,
 Peter Zijlstra <peterz@...radead.org>, Darren Hart <dvhart@...radead.org>,
 Davidlohr Bueso <dave@...olabs.net>, André Almeida
 <andrealmeid@...lia.com>, Alexander Viro <viro@...iv.linux.org.uk>,
 Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
 linux-fsdevel@...r.kernel.org
Subject: Re: [patch V3 07/12] uaccess: Provide scoped masked user access
 regions

On 2025-10-17 06:09, Thomas Gleixner wrote:

> +/**
> + * __scoped_user_access_begin - Start the masked user access
> + * @_mode:	The mode of the access class (read, write, rw)
> + * @_uptr:	The pointer to access user space memory
> + * @_size:	Size of the access
> + * @_elbl:	Error label to goto when the access region is rejected.
> + *
> + * Internal helper for __scoped_masked_user_access(). Don't use directly
> + */

^ general comment about ending sentences with '.' across this patch
(nit).


> +#define __scoped_user_access_begin(_mode, _uptr, _size, _elbl)		\
> +({									\
> +	typeof((_uptr)) ____ret;					\
> +									\
> +	if (can_do_masked_user_access()) {				\
> +		____ret = masked_user_##_mode##_access_begin((_uptr));	\

I don't think the extra () are needed here, or is there something
special happening within this macro that requires it ?

> +	} else {							\
> +		____ret = _uptr;					\
> +		if (!user_##_mode##_access_begin(_uptr, (_size)))	\

likewise around _size.

> +*/
> +#define __scoped_masked_user_access(_mode, _uptr, _size, _elbl)					\
> +for (bool ____stop = false; !____stop; ____stop = true)						\
> +	for (typeof((_uptr)) _tmpptr = __scoped_user_access_begin(_mode, _uptr, _size, _elbl);	\

The extra () around _uptr seems useless.

> +	     !____stop; ____stop = true)							\
> +		for (CLASS(masked_user_##_mode##_access, scope) (_tmpptr); !____stop;		\

Removing the space before (_tmpptr) would make it clearer that it
behaves as arguments to CLASS(masked_user_##_mode##_access, scope),
similarly to what is done in cleanup.h:scoped_class().

Nesting those constructs will cause variables to be hidden by inner
definitions. I recommend using __UNIQUE_ID() to make sure the "stop" and
"tmpptr" variables don't clash with external ones rather than trying to
solve the issue with a random amount of leading underscores.

> +		     ____stop = true)					\
> +			/* Force modified pointer usage within the scope */			\
> +			for (const typeof((_uptr)) _uptr = _tmpptr; !____stop; ____stop = true)	\

I'm puzzled that it does not trigger compiler warnings as it shadows
_uptr if _uptr is a variable defined outside of this scope.

> +				if (1)
> +

^ can be removed (as pointed out by someone else already).

[...]
> +#define scoped_masked_user_read_access_size(_usrc, _size, _elbl)		\
> +	__scoped_masked_user_access(read, (_usrc), (_size), _elbl)

Useless () around _usrc and _size.


> +#define scoped_masked_user_read_access(_usrc, _elbl)				\
> +	scoped_masked_user_read_access_size((_usrc), sizeof(*(_usrc)), _elbl)

() around the first argument are useless.


> +#define scoped_masked_user_write_access_size(_udst, _size, _elbl)		\
> +	__scoped_masked_user_access(write, (_udst),  (_size), _elbl)

Useless () around _udst and _size.

> + */
> +#define scoped_masked_user_write_access(_udst, _elbl)				\
> +	scoped_masked_user_write_access_size((_udst), sizeof(*(_udst)), _elbl)

() around the first argument are useless.

> +#define scoped_masked_user_rw_access_size(_uptr, _size, _elbl)			\
> +	__scoped_masked_user_access(rw, (_uptr), (_size), _elbl)

Useless () around _uptr and _size.

> +#define scoped_masked_user_rw_access(_uptr, _elbl)				\
> +	scoped_masked_user_rw_access_size((_uptr), sizeof(*(_uptr)), _elbl)

() around the first argument are useless.

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com