* [bug report] power: supply: bq256xx: Introduce the BQ256XX charger driver
@ 2021-01-12 8:54 Dan Carpenter
2021-01-12 23:44 ` [EXTERNAL] " Ricardo Rivera-Matos
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2021-01-12 8:54 UTC (permalink / raw)
To: r-rivera-matos; +Cc: linux-pm
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.
On the last iteration through the loop this will read beyond the end of
the array possibly setting "wd_reg_val = 7" uninitentionally.
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [EXTERNAL] [bug report] power: supply: bq256xx: Introduce the BQ256XX charger driver
2021-01-12 8:54 [bug report] power: supply: bq256xx: Introduce the BQ256XX charger driver Dan Carpenter
@ 2021-01-12 23:44 ` Ricardo Rivera-Matos
2021-01-13 9:13 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Ricardo Rivera-Matos @ 2021-01-12 23:44 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-pm
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [EXTERNAL] [bug report] power: supply: bq256xx: Introduce the BQ256XX charger driver
2021-01-12 23:44 ` [EXTERNAL] " Ricardo Rivera-Matos
@ 2021-01-13 9:13 ` Dan Carpenter
2021-01-13 22:51 ` Ricardo Rivera-Matos
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2021-01-13 9:13 UTC (permalink / raw)
To: Ricardo Rivera-Matos; +Cc: linux-pm
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!
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [EXTERNAL] [bug report] power: supply: bq256xx: Introduce the BQ256XX charger driver
2021-01-13 9:13 ` Dan Carpenter
@ 2021-01-13 22:51 ` Ricardo Rivera-Matos
0 siblings, 0 replies; 4+ messages in thread
From: Ricardo Rivera-Matos @ 2021-01-13 22:51 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-pm@vger.kernel.org
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-01-14 1:56 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-01-12 8:54 [bug report] power: supply: bq256xx: Introduce the BQ256XX charger driver Dan Carpenter
2021-01-12 23:44 ` [EXTERNAL] " Ricardo Rivera-Matos
2021-01-13 9:13 ` Dan Carpenter
2021-01-13 22:51 ` Ricardo Rivera-Matos
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox