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]

Message-ID: <7c5bbc97-b9dc-96bb-5764-58bebec0178d@i-love.sakura.ne.jp>
Date:   Sun, 19 Dec 2021 10:41:02 +0900
From:   Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
To:     Alan Stern <stern@...land.harvard.edu>,
        Julio Faracco <jcfaracco@...il.com>
Cc:     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 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 ?