[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-ad...@coverity.com> > Date: Wed, Oct 28, 2020 at 4:41 PM > Subject: New Defects reported by Coverity Scan for Das U-Boot > To: <tom.r...@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.r...@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