Coverity: pca953x_gpio_get_multiple(): Uninitialized variables

Coverity: pca953x_gpio_get_multiple(): Uninitialized variables

[coverity-bot]

Hello!

This is an experimental semi-automated report about issues detected by
Coverity from a scan of next-20200417 as part of the linux-next scan project:
https://scan.coverity.com/projects/linux-next-weekly-scan

You're getting this email because you were associated with the identified
lines of code (noted below) that were touched by commits:

  Tue Apr 14 11:28:42 2020 -0400
    96d7c7b3e654 ("gpio: gpio-pca953x, Add get_multiple function")

Coverity reported the following:

*** CID 1492652:  Uninitialized variables  (UNINIT)
/drivers/gpio/gpio-pca953x.c: 499 in pca953x_gpio_get_multiple()
493     			if (ret < 0)
494     				return ret;
495     		}
496     		/* reg_val is relative to the last read byte,
497     		 * so only shift the relative bits
498     		 */
vvv     CID 1492652:  Uninitialized variables  (UNINIT)
vvv     Using uninitialized value "reg_val".
499     		value = (reg_val >> (i % 8)) & 0x01;
500     		__assign_bit(i, bits, value);
501     	}
502     	return ret;
503     }
504

If this is a false positive, please let us know so we can mark it as
such, or teach the Coverity rules to be smarter. If not, please make
sure fixes get into linux-next. :) For patches fixing this, please
include these lines (but double-check the "Fixes" first):

Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
Addresses-Coverity-ID: 1492652 ("Uninitialized variables")
Fixes: 96d7c7b3e654 ("gpio: gpio-pca953x, Add get_multiple function")

Thanks for your attention!

-- 
Coverity-bot

Re: Coverity: pca953x_gpio_get_multiple(): Uninitialized variables

[Paul Thomas]

On Fri, Apr 17, 2020 at 5:58 PM coverity-bot <keescook@chromium.org> wrote:
>
> Hello!
>
> This is an experimental semi-automated report about issues detected by
> Coverity from a scan of next-20200417 as part of the linux-next scan project:
> https://scan.coverity.com/projects/linux-next-weekly-scan
>
> You're getting this email because you were associated with the identified
> lines of code (noted below) that were touched by commits:
>
>   Tue Apr 14 11:28:42 2020 -0400
>     96d7c7b3e654 ("gpio: gpio-pca953x, Add get_multiple function")
>
> Coverity reported the following:
>
> *** CID 1492652:  Uninitialized variables  (UNINIT)
> /drivers/gpio/gpio-pca953x.c: 499 in pca953x_gpio_get_multiple()
> 493                             if (ret < 0)
> 494                                     return ret;
> 495                     }
> 496                     /* reg_val is relative to the last read byte,
> 497                      * so only shift the relative bits
> 498                      */
> vvv     CID 1492652:  Uninitialized variables  (UNINIT)
> vvv     Using uninitialized value "reg_val".
> 499                     value = (reg_val >> (i % 8)) & 0x01;
> 500                     __assign_bit(i, bits, value);
> 501             }
> 502             return ret;
> 503     }
> 504
Well for this case it is forced on the first pass with
       offset = gc->ngpio;
so 'i' in the for_each_set_bit() loop will always be at lest 1 less
than gc->ngpio.

