| Message ID | 20220823182758.13401-3-khalid.masum.92@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: ehci: Prevent possible modulo by zero | expand |
On Wed, Aug 24, 2022 at 12:27:58AM +0600, Khalid Masum wrote: > usb_maxpacket() returns 0 if it fails to fetch the endpoint. This > value is later used for calculating modulo. Which can cause modulo > by zero in qtd_fill. > > Prevent this breakage by returning if maxpacket is found to be 0. > > Fixes coverity warning: 1487371 ("Division or modulo by zero") Odd tag format, is that in the documentation? > Fixes: 9841f37a1cca ("usb: ehci: Add support for SINGLE_STEP_SET_FEATURE test of EHSET") > Signed-off-by: Khalid Masum <khalid.masum.92@gmail.com> > --- > drivers/usb/host/ehci-q.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c > index eb31d13e9ecd..cf2585e9a09f 100644 > --- a/drivers/usb/host/ehci-q.c > +++ b/drivers/usb/host/ehci-q.c > @@ -1221,6 +1221,8 @@ static int ehci_submit_single_step_set_feature( > token |= (1 /* "in" */ << 8); /*This is IN stage*/ > > maxpacket = usb_maxpacket(urb->dev, urb->pipe); > + if (unlikely(!maxpacket)) You only ever use likely/unlikely if you can document how it matters with a benchmark or other way to notice the difference. Otherwise let the compiler and the CPU do their magic, they know how to do this better than us. > + return -1; A real error number should be returned here if this was valid. But as Alan said, coverity is often wrong, and unless you can prove otherwise, this patch isn't valid. thanks, greg k-h
On Wed, Aug 24, 2022 at 11:56 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > Odd tag format, is that in the documentation? You are right. I should have used "Addresses-coverity". > You only ever use likely/unlikely if you can document how it matters > with a benchmark or other way to notice the difference. Otherwise let > the compiler and the CPU do their magic, they know how to do this better > than us. Thanks for the important information. > > > + return -1; I noticed. The function returns -1 on failure, everywhere so I used that. I guess making them return correct error numbers using macros would be a patch. > > A real error number should be returned here if this was valid. > > But as Alan said, coverity is often wrong, and unless you can prove > otherwise, this patch isn't valid. Got you. > > thanks, > > greg k-h -- Khalid Masum
diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c index eb31d13e9ecd..cf2585e9a09f 100644 --- a/drivers/usb/host/ehci-q.c +++ b/drivers/usb/host/ehci-q.c @@ -1221,6 +1221,8 @@ static int ehci_submit_single_step_set_feature( token |= (1 /* "in" */ << 8); /*This is IN stage*/ maxpacket = usb_maxpacket(urb->dev, urb->pipe); + if (unlikely(!maxpacket)) + return -1; qtd_fill(ehci, qtd, buf, len, token, maxpacket);
usb_maxpacket() returns 0 if it fails to fetch the endpoint. This value is later used for calculating modulo. Which can cause modulo by zero in qtd_fill. Prevent this breakage by returning if maxpacket is found to be 0. Fixes coverity warning: 1487371 ("Division or modulo by zero") Fixes: 9841f37a1cca ("usb: ehci: Add support for SINGLE_STEP_SET_FEATURE test of EHSET") Signed-off-by: Khalid Masum <khalid.masum.92@gmail.com> --- drivers/usb/host/ehci-q.c | 2 ++ 1 file changed, 2 insertions(+)