linux-kernel - re: hwmon: (pmbus) Add support for reading direct mode coefficients

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <68276e1d-f262-d379-4600-88abdbecddd8@canonical.com>
Date:   Thu, 10 Jun 2021 17:55:40 +0100
From:   Colin Ian King <colin.king@...onical.com>
To:     Guenter Roeck <linux@...ck-us.net>,
        Jean Delvare <jdelvare@...e.com>, linux-hwmon@...r.kernel.org,
        Erik Rosen <erik.rosen@...ormote.com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: re: hwmon: (pmbus) Add support for reading direct mode coefficients

Hi,

Static analysis with Coverity on linux-next has detected a potential
issue in drivers/hwmon/pmbus/pmbus_core.c with the following commit:

commit 999d577d7c007d38ab83eee4532d107c2233f78f
Author: Erik Rosen <erik.rosen@...ormote.com>
Date:   Wed Jun 9 11:32:06 2021 +0200

    hwmon: (pmbus) Add support for reading direct mode coefficients

The analysis is as follows:

2219 static int pmbus_init_coefficients(struct i2c_client *client,
2220                                   struct pmbus_driver_info *info)
2221 {

    1. var_decl: Declaring variable ret without initializer.

2222        int i, n, ret;
2223        const struct pmbus_class_attr_map *map;
2224        const struct pmbus_sensor_attr *attr;
2225

    2. Condition i < 6UL /* sizeof (class_attr_map) / sizeof
(class_attr_map[0]) + (int)sizeof (struct
pmbus_init_coefficients::[unnamed type]) */, taking true branch.

    5. Condition i < 6UL /* sizeof (class_attr_map) / sizeof
(class_attr_map[0]) + (int)sizeof (struct
pmbus_init_coefficients::[unnamed type]) */, taking true branch.

    8. Condition i < 6UL /* sizeof (class_attr_map) / sizeof
(class_attr_map[0]) + (int)sizeof (struct
pmbus_init_coefficients::[unnamed type]) */, taking true branch.

2226        for (i = 0; i < ARRAY_SIZE(class_attr_map); i++) {
2227                map = &class_attr_map[i];

    3. Condition info->format[map->class] != direct, taking true branch.
    6. Condition info->format[map->class] != direct, taking true branch.
    9. Condition info->format[map->class] != direct, taking false branch.

2228                if (info->format[map->class] != direct)

    4. Continuing loop.
    7. Continuing loop.

2229                        continue;

    10. Condition n < map->nattr, taking true branch.
    13. Condition n < map->nattr, taking true branch.
    16. Condition n < map->nattr, taking false branch.

2230                for (n = 0; n < map->nattr; n++) {
2231                        attr = &map->attr[n];

    11. Condition map->class != attr->class, taking true branch.
    14. Condition map->class != attr->class, taking true branch.
2232                        if (map->class != attr->class)
    12. Continuing loop.
    15. Continuing loop.

2233                                continue;
2234                        ret = pmbus_read_coefficients(client, info,
attr);
2235                        if (ret >= 0)
2236                                break;
2237                }

Uninitialized scalar variable (UNINIT)
    17. uninit_use: Using uninitialized value ret.

2238                if (ret < 0) {
2239                        dev_err(&client->dev,
2240                                "No coefficients found for sensor
class %d\n",
2241                                map->class);
2242                        return -EINVAL;
2243                }
2244        }
2245
2246        return 0;
2247 }

With the continue statements on line 2233 (or if map->nattr is zero) it
may be possible that ret is never assigned a value and so the check on
line 2238 could be checking an uninitialized variable ret. I'm not sure
if this is a false positive, but it may be worth initializing ret to
some sane value to catch these corner cases.

Colin