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

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

Dan,

On 1/13/21 3:13 AM, Dan Carpenter wrote:
> On Tue, Jan 12, 2021 at 05:44:52PM -0600, Ricardo Rivera-Matos wrote:
>> 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.
>>
> This looks good.  This is a patch you are proposing or it's already
> merged in an upstream tree somewhere?  Either way, that sounds fine.
> Thank!
I am proposing it. I will CC you when I send off the patchset.
>
> regards,
> dan carpenter
>
Best Regards,
Ricardo