However, I could see how this is a little are hard for a detection
tool to follow through:
offset = gc->ngpio;
       for_each_set_bit(i, mask, gc->ngpio) {
               if ((offset >> BANK_SFT) != (i >> BANK_SFT)) {

These tools are very cool, and I'd like fix the detection one way or
another. Any suggestions on a better syntax?

thanks,
Paul

Re: Coverity: pca953x_gpio_get_multiple(): Uninitialized variables

[Kees Cook]

On Fri, Apr 17, 2020 at 06:15:05PM -0400, Paul Thomas wrote:
> On Fri, Apr 17, 2020 at 5:58 PM coverity-bot <keescook@chromium.org> wrote:
> >
> > Hello!
> >
> > This is an experimental semi-automated report about issues detected by
> > Coverity from a scan of next-20200417 as part of the linux-next scan project:
> > https://scan.coverity.com/projects/linux-next-weekly-scan
> >
> > You're getting this email because you were associated with the identified
> > lines of code (noted below) that were touched by commits:
> >
> >   Tue Apr 14 11:28:42 2020 -0400
> >     96d7c7b3e654 ("gpio: gpio-pca953x, Add get_multiple function")
> >
> > Coverity reported the following:
> >
> > *** CID 1492652:  Uninitialized variables  (UNINIT)
> > /drivers/gpio/gpio-pca953x.c: 499 in pca953x_gpio_get_multiple()
> > 493                             if (ret < 0)
> > 494                                     return ret;
> > 495                     }
> > 496                     /* reg_val is relative to the last read byte,
> > 497                      * so only shift the relative bits
> > 498                      */
> > vvv     CID 1492652:  Uninitialized variables  (UNINIT)
> > vvv     Using uninitialized value "reg_val".
> > 499                     value = (reg_val >> (i % 8)) & 0x01;
> > 500                     __assign_bit(i, bits, value);
> > 501             }
> > 502             return ret;
> > 503     }
> > 504
> Well for this case it is forced on the first pass with
>        offset = gc->ngpio;
> so 'i' in the for_each_set_bit() loop will always be at lest 1 less
> than gc->ngpio.
> 
> However, I could see how this is a little are hard for a detection
> tool to follow through:
> offset = gc->ngpio;
>        for_each_set_bit(i, mask, gc->ngpio) {
>                if ((offset >> BANK_SFT) != (i >> BANK_SFT)) {

Ah yeah, it can't see through the bounds of the "if" and offset and
the shifts.

> These tools are very cool, and I'd like fix the detection one way or
> another. Any suggestions on a better syntax?

Well... I don't think it's going to improve its checking of that loop. I
can just mark it false-positive and ignore it. :) (Or you can init
reg_val to zero at the top. *shrug*)

Thanks for looking at it!

-Kees

-- 
Kees Cook

Re: Coverity: pca953x_gpio_get_multiple(): Uninitialized variables

[Paul Thomas]

On Fri, Apr 17, 2020 at 6:45 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Fri, Apr 17, 2020 at 06:15:05PM -0400, Paul Thomas wrote:
> > On Fri, Apr 17, 2020 at 5:58 PM coverity-bot <keescook@chromium.org> wrote:
> > >
> > > Hello!
> > >
> > > This is an experimental semi-automated report about issues detected by
> > > Coverity from a scan of next-20200417 as part of the linux-next scan project:
> > > https://scan.coverity.com/projects/linux-next-weekly-scan
> > >
> > > You're getting this email because you were associated with the identified
> > > lines of code (noted below) that were touched by commits:
> > >
> > >   Tue Apr 14 11:28:42 2020 -0400
> > >     96d7c7b3e654 ("gpio: gpio-pca953x, Add get_multiple function")
> > >
> > > Coverity reported the following:
> > >
> > > *** CID 1492652:  Uninitialized variables  (UNINIT)
> > > /drivers/gpio/gpio-pca953x.c: 499 in pca953x_gpio_get_multiple()
> > > 493                             if (ret < 0)
> > > 494                                     return ret;
> > > 495                     }
> > > 496                     /* reg_val is relative to the last read byte,
> > > 497                      * so only shift the relative bits
> > > 498                      */
> > > vvv     CID 1492652:  Uninitialized variables  (UNINIT)
> > > vvv     Using uninitialized value "reg_val".
> > > 499                     value = (reg_val >> (i % 8)) & 0x01;
> > > 500                     __assign_bit(i, bits, value);
> > > 501             }
> > > 502             return ret;
> > > 503     }
> > > 504
> > Well for this case it is forced on the first pass with
> >        offset = gc->ngpio;
> > so 'i' in the for_each_set_bit() loop will always be at lest 1 less
> > than gc->ngpio.
> >
> > However, I could see how this is a little are hard for a detection
> > tool to follow through:
> > offset = gc->ngpio;
> >        for_each_set_bit(i, mask, gc->ngpio) {
> >                if ((offset >> BANK_SFT) != (i >> BANK_SFT)) {
>
> Ah yeah, it can't see through the bounds of the "if" and offset and
> the shifts.
>
> > These tools are very cool, and I'd like fix the detection one way or
> > another. Any suggestions on a better syntax?
>
> Well... I don't think it's going to improve its checking of that loop. I
> can just mark it false-positive and ignore it. :) (Or you can init
> reg_val to zero at the top. *shrug*)
Yeah, init to zero sounds good to me, one instruction is nothing
compared to what this saves by not doing the same i2c transaction
multiple times.

Bart do you want the original patch updated or something else?

Nice work on Coverity Kees!

thanks,
Paul

>
> Thanks for looking at it!
>
> -Kees
>
> --
> Kees Cook