[scan-admin at coverity.com: New Defects reported by Coverity Scan for Das U-Boot]

Pratyush Yadav p.yadav at ti.com
Mon Nov 2 12:54:56 CET 2020


[Copy-pasting my reply to the off-list thread].

Hi,

On 30/10/20 10:45AM, Tom Rini wrote:
> Hey all,
> 
> Here's the latest report from Coverity on new issues.  Please take a
> look and let me know if any of these are false positives or things
> that we should try and adopt a Coverity model to cover.  Thanks!
> 
> ---------- Forwarded message ---------
> From: <scan-admin at coverity.com>
> Date: Wed, Oct 28, 2020 at 4:41 PM
> Subject: New Defects reported by Coverity Scan for Das U-Boot
> To: <tom.rini at gmail.com>
> 
> 
> Hi,
> 
> Please find the latest report on new defect(s) introduced to Das
> U-Boot found with Coverity Scan.
> 
> 37 new defect(s) introduced to Das U-Boot found with Coverity Scan.
> 5 defect(s), reported by Coverity Scan earlier, were marked fixed in
> the recent build analyzed by Coverity Scan.
> 
> New defect(s) Reported-by: Coverity Scan
> Showing 20 of 37 defect(s)
> 
> 
> ** CID 312960:  Integer handling issues  (BAD_SHIFT)
> /drivers/mux/mmio.c: 107 in mmio_mux_probe()
> 
> 
> ________________________________________________________________________________________________________
> *** CID 312960:  Integer handling issues  (BAD_SHIFT)
> /drivers/mux/mmio.c: 107 in mmio_mux_probe()
> 101                     mask = mux_reg_masks[2 * i + 1];
> 102
> 103                     field.reg = reg;
> 104                     field.msb = fls(mask) - 1;
> 105                     field.lsb = ffs(mask) - 1;
> 106
> >>>     CID 312960:  Integer handling issues  (BAD_SHIFT)
> >>>     In expression "0xffffffffffffffffUL << field.lsb", left shifting by more than 63 bits has undefined behavior.  The shift amount, "field.lsb", is 4294967295.
> 107                     if (mask != GENMASK(field.msb, field.lsb))
> 108                             return log_msg_ret("invalid mask", -EINVAL);

Sounds like a legitimate complaint. If the mask is 0 then fls and ffs 
will return 0, and so msb and lsb will be 0xffffffff each. This will 
result in GENMASK() doing ~0UL << 0xffffffff. Of course, a mask of 0 is 
invalid but then this condition is supposed to check for invalid masks 
so that just defeats the purpose.

This code seems to check if a mask is all 1s or not. So it will catch a 
mask like 0b11101. But it will trip up on a mask like 0. My suggestion 
is to make the check something like:

    if (ffs(mask) == 0 || mask != GENMASK(field.msb, field.lsb))

