re: power: supply: bq25980: Add support for the BQ259xx family

re: power: supply: bq25980: Add support for the BQ259xx family

[Colin Ian King]

Hi

Static analysis with Coverity has detected a potential out-of-bounds
read issue in the following commit:

commit 5069185fc18e810715a91d80fcd075e03add600c
Author: Dan Murphy <dmurphy@ti.com>
Date:   Mon Aug 31 11:48:49 2020 -0500

    power: supply: bq25980: Add support for the BQ259xx family


Analysis is as follows:

1099 static int bq25980_hw_init(struct bq25980_device *bq)
1100 {
1101        struct power_supply_battery_info bat_info = { };
1102        int wd_reg_val;
1103        int ret = 0;
1104        int curr_val;
1105        int volt_val;
1106        int i;
1107

    1. Condition !bq->watchdog_timer, taking false branch.
1108        if (!bq->watchdog_timer) {
1109                ret = regmap_update_bits(bq->regmap,
BQ25980_CHRGR_CTRL_3,
1110                                         BQ25980_WATCHDOG_DIS,
1111                                         BQ25980_WATCHDOG_DIS);
1112        } else {

    2. Condition i < 4, taking true branch.
    6. Condition i < 4, taking true branch.
    7. cond_at_most: Checking i < 4 implies that i may be up to 3 on the
true branch.
1113                for (i = 0; i < BQ25980_NUM_WD_VAL; i++) {

    3. Condition bq->watchdog_timer > bq25980_watchdog_time[i], taking
true branch.
    4. Condition bq->watchdog_timer < bq25980_watchdog_time[i + 1],
taking false branch.
    8. Condition bq->watchdog_timer > bq25980_watchdog_time[i], taking
true branch.

Out-of-bounds read (OVERRUN)
    9. overrun-local: Overrunning array bq25980_watchdog_time of 4
4-byte elements at element index 4 (byte offset 19) using index i + 1
(which evaluates to 4).

1114                        if (bq->watchdog_timer >
bq25980_watchdog_time[i] &&
1115                            bq->watchdog_timer <
bq25980_watchdog_time[i + 1]) {
1116                                wd_reg_val = i;
1117                                break;
1118                        }
    5. Jumping back to the beginning of the loop.
1119                }

Accessing bq25980_watchdog_time[i + 1] when i is 3 causes the
out-of-range read

Colin

Re: power: supply: bq25980: Add support for the BQ259xx family

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 951 bytes --]

Hi Dan,

Can you please look into this:

On Tue, Oct 06, 2020 at 06:59:22PM +0100, Colin Ian King wrote:
> Hi
> 
> Static analysis with Coverity has detected a potential out-of-bounds
> read issue in the following commit:
> 
> commit 5069185fc18e810715a91d80fcd075e03add600c
> Author: Dan Murphy <dmurphy@ti.com>
> Date:   Mon Aug 31 11:48:49 2020 -0500
>
> [...]

TLDR: In the following code bq25980_watchdog_time[i + 1] might
access invalid bq25980_watchdog_time[4] when i reaches 3:

#define BQ25980_NUM_WD_VAL 4
static int bq25980_watchdog_time[BQ25980_NUM_WD_VAL] = {5000, 10000, 50000,
                                                        300000};

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

Thanks,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: power: supply: bq25980: Add support for the BQ259xx family

[Dan Murphy]

Sebastian

On 10/8/20 5:04 PM, Sebastian Reichel wrote:
> Hi Dan,
>
> Can you please look into this:

I have a patch for the over run and this patch should also fix the 
sparse warning just need to finish testing it I should be posting it today

Dan