[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