netdev - Re: [PATCH v3 01/28] net/socket.c: switch to CLASS(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: <20241103063113.GR1350452@ZenIV>
Date: Sun, 3 Nov 2024 06:31:13 +0000
From: Al Viro <viro@...iv.linux.org.uk>
To: Simon Horman <horms@...nel.org>
Cc: linux-fsdevel@...r.kernel.org, brauner@...nel.org,
	cgroups@...r.kernel.org, kvm@...r.kernel.org,
	netdev@...r.kernel.org, torvalds@...ux-foundation.org
Subject: Re: [PATCH v3 01/28] net/socket.c: switch to CLASS(fd)

On Sat, Nov 02, 2024 at 12:21:32PM +0000, Simon Horman wrote:

> > @@ -2926,16 +2900,18 @@ static int do_recvmmsg(int fd, struct mmsghdr __user *mmsg,
> >  
> >  	datagrams = 0;
> >  
> > -	sock = sockfd_lookup_light(fd, &err, &fput_needed);
> > -	if (!sock)
> > -		return err;
> > +	CLASS(fd, f)(fd);
> > +
> > +	if (fd_empty(f))
> > +		return -EBADF;
> > +	sock = sock_from_file(fd_file(f));
> > +	if (unlikely(!sock))
> > +		return -ENOTSOCK;
> 
> Hi Al,
> 
> There is an unconditional check on err down on line 2977.
> However, with the above change err is now only conditionally
> set before we reach that line. Are you sure that it will always
> be initialised by the time line 2977 is reached?

Nice catch, thank you.  It is possible, if you call recvmmsg(2) with
zero vlen and MSG_ERRQUEUE in flags.  Which is not going to be done in
any well-behaving code, making it really nasty - nothing like a kernel
bug that shows up only when trying to narrow down a userland bug upstream
of the syscall in question ;-/

AFAICS, that's the only bug of that sort in this commit - all other
places that used to rely upon successful sockfd_lookup_light() zeroing
err have an unconditional assignment to err shortly downstream of that.

Fix folded into commit in question, branch force-pushed; incremental follows
(I would rather not spam the lists with repost of the entire patchset for
the sake of that):

diff --git a/net/socket.c b/net/socket.c
index fb3806a11f94..c3ac02d060c0 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -2885,7 +2885,7 @@ static int do_recvmmsg(int fd, struct mmsghdr __user *mmsg,
 			  unsigned int vlen, unsigned int flags,
 			  struct timespec64 *timeout)
 {
-	int err, datagrams;
+	int err = 0, datagrams;
 	struct socket *sock;
 	struct mmsghdr __user *entry;
 	struct compat_mmsghdr __user *compat_entry;