linux-kernel - Re: [PATCH v5 4/6] usb: gadget: add mechanism to specify an explicit status stage

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: <20190116050029.GA13084@localhost.localdomain>
Date:   Wed, 16 Jan 2019 00:00:29 -0500
From:   Paul Elder <paul.elder@...asonboard.com>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     laurent.pinchart@...asonboard.com, kieran.bingham@...asonboard.com,
        b-liu@...com, rogerq@...com, balbi@...nel.org,
        gregkh@...uxfoundation.org, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 4/6] usb: gadget: add mechanism to specify an explicit
 status stage

On Mon, Jan 14, 2019 at 10:24:44AM -0500, Alan Stern wrote:
> On Mon, 14 Jan 2019, Paul Elder wrote:
> 
> > > > > Can you check your uvc
> > > > > changes using dummy_hcd with the patch below?
> > > > 
> > > > I'm not sure what to make of the test results. I get the same results
> > > > with or without the patch. Which I guess makes sense... in dummy_queue,
> > > > this is getting hit when the uvc function driver tries to complete the
> > > > delayed status:
> > > > 
> > > > 	req = usb_request_to_dummy_request(_req);
> > > > 	if (!_req || !list_empty(&req->queue) || !_req->complete)
> > > > 		return -EINVAL;
> > > > 
> > > > So the delayed/explicit status stage is never completed, afaict.
> > > 
> > > I presume you are hitting the !list_empty(&req->queue) test, yes?  The 
> > > other two tests are trivial.
> > 
> > Yes, that is what's happening.
> > 
> > > Triggering the !list_empty() test means the request has already been
> > > submitted and not yet completed.  This probably indicates there is a
> > > bug in the uvc function driver code.
> > 
> > The uvc function driver works with musb, though :/
> > 
> > I compared the sequence of calls to the uvc setup, completion handler,
> > and status stage sending, and for some reason dummy_hcd, after an OUT
> > setup-completion-status sequence, calls a completion-status-completion
> > sequence, and then goes on the the next request. musb simply goes on to
> > the next request after the setup-completion-status sequence.
> 
> I don't quite understand.  There's a control-OUT transfer, the setup, 
> data, and status transactions all complete normally, and then what 
> happens?  What do you mean by "a completion-status-completion 
> sequence"?  A more detailed description would help.
> 

I meant the functions (procedures) in the function driver, so the setup
handler (uvc_function_setup), the completion handler
(uvc_function_ep0_complete), and the status sender (uvc_send_response),
although the last one actually sends the data stage for control IN.
So after the status is sent on the uvc gadget driver's end, its
completion handler is called again without the setup handler being
called beforehand and I cant figure out why.

> > I commented out the paranoia block in dummy_timer, and dummy_hcd still
> > does the extra completion, but it doesn't error out anymore. I doubt
> > that's the/a solution though, especially since I get:
> > 
> > [   22.616577] uvcvideo: Failed to query (129) UVC probe control : -75 (exp. 26).
> > [   22.624481] uvcvideo: Failed to initialize the device (-5).
> > 
> > Not sure if that's a result of dummy_hcd not supporting isochronous
> > transfers or not.
> > 
> > I'm not sure where to continue investigating :/
> 
> Perhaps removing the "#if 0" protecting the dev_dbg line in 
> dummy_queue() would provide some helpful output.

It did, but didn't get me much farther :/

> Another thing to check would be if the "implement an emulated 
> single-request FIFO" in dummy_queue() is causing trouble.  There's no 
> harm in replacing the long "if" condition with "if (0)".

That didn't change anything.

Although I did notice that the dummy_queue that calls the completion
handler without the preceeding setup handler says that it's in the
status stage (ep->status_stage == 1).


Thanks,

Paul