linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH v2] usb:host: fix divide-by-zero in function fhci_queue_urb
@ 2019-04-18  9:31 zhuyan (M)
  2019-04-18 14:33 ` Alan Stern
  0 siblings, 1 reply; 6+ messages in thread
From: zhuyan (M) @ 2019-04-18  9:31 UTC (permalink / raw)
  To: Alan Stern; +Cc: Greg KH, anton, linux-usb, linux-kernel

On Wed, 17 Apr 2019 14:59:27 -0400, Alan Stern wrote:
> 
> On Wed, 17 Apr 2019, zhuyan (M) wrote:
> 
> > On Wed, 17 Apr 2019, Alan Stern wrote:
> > 
> > > On Wed, 17 Apr 2019, zhuyan (M) wrote:
> > > 
> > > > On Tue, 16 Apr 2019 11:07:56 -0400, Alan Stern wrote:
> > > > 
> > > > > On Tue, 16 Apr 2019, zhuyan (M) wrote:
> > > > > > On Tue, 16 Apr 2019 at 11:45:45 +0200, Greg KH wrote:
> > > > > > > On Tue, Apr 09, 2019 at 10:37:12PM +0800, zhuyan wrote:
> > > > > > > > In function fhci_queue_urb, the divisor of expression 
> > > > > > > > (urb->transfer_buffer_length % usb_maxpacket(urb->dev,
> > > > > > > > urb->pipe,
> > > > > > > > usb_pipeout(urb->pipe))) may be zero.
> > > > > > > 
> > > > > > > How can you hit that?
> > > > > > > 
> > > > > > > > When it is zero, unexpected results may occur, so it is 
> > > > > > > > necessary to ensure that the divisor is not zero.
> > > > > > > > 
> > > > > > > > Signed-off-by: zhuyan <zhuyan34@huawei.com>
> > > > > > > 
> > > > > > > I need a "Full" name here, not just a single name.  Whatever you use to sign documents is good.
> > > > > > > 
> > > > > > > thanks,
> > > > > > > 
> > > > > > > greg k-h
> > > > > > 
> > > > > > In function usb_maxpacket, when ep is NULL, its return value is 0.  
> > > > > 
> > > > > fhci_queue_urb() shouldn't use urb->pipe to compute the 
> > > > > maxpacket size anyway.  It should use usb_endpoint_maxp(&urb->ep->desc).
> > > > 
> > > > Currently, fhci_queue_urb(), call usb_maxpacket() multiple times 
> > > > to calculate  the maxpacket size. The usb_maxpacket() will call
> > > > usb_endpoint_maxp() to compute the maxpacket size.
> > > 
> > > I know that.  What fhci_queue_urb() is doing is wrong.  You should change it: 
> > > Make it call usb_endpoint_maxp directly instead of calling usb_maxpacket.
> > > 
> > 
> > From 1996456d0cc17b5ff7746a598ff355b25d13db3e Mon Sep 17 00:00:00 2001
> > From: zhuyan <zhuyan34@huawei.com>
> > Date: Thu, 18 Apr 2019 00:53:03 +0800
> > Subject: [PATCH] usb: host: fix divide-by-zero in function 
> > fhci_queue_urb
> > 
> > fhci_queue_urb() shouldn't use urb->pipe to compute the maxpacket size 
> > anyway.It should use usb_endpoint_maxp(&urb->ep->desc).
> > 
> > In function fhci_queue_urb, the divisor of expression 
> > (urb->transfer_buffer_length % usb_maxpacket(urb->dev, urb->pipe,
> > usb_pipeout(urb->pipe))) may be zero. When it is zero, unexpected 
> > results may occur, so it is necessary to ensure that the divisor is not zero.
> > 
> > Signed-off-by: zhuyan <zhuyan34@huawei.com>
> > ---
> >  drivers/usb/host/fhci-sched.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/usb/host/fhci-sched.c 
> > b/drivers/usb/host/fhci-sched.c index 3d12cdd..7dcfe22 100644
> > --- a/drivers/usb/host/fhci-sched.c
> > +++ b/drivers/usb/host/fhci-sched.c
> > @@ -704,6 +704,7 @@ void fhci_queue_urb(struct fhci_hcd *fhci, struct urb *urb)
> >  	struct td *td;
> >  	u8 *data;
> >  	u16 cnt = 0;
> > +	u16 max_pkt_size = 0;
> >  
> >  	if (ed == NULL) {
> >  		ed = fhci_get_empty_ed(fhci);
> > @@ -727,8 +728,7 @@ void fhci_queue_urb(struct fhci_hcd *fhci, struct urb *urb)
> >  		}
> >  		ed->speed = (urb->dev->speed == USB_SPEED_LOW) ?
> >  			FHCI_LOW_SPEED : FHCI_FULL_SPEED;
> > -		ed->max_pkt_size = usb_maxpacket(urb->dev,
> > -			urb->pipe, usb_pipeout(urb->pipe));
> > +		ed->max_pkt_size = usb_endpoint_maxp(&urb->ep->desc);
> >  		urb->ep->hcpriv = ed;
> >  		fhci_dbg(fhci, "new ep speed=%d max_pkt_size=%d\n",
> >  			 ed->speed, ed->max_pkt_size);
> > @@ -765,11 +765,12 @@ void fhci_queue_urb(struct fhci_hcd *fhci, 
> > struct urb *urb)
> >  
> >  	switch (ed->mode) {
> >  	case FHCI_TF_BULK:
> > +		max_pkt_size = usb_endpoint_maxp(&urb->ep->desc);
> >  		if (urb->transfer_flags & URB_ZERO_PACKET &&
> >  				urb->transfer_buffer_length > 0 &&
> > +				(max_pkt_size != 0) &&
> 
> Now you shouldn't need to add this extra test.
> 
> Alan Stern
> 
> >  				((urb->transfer_buffer_length %
> > -				usb_maxpacket(urb->dev, urb->pipe,
> > -				usb_pipeout(urb->pipe))) == 0))
> > +				max_pkt_size) == 0))
> >  			urb_state = US_BULK0;
> >  		while (data_len > 4096) {
> >  			td = fhci_td_fill(fhci, urb, urb_priv, ed, cnt, @@ -807,8 +808,8 
> > @@ void fhci_queue_urb(struct fhci_hcd *fhci, struct urb *urb)
> >  		break;
> >  	case FHCI_TF_CTRL:
> >  		ed->dev_addr = usb_pipedevice(urb->pipe);
> > -		ed->max_pkt_size = usb_maxpacket(urb->dev, urb->pipe,
> > -			usb_pipeout(urb->pipe));
> > +		ed->max_pkt_size = usb_endpoint_maxp(&urb->ep->desc);
> > +
> >  		/* setup stage */
> >  		td = fhci_td_fill(fhci, urb, urb_priv, ed, cnt++, FHCI_TA_SETUP,
> >  			USB_TD_TOGGLE_DATA0, urb->setup_packet, 8, 0, 0, true);

But the return value of usb_endpoint_maxp() may be 0. 
Do we need to take protective measures?

Yan Zhu

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] usb:host: fix divide-by-zero in function fhci_queue_urb
  2019-04-18  9:31 [PATCH v2] usb:host: fix divide-by-zero in function fhci_queue_urb zhuyan (M)
@ 2019-04-18 14:33 ` Alan Stern
  0 siblings, 0 replies; 6+ messages in thread
