[PATCH 0/2] usb: ehci: Prevent possible modulo by zero

[PATCH 0/2] usb: ehci: Prevent possible modulo by zero

[Khalid Masum]

These two patches prevents two modulo by zero error.

Khalid Masum (2):
  usb: ehci: Prevent possible modulo by zero
  usb: ehci: Prevent possible modulo by zero

 drivers/usb/host/ehci-q.c | 4 ++++
 1 file changed, 4 insertions(+)

-- 
2.37.1

[PATCH 1/2] usb: ehci: Prevent possible modulo by zero

[Khalid Masum]

usb_maxpacket() returns 0 if it fails to fetch the endpoint. This
value is later used for calculating modulo. Which can cause modulo
by zero in qtd_fill and qh_urb_transaction.

Prevent this breakage by returning if maxpacket is found to be 0.

Fixes coverity warning: 744857 ("Division or modulo by zero")
Signed-off-by: Khalid Masum <khalid.masum.92@gmail.com>
---
 drivers/usb/host/ehci-q.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
index 807e64991e3e..eb31d13e9ecd 100644
--- a/drivers/usb/host/ehci-q.c
+++ b/drivers/usb/host/ehci-q.c
@@ -646,6 +646,8 @@ qh_urb_transaction (
 	/* else it's already initted to "out" pid (0 << 8) */
 
 	maxpacket = usb_maxpacket(urb->dev, urb->pipe);
+	if (unlikely(!maxpacket))
+		return NULL;
 
 	/*
 	 * buffer gets wrapped in one or more qtds;
-- 
2.37.1

[PATCH 2/2] usb: ehci: Prevent possible modulo by zero

[Khalid Masum]

usb_maxpacket() returns 0 if it fails to fetch the endpoint. This
value is later used for calculating modulo. Which can cause modulo
by zero in qtd_fill.

Prevent this breakage by returning if maxpacket is found to be 0.

Fixes coverity warning: 1487371 ("Division or modulo by zero")
Fixes: 9841f37a1cca ("usb: ehci: Add support for SINGLE_STEP_SET_FEATURE test of EHSET")
Signed-off-by: Khalid Masum <khalid.masum.92@gmail.com>
---
 drivers/usb/host/ehci-q.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
index eb31d13e9ecd..cf2585e9a09f 100644
--- a/drivers/usb/host/ehci-q.c
+++ b/drivers/usb/host/ehci-q.c
@@ -1221,6 +1221,8 @@ static int ehci_submit_single_step_set_feature(
 	token |= (1 /* "in" */ << 8);  /*This is IN stage*/
 
 	maxpacket = usb_maxpacket(urb->dev, urb->pipe);
+	if (unlikely(!maxpacket))
+		return -1;
 
 	qtd_fill(ehci, qtd, buf, len, token, maxpacket);
 
-- 
2.37.1

Re: [PATCH 1/2] usb: ehci: Prevent possible modulo by zero

[Alan Stern]

On Wed, Aug 24, 2022 at 12:27:57AM +0600, Khalid Masum wrote:
> usb_maxpacket() returns 0 if it fails to fetch the endpoint. This
> value is later used for calculating modulo. Which can cause modulo
> by zero in qtd_fill and qh_urb_transaction.
> 
> Prevent this breakage by returning if maxpacket is found to be 0.
> 
> Fixes coverity warning: 744857 ("Division or modulo by zero")
> Signed-off-by: Khalid Masum <khalid.masum.92@gmail.com>

I'm sure we've seen at least one patch doing this submitted in the past.  
It was wrong then and it's wrong now.  Coverity doesn't have a full 
understanding of how the kernel's USB subsystem works and sometimes it 
makes mistakes.

In short, qh_urb_transaction() can be called only by pathways that pass 
through usb_submit_urb(), which already includes this check:

	ep = usb_pipe_endpoint(dev, urb->pipe);
	if (!ep)
		return -ENOENT;

There's no need to check it again in the ehci-hcd driver.


On Wed, Aug 24, 2022 at 12:27:58AM +0600, Khalid Masum wrote:
> usb_maxpacket() returns 0 if it fails to fetch the endpoint. This
> value is later used for calculating modulo. Which can cause modulo
> by zero in qtd_fill.
>
> Prevent this breakage by returning if maxpacket is found to be 0.
>
> Fixes coverity warning: 1487371 ("Division or modulo by zero")
> Fixes: 9841f37a1cca ("usb: ehci: Add support for SINGLE_STEP_SET_FEATURE test of EHSET")
> Signed-off-by: Khalid Masum <khalid.masum.92@gmail.com>

This also is unnecessary.  Calls to 
ehci_submit_single_step_set_feature() have to pass through 
request_single_step_set_feature_urb(), which already includes this 
check:

	if (!ep) {
		usb_free_urb(urb);
		return NULL;
	}

Neither of these patches is needed.

Alan Stern

Re: [PATCH 2/2] usb: ehci: Prevent possible modulo by zero

[Greg Kroah-Hartman]

