| Message ID | 20220823182758.13401-1-khalid.masum.92@gmail.com |
|---|---|
| Headers | show |
| Series | usb: ehci: Prevent possible modulo by zero | expand |
On Wed, Aug 24, 2022 at 2:21 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > if (!ep) { > usb_free_urb(urb); > return NULL; > } > > Neither of these patches is needed. > > Alan Stern Thanks, I got you. -- Khalid Masum
On Wed, Aug 24, 2022 at 05:15:47PM +0600, Khalid Masum wrote: > On Wed, Aug 24, 2022 at 2:21 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > if (!ep) { > > usb_free_urb(urb); > > return NULL; > > } > > > > Neither of these patches is needed. > > > > Alan Stern > > Thanks, I got you. In fact, Coverity wasn't completely wrong; there is a possible bug here. However the suggested fix is not the right approach. The usb_maxpacket() routine does a two-step computation. First, it looks up the endpoint number in the pipe to get a usb_host_endpoint pointer, and then it uses the pointer to get the maxpacket value. Coverity complained that the lookup in the first step can fail, and that is in fact true: If there is an interface or configuration change before usb_maxpacket() is called, the endpoint number table can change and the lookup may fail. But it turns out the first step isn't needed here at all, since the endpoint pointer is already stored in the URB (by the code in usb_submit_urb() that I pointed out earlier). So an appropriate way to fix the problem is to carry out just the second step: - maxpacket = usb_maxpacket(urb->dev, urb->pipe); + maxpacket = usb_endpoint_maxp(&urb->ep->desc); This holds for both of your patches. Alan Stern
> The usb_maxpacket() routine does a two-step computation. First, it > looks up the endpoint number in the pipe to get a usb_host_endpoint > pointer, and then it uses the pointer to get the maxpacket value. > Coverity complained that the lookup in the first step can fail, and that > is in fact true: If there is an interface or configuration change before > usb_maxpacket() is called, the endpoint number table can change and the > lookup may fail. > > But it turns out the first step isn't needed here at all, since the > endpoint pointer is already stored in the URB (by the code in That makes sense. Thanks for explaining. > usb_submit_urb() that I pointed out earlier). So an appropriate way to > fix the problem is to carry out just the second step: > > - maxpacket = usb_maxpacket(urb->dev, urb->pipe); > + maxpacket = usb_endpoint_maxp(&urb->ep->desc); > > This holds for both of your patches. Got you. > > Alan Stern -- Khalid Masum