linux-kernel - Re: [PATCH] fs: correctly check for errors from replace_fd() in receive_fd

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: <20250801220215.GS222315@ZenIV>
Date: Fri, 1 Aug 2025 23:02:15 +0100
From: Al Viro <viro@...iv.linux.org.uk>
To: Jan Kara <jack@...e.cz>
Cc: Thomas Weißschuh <thomas.weissschuh@...utronix.de>,
	Christian Brauner <brauner@...nel.org>,
	Sargun Dhillon <sargun@...gun.me>, Kees Cook <kees@...nel.org>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	stable@...r.kernel.org
Subject: Re: [PATCH] fs: correctly check for errors from replace_fd() in
 receive_fd_replace()

On Fri, Aug 01, 2025 at 12:48:09PM +0200, Jan Kara wrote:
> On Fri 01-08-25 09:38:38, Thomas Weißschuh wrote:
> > replace_fd() returns either a negative error number or the number of the
> > new file descriptor. The current code misinterprets any positive file
> > descriptor number as an error.
> > 
> > Only check for negative error numbers, so that __receive_sock() is called
> > correctly for valid file descriptors.
> > 
> > Fixes: 173817151b15 ("fs: Expand __receive_fd() to accept existing fd")
> > Fixes: 42eb0d54c08a ("fs: split receive_fd_replace from __receive_fd")
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@...utronix.de>
> 
> Indeed. I'm wondering how come nobody noticed...

One word: seccomp.  Considering the background amount of bogus userland
behaviour coming with it, I wouldn't expect a... vigorous test coverage
for that one ;-/

It's definitely a bug that needs fixing, but I'm not sure this is the right
way to fix it.

Look: replace_fd(fd, file, flags) returns fd on success and -E... on failure.
Not a single user cares which non-negative value had been returned.  What's
more, "returns fd on success" is a side effect of using do_dup2() and
being lazy about it.

And the entire thing is not on any hot paths.  So I suspect that a better fix
would be

	err = do_dup2(files, file, fd, flags);
	if (err < 0)
		return err;
	return 0;

in replace_fd() in place of
	return do_dup2(files, file, fd, flags);

so we don't invite more surprises like that.