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