> 109
> 110                     fields[i] = devm_regmap_field_alloc(dev, regmap, field);
> 111                     if (IS_ERR(fields[i])) {
> 112                             ret = PTR_ERR(fields[i]);
> 
> ** CID 312959:    (RESOURCE_LEAK)
> /drivers/mux/mmio.c: 113 in mmio_mux_probe()
> /drivers/mux/mmio.c: 108 in mmio_mux_probe()
> 
> 
> ________________________________________________________________________________________________________
> *** CID 312959:    (RESOURCE_LEAK)
> /drivers/mux/mmio.c: 113 in mmio_mux_probe()
> 107                     if (mask != GENMASK(field.msb, field.lsb))
> 108                             return log_msg_ret("invalid mask", -EINVAL);
> 109
> 110                     fields[i] = devm_regmap_field_alloc(dev, regmap, field);
> 111                     if (IS_ERR(fields[i])) {
> 112                             ret = PTR_ERR(fields[i]);
> >>>     CID 312959:    (RESOURCE_LEAK)
> >>>     Variable "idle_states" going out of scope leaks the storage it points to.

Hmm... Not sure if this is actually a leak. idle_states is allocated 
using devm_kmalloc(), so if the probe fails the device should be 
destroyed, and idle_states with it. I'm not very well versed with 
managed APIs so maybe this is wrong. Dunno.

Anyway, idle_states is local to this function so I don't know if 
devm_kmalloc() is even needed. We might as well use regular kmalloc() 
because we free it at the end of probe anyway.

Any advice on this?

> 113                             return log_msg_ret("regmap_field_alloc", ret);
> 114                     }
> 115
> 116                     bits = 1 + field.msb - field.lsb;
> 117                     mux->states = 1 << bits;
> 118
> /drivers/mux/mmio.c: 108 in mmio_mux_probe()
> 102
> 103                     field.reg = reg;
> 104                     field.msb = fls(mask) - 1;
> 105                     field.lsb = ffs(mask) - 1;
> 106
> 107                     if (mask != GENMASK(field.msb, field.lsb))
> >>>     CID 312959:    (RESOURCE_LEAK)
> >>>     Variable "idle_states" going out of scope leaks the storage it points to.

Same as above.

> 108                             return log_msg_ret("invalid mask", -EINVAL);
> 109
> 110                     fields[i] = devm_regmap_field_alloc(dev, regmap, field);
> 111                     if (IS_ERR(fields[i])) {
> 112                             ret = PTR_ERR(fields[i]);
> 113                             return log_msg_ret("regmap_field_alloc", ret);
> 
> ________________________________________________________________________________________________________
> *** CID 312954:    (DC.WEAK_CRYPTO)
> /test/dm/mux-cmd.c: 133 in dm_test_cmd_mux_select()
> 127             ut_assertnonnull(chip);
> 128
> 129             srand(get_ticks() + rand());
> 130             for (i = 0; i < chip->controllers; i++) {
> 131                     mux = &chip->mux[i];
> 132
> >>>     CID 312954:    (DC.WEAK_CRYPTO)
> >>>     "rand" should not be used for security-related applications, because linear congruential algorithms are too easy to break.

Not used for any security-related applications. No changes needed.

BTW, is the assertion that rand() is "linear congruential" even true for 
U-Boot's rand() or is it only true for the libc rand()?

> 133                     state = rand() % mux->states;
> 134
> 135                     snprintf(cmd, BUF_SIZE, "mux select
> a-mux-controller %x %x", i,
> 136                              state);
> 137                     run_command(cmd, 0);
> 138                     ut_asserteq(!!mux->in_use, true);
> /test/dm/mux-cmd.c: 129 in dm_test_cmd_mux_select()
> 123
> 124             ut_assertok(uclass_get_device_by_name(UCLASS_MUX,
> "a-mux-controller",
> 125                                                   &dev));
> 126             chip = dev_get_uclass_priv(dev);
> 127             ut_assertnonnull(chip);
> 128
> >>>     CID 312954:    (DC.WEAK_CRYPTO)
> >>>     "rand" should not be used for security-related applications, because linear congruential algorithms are too easy to break.

Same as above.

> 129             srand(get_ticks() + rand());
> 130             for (i = 0; i < chip->controllers; i++) {
> 131                     mux = &chip->mux[i];
> 132
> 133                     state = rand() % mux->states;
> 134
> 
> *** CID 312952:  Resource leaks  (RESOURCE_LEAK)
> /drivers/reset/reset-uclass.c: 331 in devm_reset_bulk_get_by_node()
> 325                                 __GFP_ZERO);
> 326             if (unlikely(!bulk))
> 327                     return ERR_PTR(-ENOMEM);
> 328
> 329             rc = __reset_get_bulk(dev, node, bulk);
> 330             if (rc)
> >>>     CID 312952:  Resource leaks  (RESOURCE_LEAK)
> >>>     Variable "bulk" going out of scope leaks the storage it points to.

Similar problem as that of idle_states above. Not sure if memory 
allocated by devres_alloc() gets freed automatically but in this case I 
get the feeling it won't be.

> 331                     return ERR_PTR(rc);
> 332
> 333             devres_add(dev, bulk);
> 334             return bulk;
> 335     }
> 336
> 
> ** CID 312951:    (RESOURCE_LEAK)
> /drivers/core/regmap.c: 315 in devm_regmap_init()
> /drivers/core/regmap.c: 315 in devm_regmap_init()
> /drivers/core/regmap.c: 306 in devm_regmap_init()
> /drivers/core/regmap.c: 306 in devm_regmap_init()
> 
> 
> ________________________________________________________________________________________________________
> *** CID 312951:    (RESOURCE_LEAK)
> /drivers/core/regmap.c: 315 in devm_regmap_init()
> 309             if (config) {
> 310                     map->width = config->width;
> 311                     map->reg_offset_shift = config->reg_offset_shift;
> 312             }
> 313
> 314             devres_add(dev, mapp);
> >>>     CID 312951:    (RESOURCE_LEAK)
> >>>     Variable "mapp" going out of scope leaks the storage it points to.

False positive because mapp is passed to devres_add().

> 315             return *mapp;
> 316     }
> 317     #endif
> 318
> 319     void *regmap_get_range(struct regmap *map, unsigned int range_num)
> 320     {
> /drivers/core/regmap.c: 315 in devm_regmap_init()
> 309             if (config) {
> 310                     map->width = config->width;
> 311                     map->reg_offset_shift = config->reg_offset_shift;
> 312             }
> 313
> 314             devres_add(dev, mapp);
> >>>     CID 312951:    (RESOURCE_LEAK)
> >>>     Variable "mapp" going out of scope leaks the storage it points to.

Same as above.

> 315             return *mapp;
> 316     }
> 317     #endif
> 318
> 319     void *regmap_get_range(struct regmap *map, unsigned int range_num)
> 320     {
> /drivers/core/regmap.c: 306 in devm_regmap_init()
> 300             if (config && config->r_size != 0)
> 301                     rc = regmap_init_mem_range(dev_ofnode(dev),
> config->r_start,
> 302                                                config->r_size, mapp);
> 303             else
> 304                     rc = regmap_init_mem(dev_ofnode(dev), mapp);
> 305             if (rc)
> >>>     CID 312951:    (RESOURCE_LEAK)
> >>>     Variable "mapp" going out of scope leaks the storage it points to.

Hmm... We have not passed it to devres_add() yet. So this looks same as 
the problem with 'bulk' above. I think it is a leak but I would like 
someone to confirm my suspicion.

> 306                     return ERR_PTR(rc);
> 307
> 308             map = *mapp;
> 309             if (config) {
> 310                     map->width = config->width;
> 311                     map->reg_offset_shift = config->reg_offset_shift;
> /drivers/core/regmap.c: 306 in devm_regmap_init()
> 300             if (config && config->r_size != 0)
> 301                     rc = regmap_init_mem_range(dev_ofnode(dev),
> config->r_start,
> 302                                                config->r_size, mapp);
> 303             else
> 304                     rc = regmap_init_mem(dev_ofnode(dev), mapp);
> 305             if (rc)
> >>>     CID 312951:    (RESOURCE_LEAK)
> >>>     Variable "mapp" going out of scope leaks the storage it points to.

Same as above.

> 306                     return ERR_PTR(rc);
> 307
> 308             map = *mapp;
> 309             if (config) {
> 310                     map->width = config->width;
> 311                     map->reg_offset_shift = config->reg_offset_shift;
> ________________________________________________________________________________________________________
> *** CID 312949:    (DC.WEAK_CRYPTO)
> /test/dm/regmap.c: 310 in dm_test_devm_regmap()
> 304             ut_assertok(uclass_get_device_by_name(UCLASS_NOP,
> "regmap-test_0",
> 305                                                   &dev));
> 306             priv = dev_get_priv(dev);
> 307
> 308             srand(get_ticks() + rand());
> 309             for (i = 0; i < REGMAP_TEST_BUF_SZ; i++) {
> >>>     CID 312949:    (DC.WEAK_CRYPTO)
> >>>     "rand" should not be used for security-related applications, because linear congruential algorithms are too easy to break.

False positive. Not used for any security-related applications.

> 310                     pattern[i] = rand();
> 311                     ut_assertok(regmap_write(priv->cfg_regmap, i,
> pattern[i]));
> 312             }
> 313             for (i = 0; i < REGMAP_TEST_BUF_SZ; i++) {
> 314                     ut_assertok(regmap_read(priv->cfg_regmap, i, &val));
> 315                     ut_asserteq(val, buffer[i]);
> /test/dm/regmap.c: 308 in dm_test_devm_regmap()
> 302                                  REGMAP_TEST_BUF_SZ * 2, MAP_NOCACHE);
> 303
> 304             ut_assertok(uclass_get_device_by_name(UCLASS_NOP,
> "regmap-test_0",
> 305                                                   &dev));
> 306             priv = dev_get_priv(dev);
> 307
> >>>     CID 312949:    (DC.WEAK_CRYPTO)
> >>>     "rand" should not be used for security-related applications, because linear congruential algorithms are too easy to break.
> 308             srand(get_ticks() + rand());
> 309             for (i = 0; i < REGMAP_TEST_BUF_SZ; i++) {
> 310                     pattern[i] = rand();
> 311                     ut_assertok(regmap_write(priv->cfg_regmap, i,
> pattern[i]));
> 312             }
> 313             for (i = 0; i < REGMAP_TEST_BUF_SZ; i++) {
> 
> ________________________________________________________________________________________________________
> *** CID 312944:  Integer handling issues  (BAD_SHIFT)
> /drivers/mux/mmio.c: 107 in mmio_mux_probe()
> 101                     mask = mux_reg_masks[2 * i + 1];
> 102
> 103                     field.reg = reg;
> 104                     field.msb = fls(mask) - 1;
> 105                     field.lsb = ffs(mask) - 1;
> 106
> >>>     CID 312944:  Integer handling issues  (BAD_SHIFT)
> >>>     In expression "0xffffffffffffffffUL >> 63U - field.msb", right shifting by more than 63 bits has undefined behavior.  The shift amount, "63U - field.msb", is 64.

Same problem as above. The tool should show issues with one file in 
sequence...

> 107                     if (mask != GENMASK(field.msb, field.lsb))
> 108                             return log_msg_ret("invalid mask", -EINVAL);
> 109
> 110                     fields[i] = devm_regmap_field_alloc(dev, regmap, field);
> 111                     if (IS_ERR(fields[i])) {
> 112                             ret = PTR_ERR(fields[i]);
> 
> ________________________________________________________________________________________________________
> To view the defects in Coverity Scan visit,
> https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0qcxCbhZ31OYv50yoA22WlOQ-2By3ieUvdbKmOyw68TMVT4Kip-2BBzfOGWXJ5yIiYplmPF9KAnKIja4Zd7tU-3DG16z_EEm8SbLgSDsaDZif-2Bv7ch8WqhKpLoKErHi4nXpwDNTttU5wxtf-2BIrYIlH6m8usGJ6Lj2sxuVx1MrdzdzgACo0LT3OFouHYVv45XtjGnMdnBHVdXsmw-2F0hVbOCFNnsrngQZqCc0sAyWQDCDYCMOEtivMS6hgdPFHSlGRRb51oma2tiPKUAklqWROrvI4MyXxqrp-2Fd4gBcYvc7-2FLXQFG0CyHS3IAPBDTyEFObYQ4RE2yA-3D
> 
>   To manage Coverity Scan email notifications for
> "tom.rini at gmail.com", click
> https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0qcxCbhZ31OYv50yped04pjJnmXOsUBtKYNIXxWeIHzDeopm-2BEWQ6S6K-2FtUHv9ZTk8qZbuzkkz9sa-2BJFw4elYDyedRVZOC-2ButxjBZdouVmTGuWB6Aj6G7lm7t25-2Biv1B-2B9082pHzCCex2kqMs-3DEOqJ_EEm8SbLgSDsaDZif-2Bv7ch8WqhKpLoKErHi4nXpwDNTttU5wxtf-2BIrYIlH6m8usGJzaB1PzDyVpqw-2FdKI2nmJ1aeEn5herkK9wV7V6RjSEoYxghGutNP9BcObkZR3VG0GThMSPIO3YwHDptrjReecWG99Q7RAogK2ghwHTok4ICj9O-2FAA-2FumHtxTSCVEgN8DQdszAdaF0kCwbpvbxr33-2Bx8r4btBT-2Bj-2BqyAjW5wzAVl4-3D

Whew! That's a lot of issues with the patches I submitted! IMO the tool 
is mostly raising valid concerns and I think most of these are actual 
bugs.

I don't know how useful the rand() warning is though. I think it will be 
a false positive most of the time but maybe it is worth it for the one 
time it actually catches a security issue. Dunno.

-- 
Regards,
Pratyush Yadav
Texas Instruments India


More information about the U-Boot mailing list