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:   Wed, 24 Aug 2022 07:56:04 +0200
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Khalid Masum <khalid.masum.92@gmail.com>
Cc:     Alan Stern <stern@rowland.harvard.edu>, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] usb: ehci: Prevent possible modulo by zero
Message-ID: <YwW9dBW/0TKHPnC1@kroah.com>
References: <20220823182758.13401-1-khalid.masum.92@gmail.com>
 <20220823182758.13401-3-khalid.masum.92@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220823182758.13401-3-khalid.masum.92@gmail.com>
Precedence: bulk

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