linux-kernel - Re: [PATCH 1/2] uaccess: Simplify code pattern for masked user copies

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: <20250209194756.4cd45e12@pumpkin>
Date: Sun, 9 Feb 2025 19:47:56 +0000
From: David Laight <david.laight.linux@...il.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, Alexander
 Viro <viro@...iv.linux.org.uk>, Christian Brauner <brauner@...nel.org>, Jan
 Kara <jack@...e.cz>, Arnd Bergmann <arnd@...db.de>, Kees Cook
 <kees@...nel.org>
Subject: Re: [PATCH 1/2] uaccess: Simplify code pattern for masked user
 copies

On Sun, 9 Feb 2025 09:40:05 -0800
Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> On Sun, 9 Feb 2025 at 02:56, David Laight <david.laight.linux@...il.com> wrote:
> >
> > Code can then be changed:
> > -               if (!user_read_access_begin(from, sizeof(*from)))
> > +               if (!masked_user_read_access_begin(&from, sizeof(*from)))
> >                         return -EFAULT;  
> 
> I really dislike the use of "pass pointer to simple variable you are
> going to change" interfaces which is why I didn't do it this way.

I'm not sure the 'goto' model works here.
The issue is that the calling code mustn't use the unmasked address.
You really want to make that as hard as possible.
So the 'function' really does need to do an in-situ update.

I did do a test compile without the &, it exploded but I didn't
check whether it always would.
IIRC there is a sparse check for 'user' pointers that would help.

Even with the current functions, someone is bound to write:
	if (!masked_user_access_begin(uaddr))
		return -EFAULT;
	unsafe_get_user(kaddr, uaddr, label);
and it will all appear to be fine...
(objtool might detect something because of the NULL pointer path.)

You almost need it to be 'void masked_user_access_begin(&uaddr)'.

	David