Re: [Linux-kernel-mentees] [PATCH] net: fix uninit value error in __sys_sendmmsg

[David Laight <David.Laight@ACULAB.COM>]

From: Greg KH
> Sent: 13 September 2020 07:14
> On Sun, Sep 13, 2020 at 11:26:39AM +0530, Anant Thazhemadam wrote:
> > The crash report showed that there was a local variable;
> >
> > ----iovstack.i@__sys_sendmmsg created at:
> >  ___sys_sendmsg net/socket.c:2388 [inline]
> >  __sys_sendmmsg+0x6db/0xc90 net/socket.c:2480
> >
> >  that was left uninitialized.
> >
> > The contents of iovstack are of interest, since the respective pointer
> > is passed down as an argument to sendmsg_copy_msghdr as well.
> > Initializing this contents of this stack prevents this bug from happening.
> >
> > Since the memory that was initialized is freed at the end of the function
> > call, memory leaks are not likely to be an issue.
> >
> > syzbot seems to have triggered this error by passing an array of 0's as
> > a parameter while making the initial system call.
> >
> > Reported-by: syzbot+09a5d591c1f98cf5efcb@syzkaller.appspotmail.com
> > Tested-by: syzbot+09a5d591c1f98cf5efcb@syzkaller.appspotmail.com
> > Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
> > ---
> >  net/socket.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/net/socket.c b/net/socket.c
> > index 0c0144604f81..d74443dfd73b 100644
> > --- a/net/socket.c
> > +++ b/net/socket.c
> > @@ -2396,6 +2396,7 @@ static int ___sys_sendmsg(struct socket *sock, struct user_msghdr __user *msg,
> >  {
> >  	struct sockaddr_storage address;
> >  	struct iovec iovstack[UIO_FASTIOV], *iov = iovstack;
> > +	memset(iov, 0, UIO_FASTIOV);
> >  	ssize_t err;
> >
> >  	msg_sys->msg_name = &address;
> 
> I don't think you built this code change, otherwise you would have seen
> that it adds a build warning to the system, right?

Also it can't be the right 'fix' for whatever sysbot found.
(I can't find the sysbot report.)

Zeroing iov[] just slows down a path that is already too slow because
of the contorted functions used to read in iov[].

If it does need to be zerod then it would be needed in a lot
of other code paths that read in iov[].

If a zero length iov[] needs converting into a single entity
with a zero length - then that needs to be done elsewhere.

I've a patch series I might redo that changes the code that
reads in iov[] to return the address of any buffer that
needed to be malloced (more than UIV_FASTIO buffers) rather
than using the iov parameter to pass in the cache and
return the buffer to free.
It would be less confusing and error prone.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees