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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YwW9dBW/0TKHPnC1@kroah.com>
Date:   Wed, 24 Aug 2022 07:56:04 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Khalid Masum <khalid.masum.92@...il.com>
Cc:     Alan Stern <stern@...land.harvard.edu>, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] usb: ehci: Prevent possible modulo by zero

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@...il.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