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: <20241106100337.GL4507@kernel.org>
Date: Wed, 6 Nov 2024 10:03:37 +0000
From: Simon Horman <horms@...nel.org>
To: Al Viro <viro@...iv.linux.org.uk>
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 Sun, Nov 03, 2024 at 06:31:13AM +0000, Al Viro wrote:
> 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 ;-/

Ouch.

> 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.

Yes, I was unable to find any others either.

> 
> 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):

Thanks, will run some checks for good measure.
But the fix below looks good to me.

> 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;
>