linux-kernel - Re: KMSAN: uninit-value in _copy_to

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] [day] [month] [year] [list]

Date:   Thu, 7 Jun 2018 22:29:13 +0300
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     syzbot <syzbot+87cfa083e727a224754b@...kaller.appspotmail.com>,
        avagin@...nvz.org, davem@...emloft.net, dingtianhong@...wei.com,
        edumazet@...gle.com, elena.reshetova@...el.com,
        jasowang@...hat.com, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, matthew@...systems.ca,
        mingo@...nel.org, netdev@...r.kernel.org, pabeni@...hat.com,
        syzkaller-bugs@...glegroups.com,
        virtualization@...ts.linux-foundation.org, willemb@...gle.com
Subject: Re: KMSAN: uninit-value in _copy_to_iter (2)

On Thu, Jun 07, 2018 at 07:04:49PM +0100, Al Viro wrote:
> On Thu, Jun 07, 2018 at 08:59:06PM +0300, Michael S. Tsirkin wrote:
> > On Thu, Jun 07, 2018 at 06:43:55PM +0100, Al Viro wrote:
> > > On Thu, Jun 07, 2018 at 06:38:48PM +0300, Michael S. Tsirkin wrote:
> > > > #syz test: https://github.com/google/kmsan.git/master d2d741e5d1898dfde1a75ea3d29a9a3e2edf0617
> > > > 
> > > > Subject: vhost: fix info leak
> > > > 
> > > > Fixes: CVE-2018-1118
> > > > Signed-off-by: Michael S. Tsirkin <mst@...hat.com>
> > > > ---
> > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > > index f0be5f35ab28..9beefa6ed1ce 100644
> > > > --- a/drivers/vhost/vhost.c
> > > > +++ b/drivers/vhost/vhost.c
> > > > @@ -2345,6 +2345,9 @@ struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type)
> > > >  	struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL);
> > > >  	if (!node)
> > > >  		return NULL;
> > > > +
> > > > +	/* Make sure all padding within the structure is initialized. */
> > > > +	memset(&node->msg, 0, sizeof node->msg);
> > > 
> > > Umm...  Maybe kzalloc(), then?  You have
> > > 
> > > struct vhost_msg_node {
> > >   struct vhost_msg msg;
> > >   struct vhost_virtqueue *vq;
> > >   struct list_head node;
> > > };
> > > 
> > > and that's what, 68 bytes in msg, then either 4 bytes pointer or
> > > 4 bytes padding + 8 bytes pointer, then two pointers?  How much
> > > does explicit partial memset() save you here?
> > 
> > Yes but 0 isn't a nop here so if this struct is used without
> > a sensible initialization, it will crash elsewhere.
> > I prefer KASAN to catch such uses.
> > 
> > 
> > > >  	node->vq = vq;
> > > >  	node->msg.type = type;
> 
> IDGI - what would your variant catch that kzalloc + 2 assignments won't?
> Accesses to uninitialized ->node?  Because that's the only difference in
> what is and is not initialized between those variants...

For now yes but we'll likely add more fields in this structure
down the road, which is where I'd expect new bugs to come from.

-- 
MST