From: Alan Stern @ 2019-04-18 14:33 UTC (permalink / raw)
  To: zhuyan (M); +Cc: Greg KH, anton, linux-usb, linux-kernel

On Thu, 18 Apr 2019, zhuyan (M) wrote:

> > > @@ -765,11 +765,12 @@ void fhci_queue_urb(struct fhci_hcd *fhci, 
> > > struct urb *urb)
> > >  
> > >  	switch (ed->mode) {
> > >  	case FHCI_TF_BULK:
> > > +		max_pkt_size = usb_endpoint_maxp(&urb->ep->desc);
> > >  		if (urb->transfer_flags & URB_ZERO_PACKET &&
> > >  				urb->transfer_buffer_length > 0 &&
> > > +				(max_pkt_size != 0) &&
> > 
> > Now you shouldn't need to add this extra test.
> > 
> > Alan Stern

> But the return value of usb_endpoint_maxp() may be 0. 
> Do we need to take protective measures?

Yes, we do.  But the protective measures don't belong in the fhci 
driver.

They should go in core/config.c:usb_parse_endpoint().  That routine 
already has code to check for maxpacket sizes that are too large; you 
can add code that checks for maxpacket sizes that are too small.

In theory an interrupt or isochronous endpoint might have maxpacket set
to 0 -- we could warn about these and accept them -- but this is not
allowed for bulk endpoints (see section 5.8.3 of the USB 2.0
specification).  In fact, bulk endpoints are not allowed to have
maxpacket size smaller than 8.

Maybe if you make this change then fhci-hcd won't need any
modifications at all.

Alan Stern


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] usb:host: fix divide-by-zero in function fhci_queue_urb
@ 2019-04-18  9:58 zhuyan (M)
  0 siblings, 0 replies; 6+ messages in thread
From: zhuyan (M) @ 2019-04-18  9:58 UTC (permalink / raw)
  To: Greg KH; +Cc: Alan Stern, anton, linux-usb, linux-kernel, zhuyan (M)

On Wed, 17 Apr 2019 21:49:03 +0200, Greg KH wrote:
> On Wed, Apr 17, 2019 at 05:05:33PM +0000, zhuyan (M) wrote:
> > On Wed, 17 Apr 2019, Alan Stern wrote:
> > 
> > > On Wed, 17 Apr 2019, zhuyan (M) wrote:
> > > 
> > > > On Tue, 16 Apr 2019 11:07:56 -0400, Alan Stern wrote:
> > > > 
> > > > > On Tue, 16 Apr 2019, zhuyan (M) wrote:
> > > > > > On Tue, 16 Apr 2019 at 11:45:45 +0200, Greg KH wrote:
> > > > > > > On Tue, Apr 09, 2019 at 10:37:12PM +0800, zhuyan wrote:
> > > > > > > > In function fhci_queue_urb, the divisor of expression 
> > > > > > > > (urb->transfer_buffer_length % usb_maxpacket(urb->dev,
> > > > > > > > urb->pipe,
> > > > > > > > usb_pipeout(urb->pipe))) may be zero.
> > > > > > > 
> > > > > > > How can you hit that?
> > > > > > > 
> > > > > > > > When it is zero, unexpected results may occur, so it is 
> > > > > > > > necessary to ensure that the divisor is not zero.
> > > > > > > > 
> > > > > > > > Signed-off-by: zhuyan <zhuyan34@huawei.com>
> > > > > > > 
> > > > > > > I need a "Full" name here, not just a single name.  Whatever you use to sign documents is good.
> > > > > > > 
> > > > > > > thanks,
> > > > > > > 
> > > > > > > greg k-h
> > > > > > 
> > > > > > In function usb_maxpacket, when ep is NULL, its return value is 0.  
> > > > > 
> > > > > fhci_queue_urb() shouldn't use urb->pipe to compute the 
> > > > > maxpacket size anyway.  It should use usb_endpoint_maxp(&urb->ep->desc).
> > > > 
> > > > Currently, fhci_queue_urb(), call usb_maxpacket() multiple times 
> > > > to calculate  the maxpacket size. The usb_maxpacket() will call
> > > > usb_endpoint_maxp() to compute the maxpacket size.
> > > 
> > > I know that.  What fhci_queue_urb() is doing is wrong.  You should change it: 
> > > Make it call usb_endpoint_maxp directly instead of calling usb_maxpacket.
> > > 
> > 
> > >From 1996456d0cc17b5ff7746a598ff355b25d13db3e Mon Sep 17 00:00:00 
> > >2001
> > From: zhuyan <zhuyan34@huawei.com>
> > Date: Thu, 18 Apr 2019 00:53:03 +0800
> > Subject: [PATCH] usb: host: fix divide-by-zero in function 
> > fhci_queue_urb
> > 
> > fhci_queue_urb() shouldn't use urb->pipe to compute the maxpacket size 
> > anyway.It should use usb_endpoint_maxp(&urb->ep->desc).
> > 
> > In function fhci_queue_urb, the divisor of expression 
> > (urb->transfer_buffer_length % usb_maxpacket(urb->dev, urb->pipe,
> > usb_pipeout(urb->pipe))) may be zero. When it is zero, unexpected 
> > results may occur, so it is necessary to ensure that the divisor is not zero.
> > 
> > Signed-off-by: zhuyan <zhuyan34@huawei.com>
> 
> I still need a full name here and on the From: line :(

I am so sorry. I will change it.

From 1996456d0cc17b5ff7746a598ff355b25d13db3e Mon Sep 17 00:00:00 2001
From: Yan Zhu <zhuyan34@huawei.com>
Date: Thu, 18 Apr 2019 00:53:03 +0800
Subject: [PATCH] usb: host: fix divide-by-zero in function fhci_queue_urb

fhci_queue_urb() shouldn't use urb->pipe to compute the maxpacket
size anyway.It should use usb_endpoint_maxp(&urb->ep->desc).

In function fhci_queue_urb, the divisor of expression
(urb->transfer_buffer_length % usb_maxpacket(urb->dev, urb->pipe,
usb_pipeout(urb->pipe))) may be zero. When it is zero, unexpected results
may occur, so it is necessary to ensure that the divisor is not zero.

Signed-off-by: Yan Zhu <zhuyan34@huawei.com>
---
 drivers/usb/host/fhci-sched.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/host/fhci-sched.c b/drivers/usb/host/fhci-sched.c
index 3d12cdd..7dcfe22 100644
--- a/drivers/usb/host/fhci-sched.c
+++ b/drivers/usb/host/fhci-sched.c
@@ -704,6 +704,7 @@ void fhci_queue_urb(struct fhci_hcd *fhci, struct urb *urb)
 	struct td *td;
 	u8 *data;
 	u16 cnt = 0;
+	u16 max_pkt_size = 0;
 
 	if (ed == NULL) {
 		ed = fhci_get_empty_ed(fhci);
@@ -727,8 +728,7 @@ void fhci_queue_urb(struct fhci_hcd *fhci, struct urb *urb)
 		}
 		ed->speed = (urb->dev->speed == USB_SPEED_LOW) ?
 			FHCI_LOW_SPEED : FHCI_FULL_SPEED;
-		ed->max_pkt_size = usb_maxpacket(urb->dev,
-			urb->pipe, usb_pipeout(urb->pipe));
+		ed->max_pkt_size = usb_endpoint_maxp(&urb->ep->desc);
 		urb->ep->hcpriv = ed;
 		fhci_dbg(fhci, "new ep speed=%d max_pkt_size=%d\n",
 			 ed->speed, ed->max_pkt_size);
@@ -765,11 +765,12 @@ void fhci_queue_urb(struct fhci_hcd *fhci, struct urb *urb)
 
 	switch (ed->mode) {
 	case FHCI_TF_BULK:
+		max_pkt_size = usb_endpoint_maxp(&urb->ep->desc);
 		if (urb->transfer_flags & URB_ZERO_PACKET &&
 				urb->transfer_buffer_length > 0 &&
+				(max_pkt_size != 0) &&
 				((urb->transfer_buffer_length %
-				usb_maxpacket(urb->dev, urb->pipe,
-				usb_pipeout(urb->pipe))) == 0))
+				max_pkt_size) == 0))
 			urb_state = US_BULK0;
 		while (data_len > 4096) {
 			td = fhci_td_fill(fhci, urb, urb_priv, ed, cnt,
@@ -807,8 +808,8 @@ void fhci_queue_urb(struct fhci_hcd *fhci, struct urb *urb)
 		break;
 	case FHCI_TF_CTRL:
 		ed->dev_addr = usb_pipedevice(urb->pipe);
-		ed->max_pkt_size = usb_maxpacket(urb->dev, urb->pipe,
-			usb_pipeout(urb->pipe));
+		ed->max_pkt_size = usb_endpoint_maxp(&urb->ep->desc);
+
 		/* setup stage */
 		td = fhci_td_fill(fhci, urb, urb_priv, ed, cnt++, FHCI_TA_SETUP,
 			USB_TD_TOGGLE_DATA0, urb->setup_packet, 8, 0, 0, true);
-- 
1.8.5.6





^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] usb:host: fix divide-by-zero in function fhci_queue_urb
  2019-04-17 17:05 zhuyan (M)
  2019-04-17 18:59 ` Alan Stern
@ 2019-04-17 19:49 ` Greg KH
  1 sibling, 0 replies; 6+ messages in thread
From: Greg KH @ 2019-04-17 19:49 UTC (permalink / raw)
  To: zhuyan (M); +Cc: Alan Stern, anton, linux-usb, linux-kernel

On Wed, Apr 17, 2019 at 05:05:33PM +0000, zhuyan (M) wrote:
> On Wed, 17 Apr 2019, Alan Stern wrote:
> 
> > On Wed, 17 Apr 2019, zhuyan (M) wrote:
> > 
> > > On Tue, 16 Apr 2019 11:07:56 -0400, Alan Stern wrote:
> > > 
> > > > On Tue, 16 Apr 2019, zhuyan (M) wrote:
> > > > > On Tue, 16 Apr 2019 at 11:45:45 +0200, Greg KH wrote:
> > > > > > On Tue, Apr 09, 2019 at 10:37:12PM +0800, zhuyan wrote:
> > > > > > > In function fhci_queue_urb, the divisor of expression 
> > > > > > > (urb->transfer_buffer_length % usb_maxpacket(urb->dev, 
> > > > > > > urb->pipe,
> > > > > > > usb_pipeout(urb->pipe))) may be zero.
> > > > > > 
> > > > > > How can you hit that?
> > > > > > 
> > > > > > > When it is zero, unexpected results may occur, so it is 
> > > > > > > necessary to ensure that the divisor is not zero.
> > > > > > > 
> > > > > > > Signed-off-by: zhuyan <zhuyan34@huawei.com>
> > > > > > 
> > > > > > I need a "Full" name here, not just a single name.  Whatever you use to sign documents is good.
> > > > > > 
> > > > > > thanks,
> > > > > > 
> > > > > > greg k-h
> > > > > 
> > > > > In function usb_maxpacket, when ep is NULL, its return value is 0.  
> > > > 
> > > > fhci_queue_urb() shouldn't use urb->pipe to compute the maxpacket 
> > > > size anyway.  It should use usb_endpoint_maxp(&urb->ep->desc).
> > > 
> > > Currently, fhci_queue_urb(), call usb_maxpacket() multiple times to 
> > > calculate  the maxpacket size. The usb_maxpacket() will call 
> > > usb_endpoint_maxp() to compute the maxpacket size.
> > 
> > I know that.  What fhci_queue_urb() is doing is wrong.  You should change it: 
> > Make it call usb_endpoint_maxp directly instead of calling usb_maxpacket.
> > 
> 
> >From 1996456d0cc17b5ff7746a598ff355b25d13db3e Mon Sep 17 00:00:00 2001
> From: zhuyan <zhuyan34@huawei.com>
> Date: Thu, 18 Apr 2019 00:53:03 +0800
> Subject: [PATCH] usb: host: fix divide-by-zero in function fhci_queue_urb
> 
> fhci_queue_urb() shouldn't use urb->pipe to compute the maxpacket
> size anyway.It should use usb_endpoint_maxp(&urb->ep->desc).
> 
> In function fhci_queue_urb, the divisor of expression
> (urb->transfer_buffer_length % usb_maxpacket(urb->dev, urb->pipe,
> usb_pipeout(urb->pipe))) may be zero. When it is zero, unexpected results
> may occur, so it is necessary to ensure that the divisor is not zero.
> 
> Signed-off-by: zhuyan <zhuyan34@huawei.com>

I still need a full name here and on the From: line :(

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] usb:host: fix divide-by-zero in function fhci_queue_urb
  2019-04-17 17:05 zhuyan (M)
@ 2019-04-17 18:59 ` Alan Stern
  2019-04-17 19:49 ` Greg KH
  1 sibling, 0 replies; 6+ messages in thread
From: Alan Stern @ 2019-04-17 18:59 UTC (permalink / raw)
  To: zhuyan (M); +Cc: Greg KH, anton, linux-usb, linux-kernel

On Wed, 17 Apr 2019, zhuyan (M) wrote:

> On Wed, 17 Apr 2019, Alan Stern wrote:
> 
> > On Wed, 17 Apr 2019, zhuyan (M) wrote:
> > 
> > > On Tue, 16 Apr 2019 11:07:56 -0400, Alan Stern wrote:
> > > 
> > > > On Tue, 16 Apr 2019, zhuyan (M) wrote:
> > > > > On Tue, 16 Apr 2019 at 11:45:45 +0200, Greg KH wrote:
> > > > > > On Tue, Apr 09, 2019 at 10:37:12PM +0800, zhuyan wrote:
> > > > > > > In function fhci_queue_urb, the divisor of expression 
> > > > > > > (urb->transfer_buffer_length % usb_maxpacket(urb->dev, 
> > > > > > > urb->pipe,
> > > > > > > usb_pipeout(urb->pipe))) may be zero.
> > > > > > 
> > > > > > How can you hit that?
> > > > > > 
> > > > > > > When it is zero, unexpected results may occur, so it is 
> > > > > > > necessary to ensure that the divisor is not zero.
> > > > > > > 
> > > > > > > Signed-off-by: zhuyan <zhuyan34@huawei.com>
> > > > > > 
> > > > > > I need a "Full" name here, not just a single name.  Whatever you use to sign documents is good.
> > > > > > 
> > > > > > thanks,
> > > > > > 
> > > > > > greg k-h
> > > > > 
> > > > > In function usb_maxpacket, when ep is NULL, its return value is 0.  
> > > > 
> > > > fhci_queue_urb() shouldn't use urb->pipe to compute the maxpacket 
> > > > size anyway.  It should use usb_endpoint_maxp(&urb->ep->desc).
> > > 
> > > Currently, fhci_queue_urb(), call usb_maxpacket() multiple times to 
> > > calculate  the maxpacket size. The usb_maxpacket() will call 
> > > usb_endpoint_maxp() to compute the maxpacket size.
> > 
> > I know that.  What fhci_queue_urb() is doing is wrong.  You should change it: 
> > Make it call usb_endpoint_maxp directly instead of calling usb_maxpacket.
> > 
> 
> From 1996456d0cc17b5ff7746a598ff355b25d13db3e Mon Sep 17 00:00:00 2001
> From: zhuyan <zhuyan34@huawei.com>
> Date: Thu, 18 Apr 2019 00:53:03 +0800
> Subject: [PATCH] usb: host: fix divide-by-zero in function fhci_queue_urb
> 
> fhci_queue_urb() shouldn't use urb->pipe to compute the maxpacket
> size anyway.It should use usb_endpoint_maxp(&urb->ep->desc).
> 
> In function fhci_queue_urb, the divisor of expression
> (urb->transfer_buffer_length % usb_maxpacket(urb->dev, urb->pipe,
> usb_pipeout(urb->pipe))) may be zero. When it is zero, unexpected results
> may occur, so it is necessary to ensure that the divisor is not zero.
> 
> Signed-off-by: zhuyan <zhuyan34@huawei.com>
> ---
>  drivers/usb/host/fhci-sched.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/host/fhci-sched.c b/drivers/usb/host/fhci-sched.c
> index 3d12cdd..7dcfe22 100644
> --- a/drivers/usb/host/fhci-sched.c
> +++ b/drivers/usb/host/fhci-sched.c
> @@ -704,6 +704,7 @@ void fhci_queue_urb(struct fhci_hcd *fhci, struct urb *urb)
>  	struct td *td;
>  	u8 *data;
>  	u16 cnt = 0;
> +	u16 max_pkt_size = 0;
>  
>  	if (ed == NULL) {
>  		ed = fhci_get_empty_ed(fhci);
> @@ -727,8 +728,7 @@ void fhci_queue_urb(struct fhci_hcd *fhci, struct urb *urb)
>  		}
>  		ed->speed = (urb->dev->speed == USB_SPEED_LOW) ?
>  			FHCI_LOW_SPEED : FHCI_FULL_SPEED;
> -		ed->max_pkt_size = usb_maxpacket(urb->dev,
> -			urb->pipe, usb_pipeout(urb->pipe));
> +		ed->max_pkt_size = usb_endpoint_maxp(&urb->ep->desc);
>  		urb->ep->hcpriv = ed;
>  		fhci_dbg(fhci, "new ep speed=%d max_pkt_size=%d\n",
>  			 ed->speed, ed->max_pkt_size);
> @@ -765,11 +765,12 @@ void fhci_queue_urb(struct fhci_hcd *fhci, struct urb *urb)
>  
>  	switch (ed->mode) {
>  	case FHCI_TF_BULK:
> +		max_pkt_size = usb_endpoint_maxp(&urb->ep->desc);
>  		if (urb->transfer_flags & URB_ZERO_PACKET &&
>  				urb->transfer_buffer_length > 0 &&
> +				(max_pkt_size != 0) &&

Now you shouldn't need to add this extra test.

Alan Stern

>  				((urb->transfer_buffer_length %
> -				usb_maxpacket(urb->dev, urb->pipe,
> -				usb_pipeout(urb->pipe))) == 0))
> +				max_pkt_size) == 0))
>  			urb_state = US_BULK0;
>  		while (data_len > 4096) {
>  			td = fhci_td_fill(fhci, urb, urb_priv, ed, cnt,
> @@ -807,8 +808,8 @@ void fhci_queue_urb(struct fhci_hcd *fhci, struct urb *urb)
>  		break;
>  	case FHCI_TF_CTRL:
>  		ed->dev_addr = usb_pipedevice(urb->pipe);
> -		ed->max_pkt_size = usb_maxpacket(urb->dev, urb->pipe,
> -			usb_pipeout(urb->pipe));
> +		ed->max_pkt_size = usb_endpoint_maxp(&urb->ep->desc);
> +
>  		/* setup stage */
>  		td = fhci_td_fill(fhci, urb, urb_priv, ed, cnt++, FHCI_TA_SETUP,
>  			USB_TD_TOGGLE_DATA0, urb->setup_packet, 8, 0, 0, true);


^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH v2] usb:host: fix divide-by-zero in function fhci_queue_urb
@ 2019-04-17 17:05 zhuyan (M)
  2019-04-17 18:59 ` Alan Stern
  2019-04-17 19:49 ` Greg KH
  0 siblings, 2 replies; 6+ messages in thread
From: zhuyan (M) @ 2019-04-17 17:05 UTC (permalink / raw)
  To: Alan Stern; +Cc: Greg KH, anton, linux-usb, linux-kernel

On Wed, 17 Apr 2019, Alan Stern wrote:

> On Wed, 17 Apr 2019, zhuyan (M) wrote:
> 
> > On Tue, 16 Apr 2019 11:07:56 -0400, Alan Stern wrote:
> > 
> > > On Tue, 16 Apr 2019, zhuyan (M) wrote:
> > > > On Tue, 16 Apr 2019 at 11:45:45 +0200, Greg KH wrote:
> > > > > On Tue, Apr 09, 2019 at 10:37:12PM +0800, zhuyan wrote:
> > > > > > In function fhci_queue_urb, the divisor of expression 
> > > > > > (urb->transfer_buffer_length % usb_maxpacket(urb->dev, 
> > > > > > urb->pipe,
> > > > > > usb_pipeout(urb->pipe))) may be zero.
> > > > > 
> > > > > How can you hit that?
> > > > > 
> > > > > > When it is zero, unexpected results may occur, so it is 
> > > > > > necessary to ensure that the divisor is not zero.
> > > > > > 
> > > > > > Signed-off-by: zhuyan <zhuyan34@huawei.com>
> > > > > 
> > > > > I need a "Full" name here, not just a single name.  Whatever you use to sign documents is good.
> > > > > 
> > > > > thanks,
> > > > > 
> > > > > greg k-h
> > > > 
> > > > In function usb_maxpacket, when ep is NULL, its return value is 0.  
> > > 
> > > fhci_queue_urb() shouldn't use urb->pipe to compute the maxpacket 
> > > size anyway.  It should use usb_endpoint_maxp(&urb->ep->desc).
> > 
> > Currently, fhci_queue_urb(), call usb_maxpacket() multiple times to 
> > calculate  the maxpacket size. The usb_maxpacket() will call 
> > usb_endpoint_maxp() to compute the maxpacket size.
> 
> I know that.  What fhci_queue_urb() is doing is wrong.  You should change it: 
> Make it call usb_endpoint_maxp directly instead of calling usb_maxpacket.
> 

From 1996456d0cc17b5ff7746a598ff355b25d13db3e Mon Sep 17 00:00:00 2001
From: zhuyan <zhuyan34@huawei.com>
Date: Thu, 18 Apr 2019 00:53:03 +0800
Subject: [PATCH] usb: host: fix divide-by-zero in function fhci_queue_urb

fhci_queue_urb() shouldn't use urb->pipe to compute the maxpacket
size anyway.It should use usb_endpoint_maxp(&urb->ep->desc).

In function fhci_queue_urb, the divisor of expression
(urb->transfer_buffer_length % usb_maxpacket(urb->dev, urb->pipe,
usb_pipeout(urb->pipe))) may be zero. When it is zero, unexpected results
may occur, so it is necessary to ensure that the divisor is not zero.

Signed-off-by: zhuyan <zhuyan34@huawei.com>
---
 drivers/usb/host/fhci-sched.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/host/fhci-sched.c b/drivers/usb/host/fhci-sched.c
index 3d12cdd..7dcfe22 100644
--- a/drivers/usb/host/fhci-sched.c
+++ b/drivers/usb/host/fhci-sched.c
@@ -704,6 +704,7 @@ void fhci_queue_urb(struct fhci_hcd *fhci, struct urb *urb)
 	struct td *td;
 	u8 *data;
 	u16 cnt = 0;
+	u16 max_pkt_size = 0;
 
 	if (ed == NULL) {
 		ed = fhci_get_empty_ed(fhci);
@@ -727,8 +728,7 @@ void fhci_queue_urb(struct fhci_hcd *fhci, struct urb *urb)
 		}
 		ed->speed = (urb->dev->speed == USB_SPEED_LOW) ?
 			FHCI_LOW_SPEED : FHCI_FULL_SPEED;
-		ed->max_pkt_size = usb_maxpacket(urb->dev,
-			urb->pipe, usb_pipeout(urb->pipe));
+		ed->max_pkt_size = usb_endpoint_maxp(&urb->ep->desc);
 		urb->ep->hcpriv = ed;
 		fhci_dbg(fhci, "new ep speed=%d max_pkt_size=%d\n",
 			 ed->speed, ed->max_pkt_size);
@@ -765,11 +765,12 @@ void fhci_queue_urb(struct fhci_hcd *fhci, struct urb *urb)
 
 	switch (ed->mode) {
 	case FHCI_TF_BULK:
+		max_pkt_size = usb_endpoint_maxp(&urb->ep->desc);
 		if (urb->transfer_flags & URB_ZERO_PACKET &&
 				urb->transfer_buffer_length > 0 &&
+				(max_pkt_size != 0) &&
 				((urb->transfer_buffer_length %
-				usb_maxpacket(urb->dev, urb->pipe,
-				usb_pipeout(urb->pipe))) == 0))
+				max_pkt_size) == 0))
 			urb_state = US_BULK0;
 		while (data_len > 4096) {
 			td = fhci_td_fill(fhci, urb, urb_priv, ed, cnt,
@@ -807,8 +808,8 @@ void fhci_queue_urb(struct fhci_hcd *fhci, struct urb *urb)
 		break;
 	case FHCI_TF_CTRL:
 		ed->dev_addr = usb_pipedevice(urb->pipe);
-		ed->max_pkt_size = usb_maxpacket(urb->dev, urb->pipe,
-			usb_pipeout(urb->pipe));
+		ed->max_pkt_size = usb_endpoint_maxp(&urb->ep->desc);
+
 		/* setup stage */
 		td = fhci_td_fill(fhci, urb, urb_priv, ed, cnt++, FHCI_TA_SETUP,
 			USB_TD_TOGGLE_DATA0, urb->setup_packet, 8, 0, 0, true);
-- 
1.8.5.6


^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-04-18 14:33 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-18  9:31 [PATCH v2] usb:host: fix divide-by-zero in function fhci_queue_urb zhuyan (M)
2019-04-18 14:33 ` Alan Stern
  -- strict thread matches above, loose matches on Subject: below --
2019-04-18  9:58 zhuyan (M)
2019-04-17 17:05 zhuyan (M)
2019-04-17 18:59 ` Alan Stern
2019-04-17 19:49 ` Greg KH

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).