linux-kernel - Re: [PATCH v2] usb: gadget: eem: fix echo command packet response issue

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] [day] [month] [year] [list]

Message-ID: <YMitW5K85kGTqPTf@kroah.com>
Date:   Tue, 15 Jun 2021 15:38:35 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Linyu Yuan <linyyuan@...eaurora.org>
Cc:     Felipe Balbi <balbi@...nel.org>, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org, Linyu Yuan <linyyuan@...eaurora.com>
Subject: Re: [PATCH v2] usb: gadget: eem: fix echo command packet response
 issue

On Tue, Jun 15, 2021 at 07:41:33AM +0800, Linyu Yuan wrote:
> From: Linyu Yuan <linyyuan@...eaurora.com>
> 
> when receive eem echo command, it will send a response,
> but queue this response to the usb request which allocate
> from gadget device endpoint zero,
> and transmit the request to IN endpoint of eem interface.
> 
> on dwc3 gadget, it will trigger following warning in function
> __dwc3_gadget_ep_queue(),
> 
> 	if (WARN(req->dep != dep, "request %pK belongs to '%s'\n",
> 				&req->request, req->dep->name))
> 		return -EINVAL;
> 
> fix it by allocating a usb request from IN endpoint of eem interface,
> and transmit the usb request to same IN endpoint of eem interface.
> 
> Signed-off-by: Linyu Yuan <linyyuan@...eaurora.com>
> ---
> 
> v2: fix mail format and expand commit message
> 
>  drivers/usb/gadget/function/f_eem.c | 44 +++++++++++++++++++++++++----
>  1 file changed, 39 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_eem.c b/drivers/usb/gadget/function/f_eem.c
> index 2cd9942707b4..7de355c63189 100644
> --- a/drivers/usb/gadget/function/f_eem.c
> +++ b/drivers/usb/gadget/function/f_eem.c
> @@ -30,6 +30,11 @@ struct f_eem {
>  	u8				ctrl_id;
>  };
>  
> +struct in_context {
> +	struct sk_buff	*skb;
> +	struct usb_ep	*ep;
> +};
> +
>  static inline struct f_eem *func_to_eem(struct usb_function *f)
>  {
>  	return container_of(f, struct f_eem, port.func);
> @@ -320,9 +325,12 @@ static int eem_bind(struct usb_configuration *c, struct usb_function *f)
>  
>  static void eem_cmd_complete(struct usb_ep *ep, struct usb_request *req)
>  {
> -	struct sk_buff *skb = (struct sk_buff *)req->context;
> +	struct in_context *ctx = req->context;
>  
> -	dev_kfree_skb_any(skb);
> +	dev_kfree_skb_any(ctx->skb);
> +	kfree(req->buf);
> +	usb_ep_free_request(ctx->ep, req);
> +	kfree(ctx);
>  }
>  
>  /*
> @@ -410,7 +418,9 @@ static int eem_unwrap(struct gether *port,
>  		 * b15:		bmType (0 == data, 1 == command)
>  		 */
>  		if (header & BIT(15)) {
> -			struct usb_request	*req = cdev->req;
> +			struct usb_request	*req;
> +			struct in_context	*ctx;
> +			struct usb_ep		*ep;
>  			u16			bmEEMCmd;
>  
>  			/* EEM command packet format:
> @@ -439,13 +449,37 @@ static int eem_unwrap(struct gether *port,
>  				skb_trim(skb2, len);
>  				put_unaligned_le16(BIT(15) | BIT(11) | len,
>  							skb_push(skb2, 2));
> +
> +				ep = port->in_ep;
> +				req = usb_ep_alloc_request(ep, GFP_ATOMIC);
> +				if (!req)
> +					goto freeskb;
> +
> +				ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
> +				if (!ctx)
> +					goto freereq;
> +				ctx->skb = skb2;
> +				ctx->ep = ep;
> +
> +				req->buf = kmalloc(skb2->len, GFP_KERNEL);
> +				if (!req->buf)
> +					goto freectx;
> +
>  				skb_copy_bits(skb2, 0, req->buf, skb2->len);
>  				req->length = skb2->len;
>  				req->complete = eem_cmd_complete;
>  				req->zero = 1;
> -				req->context = skb2;
> -				if (usb_ep_queue(port->in_ep, req, GFP_ATOMIC))
> +				req->context = ctx;
> +				if (usb_ep_queue(ep, req, GFP_ATOMIC)) {
>  					DBG(cdev, "echo response queue fail\n");
> +					kfree(req->buf);
> +freectx:
> +					kfree(ctx);
> +freereq:
> +					usb_ep_free_request(ep, req);
> +freeskb:
> +					dev_kfree_skb_any(skb2);

As much fun as it is jumping to the middle of a case statement, are you
sure you want to maintain this?

And you do not return an error?  Why not?  What happens then?

thanks,

greg k-h