On Wed, Aug 24, 2022 at 12:27:58AM +0600, Khalid Masum wrote:
> usb_maxpacket() returns 0 if it fails to fetch the endpoint. This
> value is later used for calculating modulo. Which can cause modulo
> by zero in qtd_fill.
> 
> Prevent this breakage by returning if maxpacket is found to be 0.
> 
> Fixes coverity warning: 1487371 ("Division or modulo by zero")

Odd tag format, is that in the documentation?

> Fixes: 9841f37a1cca ("usb: ehci: Add support for SINGLE_STEP_SET_FEATURE test of EHSET")
> Signed-off-by: Khalid Masum <khalid.masum.92@gmail.com>
> ---
>  drivers/usb/host/ehci-q.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
> index eb31d13e9ecd..cf2585e9a09f 100644
> --- a/drivers/usb/host/ehci-q.c
> +++ b/drivers/usb/host/ehci-q.c
> @@ -1221,6 +1221,8 @@ static int ehci_submit_single_step_set_feature(
>  	token |= (1 /* "in" */ << 8);  /*This is IN stage*/
>  
>  	maxpacket = usb_maxpacket(urb->dev, urb->pipe);
> +	if (unlikely(!maxpacket))

You only ever use likely/unlikely if you can document how it matters
with a benchmark or other way to notice the difference.  Otherwise let
the compiler and the CPU do their magic, they know how to do this better
than us.

> +		return -1;

A real error number should be returned here if this was valid.

But as Alan said, coverity is often wrong, and unless you can prove
otherwise, this patch isn't valid.

thanks,

greg k-h

Re: [PATCH 1/2] usb: ehci: Prevent possible modulo by zero

[Khalid Masum]

On Wed, Aug 24, 2022 at 2:21 AM Alan Stern <stern@rowland.harvard.edu> wrote:
>
>         if (!ep) {
>                 usb_free_urb(urb);
>                 return NULL;
>         }
>
> Neither of these patches is needed.
>
> Alan Stern

Thanks, I got you.

  -- Khalid Masum

Re: [PATCH 2/2] usb: ehci: Prevent possible modulo by zero

[Khalid Masum]

On Wed, Aug 24, 2022 at 11:56 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:

>

> Odd tag format, is that in the documentation?

You are right. I should have used "Addresses-coverity".

> You only ever use likely/unlikely if you can document how it matters
> with a benchmark or other way to notice the difference.  Otherwise let
> the compiler and the CPU do their magic, they know how to do this better
> than us.

Thanks for the important information.
>
> > +             return -1;

I noticed. The function returns -1 on failure, everywhere so I used that.
I guess making them return correct error numbers using macros would
be a patch.
>
> A real error number should be returned here if this was valid.
>
> But as Alan said, coverity is often wrong, and unless you can prove
> otherwise, this patch isn't valid.

Got you.
>
> thanks,
>
> greg k-h

  -- Khalid Masum

Re: [PATCH 1/2] usb: ehci: Prevent possible modulo by zero

[Alan Stern]

On Wed, Aug 24, 2022 at 05:15:47PM +0600, Khalid Masum wrote:
> On Wed, Aug 24, 2022 at 2:21 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> >         if (!ep) {
> >                 usb_free_urb(urb);
> >                 return NULL;
> >         }
> >
> > Neither of these patches is needed.
> >
> > Alan Stern
> 
> Thanks, I got you.

In fact, Coverity wasn't completely wrong; there is a possible bug here.  
However the suggested fix is not the right approach.

The usb_maxpacket() routine does a two-step computation.  First, it 
looks up the endpoint number in the pipe to get a usb_host_endpoint 
pointer, and then it uses the pointer to get the maxpacket value.  
Coverity complained that the lookup in the first step can fail, and that 
is in fact true: If there is an interface or configuration change before 
usb_maxpacket() is called, the endpoint number table can change and the 
lookup may fail.

But it turns out the first step isn't needed here at all, since the 
endpoint pointer is already stored in the URB (by the code in 
usb_submit_urb() that I pointed out earlier).  So an appropriate way to 
fix the problem is to carry out just the second step:

-	maxpacket = usb_maxpacket(urb->dev, urb->pipe);
+	maxpacket = usb_endpoint_maxp(&urb->ep->desc);

This holds for both of your patches.

Alan Stern

Re: [PATCH 1/2] usb: ehci: Prevent possible modulo by zero

[Khalid Masum]

> The usb_maxpacket() routine does a two-step computation.  First, it
> looks up the endpoint number in the pipe to get a usb_host_endpoint
> pointer, and then it uses the pointer to get the maxpacket value.
> Coverity complained that the lookup in the first step can fail, and that
> is in fact true: If there is an interface or configuration change before
> usb_maxpacket() is called, the endpoint number table can change and the
> lookup may fail.
>
> But it turns out the first step isn't needed here at all, since the
> endpoint pointer is already stored in the URB (by the code in

That makes sense. Thanks for explaining.
> usb_submit_urb() that I pointed out earlier).  So an appropriate way to
> fix the problem is to carry out just the second step:
>
> -       maxpacket = usb_maxpacket(urb->dev, urb->pipe);
> +       maxpacket = usb_endpoint_maxp(&urb->ep->desc);
>
> This holds for both of your patches.

Got you.
>
> Alan Stern

-- Khalid Masum