linux-kernel - Re: [PATCH v4 5/6] usb: musb: gadget: implement optional 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: <Pine.LNX.4.44L0.1901061449520.18206-100000@netrider.rowland.org>
Date:   Sun, 6 Jan 2019 15:03:09 -0500 (EST)
From:   Alan Stern <stern@...land.harvard.edu>
To:     Paul Elder <paul.elder@...asonboard.com>
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 v4 5/6] usb: musb: gadget: implement optional explicit
 status stage

On Sun, 6 Jan 2019, Paul Elder wrote:

> Implement the mechanism for optional explicit status stage for the MUSB
> driver. This allows a function driver to specify what to reply for the
> status stage. The functionality for an implicit status stage is
> retained.
> 
> Signed-off-by: Paul Elder <paul.elder@...asonboard.com>
> v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> v1 Acked-by: Bin Liu <b-liu@...com>
> ---
> No change from v3
> 
> Changes from v2:
> - update call to usb_gadget_control_complete to include status
> - since sending STALL from the function driver is now done with
>   usb_ep_set_halt, there is no need for the internal ep0_send_response to
>   take a stall/ack parameter; remove the parameter and make the function
>   only send ack, and remove checking for the status reply in the
>   usb_request for the status stage
> 
> Changes from v1:
> - obvious change to implement v2 mechanism laid out by 4/6 of this
>   series (send_response, and musb_g_ep0_send_response function has
>   been removed, call to usb_gadget_control_complete has been added)
> - ep0_send_response's ack argument has been changed from stall
> - last_packet flag in ep0_rxstate has been removed, since it is equal to
>   req != NULL
> 
>  drivers/usb/musb/musb_gadget.c     |  2 ++
>  drivers/usb/musb/musb_gadget_ep0.c | 23 +++++++++++++++++++++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> index d3f33f449445..a7a992ab0c9d 100644
> --- a/drivers/usb/musb/musb_gadget.c
> +++ b/drivers/usb/musb/musb_gadget.c
> @@ -145,6 +145,8 @@ __acquires(ep->musb->lock)
>  
>  	trace_musb_req_gb(req);
>  	usb_gadget_giveback_request(&req->ep->end_point, &req->request);
> +	usb_gadget_control_complete(&musb->g, request->explicit_status,
> +			request->status);

I haven't paid much attention to this part of the patch series, not 
knowing much about musb.  Still, it's clear that 
usb_gadget_control_complete should be called only for transfers on 
ep0.  You need to test the endpoint value.

Another problem: the completion handler may deallocate the request.  
Dereferencing request->expicit_status and request->status would then 
cause errors.  Would it be preferable to call 
usb_gadget_control_complete before usb_gadget_giveback_request?  If 
it gets done that way then the arguments could be simplified: we could 
pass a pointer to the request instead of the separate explicit_status 
and status values.

Alan Stern