linux-kernel - Re: [PATCH] Drivers: hv: vmbus: Add mutex lock to channel show functions

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]

Date:   Tue, 22 Jan 2019 01:42:46 -0500
From:   Kimberly Brown <kimbrownkd@...il.com>
To:     Dexuan Cui <decui@...rosoft.com>
Cc:     Michael Kelley <mikelley@...rosoft.com>,
        Long Li <longli@...rosoft.com>,
        Sasha Levin <Alexander.Levin@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        "devel@...uxdriverproject.org" <devel@...uxdriverproject.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Drivers: hv: vmbus: Add mutex lock to channel show
 functions

On Tue, Jan 22, 2019 at 03:46:48AM +0000, Dexuan Cui wrote:
> > From: Kimberly Brown <kimbrownkd@...il.com>
> > Sent: Monday, January 21, 2019 6:08 PM
> > Subject: [PATCH] Drivers: hv: vmbus: Add mutex lock to channel show functions
> > 
> > The channel level "_show" functions are vulnerable to race conditions.
> > Add a mutex lock and unlock around the call to the channel level "_show"
> > functions in vmbus_chan_attr_show().
> > 
> > This problem was discussed here:
> > https://lkml.org/lkml/2018/10/18/830
> > 
> > --- a/drivers/hv/vmbus_drv.c
> > +++ b/drivers/hv/vmbus_drv.c
> > @@ -1414,6 +1414,7 @@ static ssize_t vmbus_chan_attr_show(struct kobject
> > *kobj,
> >  		= container_of(attr, struct vmbus_chan_attribute, attr);
> >  	const struct vmbus_channel *chan
> >  		= container_of(kobj, struct vmbus_channel, kobj);
> > +	ssize_t ret;
> > 
> >  	if (!attribute->show)
> >  		return -EIO;
> > @@ -1421,7 +1422,10 @@ static ssize_t vmbus_chan_attr_show(struct
> > kobject *kobj,
> >  	if (chan->state != CHANNEL_OPENED_STATE)
> >  		return -EINVAL;
> > 
> > -	return attribute->show(chan, buf);
> > +	mutex_lock(&vmbus_connection.channel_mutex);
> > +	ret = attribute->show(chan, buf);
> > +	mutex_unlock(&vmbus_connection.channel_mutex);
> > +	return ret;
> >  }
> 
> It looks this patch is already able to fix the original issue:
> 6712cc9c2211 ("vmbus: don't return values for uninitalized channels"),
> as chan->state can't be CHANNEL_OPENED_STATE when
> channel->ringbuffer_page is not set up yet, or has been freed.
> 
> Thanks,
> -- Dexuan

I think that patch 6712cc9c2211 fixes the problem when the channel is
not set up yet, but it doesn't fix the problem when the channel is being
closed. The channel could be freed after the check that "chan->state" is
CHANNEL_OPENED_STATE, while the "attribute->show()" function is running.

Actually, there should be checks that "chan" is not null and that
"chan->state" is CHANNEL_OPENED_STATE within the locked section. I'll
need to fix that.