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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190114051113.GD32268@localhost.localdomain>
Date:   Mon, 14 Jan 2019 00:11:13 -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 Fri, Jan 11, 2019 at 10:50:11AM -0500, Alan Stern wrote:
> On Fri, 11 Jan 2019, Paul Elder wrote:
> 
> > On Wed, Jan 09, 2019 at 02:06:31PM -0500, Alan Stern wrote:
> > > On Wed, 9 Jan 2019, Paul Elder wrote:
> > > 
> > > > A usb gadget function driver may or may not want to delay the status
> > > > stage of a control OUT request. An instance where it might want to is to
> > > > asynchronously validate the data of a class-specific request.
> > > > 
> > > > A function driver that wants an explicit status stage should set the
> > > > newly added explicit_status flag of the usb_request corresponding to the
> > > > data stage. Later on, the function driver can explicitly complete the
> > > > status stage by enqueueing a usb_request for ACK, or calling
> > > > usb_ep_set_halt() for STALL.
> > > > 
> > > > To support both explicit and implicit status stages, a UDC driver must
> > > > call the newly added usb_gadget_control_complete function right before
> > > > calling usb_gadget_giveback_request. To support the explicit status
> > > > stage, it might then check what stage the usb_request was queued in, and
> > > > for control IN ACK the host's zero-length data packet, or for control
> > > > OUT send a zero-length DATA1 ACK packet.
> > > > 
> > > > Signed-off-by: Paul Elder <paul.elder@...asonboard.com>
> > > > v4 Acked-by: Alan Stern <stern@...land.harvard.edu>
> > > > v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> > > 
> > > This looks good and has passed my tests so far.
> > 
> > Good! Thank you :)
> > 
> > > 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 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 :/


Thanks,

Paul