Coverity: pca953x_gpio_get_multiple(): Uninitialized variables

DISCUSSION coverity-bot 2 participants 4 messages Apr 17, 2020 Linux-next tree

Share & tools

coverity-bot

author

2020-04-17 21:58 UTC #0 lore raw

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 <[email protected]>

Addresses-Coverity-ID: 1492652 ("Uninitialized variables")

Fixes 96d7c7b3e654 ("gpio: gpio-pca953x, Add get_multiple function")

Thanks for your attention!

Signature · 1 line

Coverity-bot

Paul Thomas

2020-04-17 22:15 UTC #1 lore raw

Quoted 15 lines · 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:

Quoted 16 lines

*** 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

Kees Cook

author

2020-04-17 22:45 UTC #2 lore raw

Quoted 34 lines · Paul Thomas

Quoted 15 lines · 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:
Quoted 16 lines
*** 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;

Quoted 8 lines

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.

Quoted 2 lines

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

Signature · 1 line

Kees Cook

Paul Thomas

2020-04-17 23:18 UTC #3 lore raw

Quoted 48 lines · Kees Cook

Quoted 34 lines · Paul Thomas
Quoted 15 lines · 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:
Quoted 16 lines
*** 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;
Quoted 8 lines
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.

Quoted 6 lines

Quoted 2 lines
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

Quoted 7 lines

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