linux-kernel - RE: [PATCH 4.19 093/125] usb: gadget: configfs: fix concurrent issue between composite APIs

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]

Message-ID: <VI1PR04MB532778F3728AB7465D0892F38B710@VI1PR04MB5327.eurprd04.prod.outlook.com>
Date:   Thu, 14 Nov 2019 06:44:58 +0000
From:   Peter Chen <peter.chen@....com>
To:     Pavel Machek <pavel@...x.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>,
        Felipe Balbi <felipe.balbi@...ux.intel.com>,
        Sasha Levin <sashal@...nel.org>
Subject: RE: [PATCH 4.19 093/125] usb: gadget: configfs: fix concurrent issue
 between composite APIs

 
> >
> > We meet several NULL pointer issues if configfs_composite_unbind and
> > composite_setup (or composite_disconnect) are running together.
> > These issues occur when do the function switch stress test, the
> > configfs_compsoite_unbind is called from user mode by echo "" to
> > /sys/../UDC entry, and meanwhile, the setup interrupt or disconnect
> > interrupt occurs by hardware. The composite_setup
> 
> > +++ b/drivers/usb/gadget/configfs.c
> > @@ -61,6 +61,8 @@ struct gadget_info {
> >  	bool use_os_desc;
> >  	char b_vendor_code;
> >  	char qw_sign[OS_STRING_QW_SIGN_LEN];
> > +	spinlock_t spinlock;
> > +	bool unbind;
> >  };
> >
> >  static inline struct gadget_info *to_gadget_info(struct config_item
> > *item) @@ -1244,6 +1246,7 @@ static int configfs_composite_bind(struct
> usb_gadget *gadget,
> >  	int				ret;
> >
> >  	/* the gi->lock is hold by the caller */
> > +	gi->unbind = 0;
> >  	cdev->gadget = gadget;
> 
> Since variable is bool, I'd expect "= false" here?
> 
> > +	unsigned long flags;
> >
> >  	/* the gi->lock is hold by the caller */
> 
> "is held".
> 
> >  	cdev = get_gadget_data(gadget);
> >  	gi = container_of(cdev, struct gadget_info, cdev);
> > +	spin_lock_irqsave(&gi->spinlock, flags);
> > +	gi->unbind = 1;
> 
> = true;
> 
> > +static int configfs_composite_setup(struct usb_gadget *gadget,
> > +		const struct usb_ctrlrequest *ctrl) {
> > +	struct usb_composite_dev *cdev;
> > +	struct gadget_info *gi;
> > +	unsigned long flags;
> > +	int ret;
> > +
> > +	cdev = get_gadget_data(gadget);
> > +	if (!cdev)
> > +		return 0;
> > +
> > +	gi = container_of(cdev, struct gadget_info, cdev);
> > +	spin_lock_irqsave(&gi->spinlock, flags);
> > +	cdev = get_gadget_data(gadget);
> 
> cdev already contains required value, why get it second time? (If it needs to be
> done under lock, comment might be useful...)
> 

Hi Pavel,

Thanks for comment.

The reason is the cdev may be set to NULL by configfs_composite_unbind through
another process, eg, the user wants to disable current USB gadget function, 
this patch tries to fix such kinds of concurrent issue.

Peter