linux-kernel - Re: [PATCH 1/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]

Date:   Tue, 23 Aug 2022 16:21:18 -0400
From:   Alan Stern <stern@...land.harvard.edu>
To:     Khalid Masum <khalid.masum.92@...il.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] usb: ehci: Prevent possible modulo by zero

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