[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@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@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-2F2Czv4BOaCWWCy7my0P0...
To manage Coverity Scan email notifications for "tom.rini@gmail.com", click https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0...
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.