Date:   Sat, 18 Dec 2021 21:50:22 -0500
From:   Alan Stern <stern@rowland.harvard.edu>
To:     Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc:     Julio Faracco <jcfaracco@gmail.com>, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org, gregkh@linuxfoundation.org,
        axboe@kernel.dk, tglx@linutronix.de, damien.lemoal@wdc.com,
        dkadashev@gmail.com, paul.gortmaker@windriver.com,
        zhouyanjie@wanyeetech.com, niklas.cassel@wdc.com,
        macro@orcam.me.uk, caihuoqing@baidu.com
Subject: Re: [PATCH] usb: fixing some clang warnings inside usb host drivers
Message-ID: <Yb6d7tflQeJ+1Et2@rowland.harvard.edu>
References: <20211218042420.28466-1-jcfaracco@gmail.com>
 <Yb4i7LyYIlJi/9fb@rowland.harvard.edu>
 <7c5bbc97-b9dc-96bb-5764-58bebec0178d@i-love.sakura.ne.jp>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <7c5bbc97-b9dc-96bb-5764-58bebec0178d@i-love.sakura.ne.jp>
Precedence: bulk

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