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@xxxxxxxxx>

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@xxxxxxxxx>

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