Re: [EXTERNAL] [bug report] power: supply: bq256xx: Introduce the BQ256XX charger driver

[Ricardo Rivera-Matos <r-rivera-matos@ti.com>]

Dan,

On 1/12/21 2:54 AM, Dan Carpenter wrote:
> Hello Ricardo Rivera-Matos,
>
> The patch 32e4978bb920: "power: supply: bq256xx: Introduce the
> BQ256XX charger driver" from Jan 6, 2021, leads to the following
> static checker warning:
>
> 	drivers/power/supply/bq256xx_charger.c:1512 bq256xx_hw_init()
> 	error: buffer overflow 'bq256xx_watchdog_time' 8 <= 8
>
> drivers/power/supply/bq256xx_charger.c
>    1503  static int bq256xx_hw_init(struct bq256xx_device *bq)
>    1504  {
>    1505          struct power_supply_battery_info bat_info = { };
>    1506          int wd_reg_val = BQ256XX_WATCHDOG_DIS;
>    1507          int ret = 0;
>    1508          int i;
>    1509
>    1510          for (i = 0; i < BQ256XX_NUM_WD_VAL; i++) {
>    1511                  if (bq->watchdog_timer > bq256xx_watchdog_time[i] &&
>    1512                      bq->watchdog_timer < bq256xx_watchdog_time[i + 1])
>                                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> The last four members of this array are all zero.
ACK, BQ256XX_NUM_WD_VAL should actually be 4 instead of 8
>
> On the last iteration through the loop this will read beyond the end of
> the array possibly setting "wd_reg_val = 7" uninitentionally.
ACK, bq256xx_parse_dt() will clamp bq->watchdog_timer so then in 
bq256xx_hw_init()

     for (i = 0; i < BQ256XX_NUM_WD_VAL; i++) {
         if (bq->watchdog_timer == bq256xx_watchdog_time[i]) {
             wd_reg_val = i;
             break;
         }
         if (bq->watchdog_timer > bq256xx_watchdog_time[i] &&
             bq->watchdog_timer < bq256xx_watchdog_time[i + 1])
             wd_reg_val = i;
     }

The first if will catch the exact matches and the second if will catch 
the "in-betweens" and round down. The final iteration will always fall 
into the first if statement and break.

>
>    1513                          wd_reg_val = i;
>    1514          }
>    1515          ret = regmap_update_bits(bq->regmap, BQ256XX_CHARGER_CONTROL_1,
>    1516                                   BQ256XX_WATCHDOG_MASK, wd_reg_val <<
>    1517                                                  BQ256XX_WDT_BIT_SHIFT);
>
> regards,
> dan carpenter
Ricardo