linux-kernel - Re: [PATCH v4 5/6] add listmount(2) syscall

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]

Date: Thu, 11 Jan 2024 12:14:07 -0800
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Guenter Roeck <linux@...ck-us.net>, Yoshinori Sato <ysato@...rs.sourceforge.jp>, 
	Rich Felker <dalias@...c.org>, John Paul Adrian Glaubitz <glaubitz@...sik.fu-berlin.de>
Cc: Miklos Szeredi <mszeredi@...hat.com>, linux-fsdevel@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-api@...r.kernel.org, 
	linux-man@...r.kernel.org, linux-security-module@...r.kernel.org, 
	Karel Zak <kzak@...hat.com>, Ian Kent <raven@...maw.net>, David Howells <dhowells@...hat.com>, 
	Al Viro <viro@...iv.linux.org.uk>, Christian Brauner <christian@...uner.io>, 
	Amir Goldstein <amir73il@...il.com>, Matthew House <mattlloydhouse@...il.com>, 
	Florian Weimer <fweimer@...hat.com>, Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH v4 5/6] add listmount(2) syscall

On Thu, 11 Jan 2024 at 10:57, Guenter Roeck <linux@...ck-us.net> wrote:
>
> Any variance of put_user() with &buf[ctr] or buf + ctr fails
> if ctr is a variable and permitted to be != 0.

Crazy. But the 64-bit put_user() is a bit special and tends to require
more registers (the 64-bit value is passed in two registers), so that
probably then results in the ICE.

Side note: looking at the SH version of __put_user_u64(), I think it's
buggy and is missing the exception handler for the second 32-bit move.
I dunno, I don't read sh asm, but it looks suspicious.

> The following works. Would this be acceptable ?

It might be very easy to trigger this once again if somebody goes "that's silly"

That said, I also absolutely detest the "error handling" in that
function. It's horrible.

Noticing the user access error in the middle is just sad, and if that
was just handled better and at least the range was checked first, the
overflow error couldn't happen and checking for it is thus pointless.

And looking at it all, it really looks like the whole interface is
broken. The "bufsize" argument isn't the size of the buffer at all.
It's the number of entries.

Extra confusingly, in the *other* system call, bufsize is in fact the
size of the buffer.

And the 'ctr' overflow checking is doubly garbage, because the only
reason *that* can happen is that we didn't check the incoming
arguments properly.

Same goes for the whole array_index_nospec() - it's pointless, because
the user controls what that code checks against anyway, so there's no
point to trying to manage some range checking.

The only range checking there that matters would be the one that
put_user() has to do against the address space size, but that's done
by put_user().

End result: that thing needs a rewrite.

The SH put_user64() needs to be looked at too, but in the meantime,
maybe something like this fixes the problems with listmount?

NOTE! ENTIRELY untested, but that naming and lack of argument sanity
checking really is horrendous. We should have caught this earlier.

                   Linus

View attachment "patch.diff" of type "text/x-patch" (2265 bytes)