linux-kernel - RE: [PATCH] Drivers: hv: util: on deinit, don't wait the release event, if we shouldn't

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, 28 Feb 2017 12:42:30 +0000
From:   Dexuan Cui <decui@...rosoft.com>
To:     Vitaly Kuznetsov <vkuznets@...hat.com>
CC:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "driverdev-devel@...uxdriverproject.org" 
        <driverdev-devel@...uxdriverproject.org>,
        KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Alex Ng (LIS)" <alexng@...rosoft.com>
Subject: RE: [PATCH] Drivers: hv: util: on deinit, don't wait the release
 event, if we shouldn't

> From: Vitaly Kuznetsov [mailto:vkuznets@...hat.com]
> >  void hv_fcopy_deinit(void)
> >  {
> > +	bool wait = hvt->dev_opened;
> > +
> >  	fcopy_transaction.state = HVUTIL_DEVICE_DYING;
> >  	cancel_delayed_work_sync(&fcopy_timeout_work);
> >  	hvutil_transport_destroy(hvt);
> > -	wait_for_completion(&release_event);
> > +	if (wait)
> > +		wait_for_completion(&release_event);
> 
> This is racy I think. We need to prevent openning the device first and
> then query its state:
> 
> 	bool wait;
> 
>  	fcopy_transaction.state = HVUTIL_DEVICE_DYING;
>         /* make sure state is set */
>         mb();
>         wait = hvt->dev_opened;
>   	cancel_delayed_work_sync(&fcopy_timeout_work);
>   	hvutil_transport_destroy(hvt);
>         if (wait)
> 		wait_for_completion(&release_event);
> 
> otherwise someone could open the device before we manage to update its
> state.

I agree.

> > @@ -182,6 +183,7 @@ static int hvt_op_release(struct inode *inode,
> struct file *file)
> >  	 * connects back.
> >  	 */
> >  	hvt_reset(hvt);
> > +	hvt->dev_opened = false;
> >  	mutex_unlock(&hvt->lock);
> >
> 
> Not sure but it seems this may also be racy (what if we query the state
> just before we reset it?).

Yeah, I agree.

> >  	if (mode_old == HVUTIL_TRANSPORT_DESTROY)
> > diff --git a/drivers/hv/hv_utils_transport.h
> b/drivers/hv/hv_utils_transport.h
> > index d98f522..9871283 100644
> > --- a/drivers/hv/hv_utils_transport.h
> > +++ b/drivers/hv/hv_utils_transport.h
> > @@ -32,6 +32,7 @@ struct hvutil_transport {
> >  	int mode;                           /* hvutil_transport_mode */
> >  	struct file_operations fops;        /* file operations */
> >  	struct miscdevice mdev;             /* misc device */
> > +	bool   dev_opened;                  /* Is the device opened? */
> >  	struct cb_id cn_id;                 /* CN_*_IDX/CN_*_VAL */
> >  	struct list_head list;              /* hvt_list */
> >  	int (*on_msg)(void *, int);         /* callback on new user message */
> 
> I think we can get away without introducing this new flag, e.g. if we
> replace release_event with an atomic which will hold the state
> (open/closed). This will also elimenate possible races above. I can try
> prototyping a patch if you want me to.
> --
>   Vitaly

Thanks for offering the help! Please do. :-)

Thanks,
-- Dexuan