linux-kernel - RE: [Linux-kernel-mentees] [PATCH] net: fix uninit value error in __sys

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <346bcf816616429abb01a475dd8d87fc@AcuMS.aculab.com>
Date:   Mon, 14 Sep 2020 07:58:54 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Greg KH' <greg@...ah.com>,
        Anant Thazhemadam <anant.thazhemadam@...il.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        "syzbot+09a5d591c1f98cf5efcb@...kaller.appspotmail.com" 
        <syzbot+09a5d591c1f98cf5efcb@...kaller.appspotmail.com>,
        "David S. Miller" <davem@...emloft.net>,
        "linux-kernel-mentees@...ts.linuxfoundation.org" 
        <linux-kernel-mentees@...ts.linuxfoundation.org>
Subject: RE: [Linux-kernel-mentees] [PATCH] net: fix uninit value error in
 __sys_sendmmsg

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@...ys_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@...kaller.appspotmail.com
> > Tested-by: syzbot+09a5d591c1f98cf5efcb@...kaller.appspotmail.com
> > Signed-off-by: Anant Thazhemadam <anant.thazhemadam@...il.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)