Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 23 Aug 2022 16:21:18 -0400
From:   Alan Stern <stern@rowland.harvard.edu>
To:     Khalid Masum <khalid.masum.92@gmail.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] usb: ehci: Prevent possible modulo by zero
Message-ID: <YwU2vp0FSR5dS/FX@rowland.harvard.edu>
References: <20220823182758.13401-1-khalid.masum.92@gmail.com>
 <20220823182758.13401-2-khalid.masum.92@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220823182758.13401-2-khalid.masum.92@gmail.com>
Precedence: bulk

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