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 ?
