linux-kernel - Re: [PATCH] usb: fixing some clang warnings inside usb host drivers

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:   Sat, 18 Dec 2021 21:50:22 -0500
From:   Alan Stern <stern@...land.harvard.edu>
To:     Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc:     Julio Faracco <jcfaracco@...il.com>, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org, gregkh@...uxfoundation.org,
        axboe@...nel.dk, tglx@...utronix.de, damien.lemoal@....com,
        dkadashev@...il.com, paul.gortmaker@...driver.com,
        zhouyanjie@...yeetech.com, niklas.cassel@....com,
        macro@...am.me.uk, caihuoqing@...du.com
Subject: Re: [PATCH] usb: fixing some clang warnings inside usb host drivers

On Sun, Dec 19, 2021 at 10:41:02AM +0900, Tetsuo Handa wrote:
> On 2021/12/19 3:05, Alan Stern wrote:
> >> diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
> >> index 2cbf4f85bff3..98cb44414e78 100644
> >> --- a/drivers/usb/host/ehci-q.c
> >> +++ b/drivers/usb/host/ehci-q.c
> >> @@ -64,7 +64,7 @@ qtd_fill(struct ehci_hcd *ehci, struct ehci_qtd *qtd, dma_addr_t buf,
> >>  		}
> >>  
> >>  		/* short packets may only terminate transfers */
> >> -		if (count != len)
> >> +		if (count != len && maxpacket > 0)
> >>  			count -= (count % maxpacket);
> > 
> > This is different.  But again, I do not think the extra check should be 
> > added.  If maxpacket is 0, we _want_ the code to fail in a highly 
> > visible manner -- it would mean there is a bug somewhere else in the 
> > kernel.
> 
> Some of the callers are passing the return value from usb_maxpacket(), and
> usb_maxpacket() can return 0. But division by 0 bug here becomes visible
> only when len < count in
> 
> 	count = 0x1000 - (buf & 0x0fff);	/* rest of that page */
> 	if (likely (len < count))		/* ... iff needed */
> 		count = len;
> 
> is false and count != len in
> 
> 		if (count != len)
> 			count -= (count % maxpacket);
> 
> is true, which may be quite difficult to trigger.
> 
> Maybe we should make sure that maxpacket > 0 on the caller side, for e.g.
> 
> 	/* qh makes control packets use qtd toggle; maybe switch it */
> 	if ((maxpacket & (this_qtd_len + (maxpacket - 1))) == 0)
> 		token ^= QTD_TOGGLE;
> 
> and
> 
> 	if (usb_pipecontrol (urb->pipe)) {
> 		one_more = 1;
> 		token ^= 0x0100;	/* "in" <--> "out"  */
> 		token |= QTD_TOGGLE;	/* force DATA1 */
> 	} else if (usb_pipeout(urb->pipe)
> 			&& (urb->transfer_flags & URB_ZERO_PACKET)
> 			&& !(urb->transfer_buffer_length % maxpacket)) {
> 		one_more = 1;
> 	}
> 
> are expecting that maxpacket > 0 ?

You should read this code in usb_submit_urb():

	max = usb_endpoint_maxp(&ep->desc);
	if (max <= 0) {
		dev_dbg(&dev->dev,
			"bogus endpoint ep%d%s in %s (bad maxpacket %d)\n",
			usb_endpoint_num(&ep->desc), is_out ? "out" : "in",
			__func__, max);
		return -EMSGSIZE;
	}

As far as I know, every code path leading to qtd_fill() has to pass this 
test.

Alan Stern