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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 ?




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux