[next] USB: EHCI: ehci-mv: fix less than zero comparison of an unsigned int

Commit Message

Colin King May 15, 2020, 4:54 p.m. UTC

From: Colin Ian King <[e-mail]>

The comparison of hcd->irq to less than zero for an error check will
never be true because hcd->irq is an unsigned int.  Fix this by
assigning the int retval to the return of platform_get_irq and checking
this for the -ve error condition and assigning hcd->irq to retval.

Addresses-Coverity: ("Unsigned compared against 0")
Fixes: c856b4b0fdb5 ("USB: EHCI: ehci-mv: fix error handling in mv_ehci_probe()")
Signed-off-by: Colin Ian King <[e-mail]>
---
 drivers/usb/host/ehci-mv.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Alan Stern May 15, 2020, 5:21 p.m. UTC | #1

On Fri, May 15, 2020 at 05:54:53PM +0100, Colin King wrote:
> From: Colin Ian King <[e-mail]>
> 
> The comparison of hcd->irq to less than zero for an error check will
> never be true because hcd->irq is an unsigned int.  Fix this by
> assigning the int retval to the return of platform_get_irq and checking
> this for the -ve error condition and assigning hcd->irq to retval.
> 
> Addresses-Coverity: ("Unsigned compared against 0")
> Fixes: c856b4b0fdb5 ("USB: EHCI: ehci-mv: fix error handling in mv_ehci_probe()")
> Signed-off-by: Colin Ian King <[e-mail]>
> ---

Thanks to Coverity for spotting this.  Any reason why it didn't spot 
exactly the same mistake in the ohci-da8xx.c driver?

Also, why wasn't the patch CC'ed for the stable series?

Alan Stern

Colin King May 15, 2020, 5:26 p.m. UTC | #2

On 15/05/2020 18:21, Alan Stern wrote:
> On Fri, May 15, 2020 at 05:54:53PM +0100, Colin King wrote:
>> From: Colin Ian King <[e-mail]>
>>
>> The comparison of hcd->irq to less than zero for an error check will
>> never be true because hcd->irq is an unsigned int.  Fix this by
>> assigning the int retval to the return of platform_get_irq and checking
>> this for the -ve error condition and assigning hcd->irq to retval.
>>
>> Addresses-Coverity: ("Unsigned compared against 0")
>> Fixes: c856b4b0fdb5 ("USB: EHCI: ehci-mv: fix error handling in mv_ehci_probe()")
>> Signed-off-by: Colin Ian King <[e-mail]>
>> ---
> 
> Thanks to Coverity for spotting this.  Any reason why it didn't spot 
> exactly the same mistake in the ohci-da8xx.c driver?

No idea, it is curious that it can spot one error but miss another.
Sometimes I see these issues on the next scan, so it maybe the database
diff'ing is awry.

> 
> Also, why wasn't the patch CC'ed for the stable series?

My bad on that. Human error

> 
> Alan Stern
>

Alan Stern May 15, 2020, 6:43 p.m. UTC | #3

On Fri, May 15, 2020 at 06:26:04PM +0100, Colin Ian King wrote:
> On 15/05/2020 18:21, Alan Stern wrote:
> > On Fri, May 15, 2020 at 05:54:53PM +0100, Colin King wrote:
> >> From: Colin Ian King <[e-mail]>
> >>
> >> The comparison of hcd->irq to less than zero for an error check will
> >> never be true because hcd->irq is an unsigned int.  Fix this by
> >> assigning the int retval to the return of platform_get_irq and checking
> >> this for the -ve error condition and assigning hcd->irq to retval.
> >>
> >> Addresses-Coverity: ("Unsigned compared against 0")
> >> Fixes: c856b4b0fdb5 ("USB: EHCI: ehci-mv: fix error handling in mv_ehci_probe()")
> >> Signed-off-by: Colin Ian King <[e-mail]>
> >> ---
> > 
> > Thanks to Coverity for spotting this.  Any reason why it didn't spot 
> > exactly the same mistake in the ohci-da8xx.c driver?
> 
> No idea, it is curious that it can spot one error but miss another.
> Sometimes I see these issues on the next scan, so it maybe the database
> diff'ing is awry.
> 
> > 
> > Also, why wasn't the patch CC'ed for the stable series?
> 
> My bad on that. Human error

Actually the question itself was my mistake.  I didn't notice that your 
patch was a fix to something that was just merged and hadn't been CC'ed 
to stable.

Alan Stern

Tang Bin May 15, 2020, 11:23 p.m. UTC | #4

Hi Alan & Colin：

On 2020/5/16 1:21, Alan Stern wrote:
> On Fri, May 15, 2020 at 05:54:53PM +0100, Colin King wrote:
>> From: Colin Ian King <[e-mail]>
>>
>> The comparison of hcd->irq to less than zero for an error check will
>> never be true because hcd->irq is an unsigned int.  Fix this by
>> assigning the int retval to the return of platform_get_irq and checking
>> this for the -ve error condition and assigning hcd->irq to retval.
>>
>> Addresses-Coverity: ("Unsigned compared against 0")
>> Fixes: c856b4b0fdb5 ("USB: EHCI: ehci-mv: fix error handling in mv_ehci_probe()")
>> Signed-off-by: Colin Ian King <[e-mail]>
>> ---
> Thanks to Coverity for spotting this.  Any reason why it didn't spot
> exactly the same mistake in the ohci-da8xx.c driver?

I just looked at the code and wondered whether it would be more 
appropriate to modify the header file "hcd.h".  Since 'irq' might be an 
negative, why not just modify the variables in the 'struct usb_hcd',  
'unsigned int  irq'--> 'int irq'? After all, it's a public one.

Thanks,

Tang Bin

>
>

Alan Stern May 16, 2020, 1:17 a.m. UTC | #5

On Sat, May 16, 2020 at 07:23:42AM +0800, Tang Bin wrote:
> Hi Alan & Colin：
> 
> On 2020/5/16 1:21, Alan Stern wrote:
> > On Fri, May 15, 2020 at 05:54:53PM +0100, Colin King wrote:
> > > From: Colin Ian King <[e-mail]>
> > > 
> > > The comparison of hcd->irq to less than zero for an error check will
> > > never be true because hcd->irq is an unsigned int.  Fix this by
> > > assigning the int retval to the return of platform_get_irq and checking
> > > this for the -ve error condition and assigning hcd->irq to retval.
> > > 
> > > Addresses-Coverity: ("Unsigned compared against 0")
> > > Fixes: c856b4b0fdb5 ("USB: EHCI: ehci-mv: fix error handling in mv_ehci_probe()")
> > > Signed-off-by: Colin Ian King <[e-mail]>
> > > ---
> > Thanks to Coverity for spotting this.  Any reason why it didn't spot
> > exactly the same mistake in the ohci-da8xx.c driver?
> 
> I just looked at the code and wondered whether it would be more appropriate
> to modify the header file "hcd.h".  Since 'irq' might be an negative, why
> not just modify the variables in the 'struct usb_hcd',  'unsigned int 
> irq'--> 'int irq'? After all, it's a public one.

I think the code in the drivers should be changed.  The reason is if 
platform_get_irq() returns a negative value, then that value should be 
what the probe function returns.

Alan Stern

Greg KH May 16, 2020, 6:30 a.m. UTC | #6

On Fri, May 15, 2020 at 01:21:21PM -0400, Alan Stern wrote:
> On Fri, May 15, 2020 at 05:54:53PM +0100, Colin King wrote:
> > From: Colin Ian King <[e-mail]>
> > 
> > The comparison of hcd->irq to less than zero for an error check will
> > never be true because hcd->irq is an unsigned int.  Fix this by
> > assigning the int retval to the return of platform_get_irq and checking
> > this for the -ve error condition and assigning hcd->irq to retval.
> > 
> > Addresses-Coverity: ("Unsigned compared against 0")
> > Fixes: c856b4b0fdb5 ("USB: EHCI: ehci-mv: fix error handling in mv_ehci_probe()")
> > Signed-off-by: Colin Ian King <[e-mail]>
> > ---
> 
> Thanks to Coverity for spotting this.  Any reason why it didn't spot 
> exactly the same mistake in the ohci-da8xx.c driver?

I think Coverity only runs on a limited kernel configuration and does
not build everything :(

Colin King May 16, 2020, 9:25 a.m. UTC | #7

On 16/05/2020 07:30, Greg Kroah-Hartman wrote:
> On Fri, May 15, 2020 at 01:21:21PM -0400, Alan Stern wrote:
>> On Fri, May 15, 2020 at 05:54:53PM +0100, Colin King wrote:
>>> From: Colin Ian King <[e-mail]>
>>>
>>> The comparison of hcd->irq to less than zero for an error check will
>>> never be true because hcd->irq is an unsigned int.  Fix this by
>>> assigning the int retval to the return of platform_get_irq and checking
>>> this for the -ve error condition and assigning hcd->irq to retval.
>>>
>>> Addresses-Coverity: ("Unsigned compared against 0")
>>> Fixes: c856b4b0fdb5 ("USB: EHCI: ehci-mv: fix error handling in mv_ehci_probe()")
>>> Signed-off-by: Colin Ian King <[e-mail]>
>>> ---
>>
>> Thanks to Coverity for spotting this.  Any reason why it didn't spot 
>> exactly the same mistake in the ohci-da8xx.c driver?
> 
> I think Coverity only runs on a limited kernel configuration and does
> not build everything :(
> 
I do x86-64 make allmodconfig builds, so it does a fair amount of
coverage on the builds

Colin

Patch

diff --git a/drivers/usb/host/ehci-mv.c b/drivers/usb/host/ehci-mv.c
index 0d61f43c29dc..cffdc8d01b2a 100644
--- a/drivers/usb/host/ehci-mv.c
+++ b/drivers/usb/host/ehci-mv.c
@@ -166,11 +166,10 @@  static int mv_ehci_probe(struct platform_device *pdev)
 	hcd->rsrc_len = resource_size(r);
 	hcd->regs = ehci_mv->op_regs;
 
-	hcd->irq = platform_get_irq(pdev, 0);
-	if (hcd->irq < 0) {
-		retval = hcd->irq;
+	retval = platform_get_irq(pdev, 0);
+	if (retval < 0)
 		goto err_disable_clk;
-	}
+	hcd->irq = retval;
 
 	ehci = hcd_to_ehci(hcd);
 	ehci->caps = (struct ehci_caps __iomem *) ehci_mv->cap_regs;