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