linux-kernel - Re: [PATCH] usb: dwc3: ep0: fix delayed status is queued too early

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: <20140508043902.GI19925@intel.com>
Date:	Thu, 8 May 2014 00:39:02 -0400
From:	Zhuang Jin Can <jin.can.zhuang@...el.com>
To:	Alan Stern <stern@...land.harvard.edu>
Cc:	Felipe Balbi <balbi@...com>, USB list <linux-usb@...r.kernel.org>,
	linux-omap@...r.kernel.org,
	Kernel development list <linux-kernel@...r.kernel.org>,
	liping.zhou@...el.com, david.a.cohen@...ux.intel.com
Subject: Re: [PATCH] usb: dwc3: ep0: fix delayed status is queued too early

On Wed, May 07, 2014 at 11:03:42AM -0400, Alan Stern wrote:
> On Wed, 7 May 2014, Zhuang Jin Can wrote:
> 
> > A delayed status request may be queued before composite framework returns
> > USB_GADGET_DELAYED_STATUS, because the thread queueing the request can run
> > on a different core in parallel with the control request irq.
> > 
> > SETUP XferComplete IRQ        		fsg_main_thread
> > ----------------------        		---------------
> > 	|					|
> > spin_lock_irqsave(&dwc->lock)               sleeping
> > 	|					|
> > 	...					...
> > dwc3_ep0_inspect_setup()			|
> > 	|					|
> > dwc3_ep0_delegate_req() 			|
> > 	|					|
> > 	...					|
> > spin_unlock(&dwc->lock);			|
> > 	|					|
> > fsg_set_alt()   ======> Signal Wakeup ====>	|
> > 	| 					|
> > other gadgets->set_alt() 	       handle exception
> > 	|					|
> > 	|			usb_composite_setup_continue()
> > 	|					|
> > 	|			spin_lock_irqsave(&dwc->lock)
> >   	|			     __dwc3_gadget_ep0_queue()
> > 	|				 delay_status is false
> > 	|			spin_unlock_irqrestore(&dwc->lock)
> > 	|					|
> > 	|				     sleeping
> > spin_lock(&dwc->lock); 				|
> > 	|					|
> > delayed_status=true				|
> > 	|					|
> > 
> > 		STATUS XferNotReady IRQ
> > 		------------------------
> > 			|
> > 		dwc3_ep0_xfernotready()
> > 			|
> > 		   delayed_status is true, return;
> > 
> > The result is the status packet will never be transferred, and
> > delayed_status is not cleared.
> > 
> > Signed-off-by: Zhuang Jin Can <jin.can.zhuang@...el.com>
> > Reported-by: Zhou Liping <liping.zhou@...el.com>
> 
> A similar problem can occur in the opposite sense: The thread queuing
> the delayed status request might be delayed for so long that another
> SETUP packet arrives from the host first.  In that case, the delayed
> status request is a response for a stale transfer, so it must not be
> sent to the host.
> 
> Do dwc3 and composite.c handle this case correctly?
> 
So the situation you describe is that we get the STATUS XferNotReady
event, but gadget queues a status request when control transfer already
failed. dwc3 can't move to SETUP phase until the status request arrives,
so any SETUP transaction from host will fail. If status request
eventually arrives, it already missed the first control transfer, and
I don't know how the controller will behave. If we still can get a
STATUS XferComplete event without actually transfer anything on the
bus, then we can move back to SETUP PHASE which will remove the stale
delayed status request and start the new SETUP transaction. But I think
in this situation, the host should already lose it patience and start
to reset the bus.

Per my understanding, it's impossible for dwc3 to send a stale STATUS
request for a new SETUP transaction. 

> Back in the old g_file_storage driver, I addressed this issue by
> keeping a counter of all the setup requests.  When it came time to send
> a delayed status response, the response would be sent only if the
> counter had not changed from when the original setup request was
> received.
> 
> As far as I can see, composite.c doesn't do anything like that.
> 
> Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/