lkml.org 
[lkml]   [2021]   [Dec]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] usb: fixing some clang warnings inside usb host drivers
From
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 ?

\
 
 \ /
  Last update: 2021-12-19 02:42    [W:0.098 / U:0.676 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site