On Wed, May 06, 2026 at 09:46:44AM +0200, Stefano Brivio wrote: > On Wed, 6 May 2026 00:41:15 +1000 > David Gibson wrote: > > > On Tue, May 05, 2026 at 12:13:41PM +0200, Stefano Brivio wrote: > > > On Tue, 5 May 2026 16:22:43 +1000 > > > David Gibson wrote: > > > > > > > On Tue, May 05, 2026 at 01:11:42AM +0200, Stefano Brivio wrote: > > > > > The new checks are actually sufficient but not enough for Coverity > > > > > Scan. Now that fwd->sock_count and new->last are affected or supplied > > > > > by clients, we need explicit (albeit redundant) checks on them. > > > > > > > > > > Signed-off-by: Stefano Brivio > > > > > > > > I'm assuming this does squash the warnings, but I think it does so in > > > > a somewhat confusing way. > > > > > > You don't need to assume that, you could try yourself without this > > > patch and you'll see exactly two warnings with a lot of details. > > > > I'm getting better, but I'm by no means well yet. Some emails were > > within my capacity. Sorting out the new license key and doing a > > Coverity run, not so much. > > > > > > > > > > --- > > > > > fwd_rule.c | 9 +++++++++ > > > > > 1 file changed, 9 insertions(+) > > > > > > > > > > diff --git a/fwd_rule.c b/fwd_rule.c > > > > > index b55e4df..03e8e80 100644 > > > > > --- a/fwd_rule.c > > > > > +++ b/fwd_rule.c > > > > > @@ -271,13 +271,22 @@ int fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new) > > > > > warn("Too many rules (maximum %d)", ARRAY_SIZE(fwd->rules)); > > > > > return -ENOSPC; > > > > > } > > > > > + > > > > > if ((fwd->sock_count + num) > ARRAY_SIZE(fwd->socks)) { > > > > > warn("Rules require too many listening sockets (maximum %d)", > > > > > ARRAY_SIZE(fwd->socks)); > > > > > return -ENOSPC; > > > > > } > > > > > + /* Redundant, to make static checkers happy */ > > > > > + if (fwd->sock_count > ARRAY_SIZE(fwd->socks)) > > > > > + return -ENOSPC; > > > > > > > > So there's actually two conditions that this is kind of relevant to: > > > > > > > > 1) (fwd->sock_count > ARRAY_SIZE(fwd->socks)) on entry > > > > > > > > That means something is horribly wrong before we were even called. > > > > So, I think that would be better as an assert(). > > But the (good) reason why Coverity Scan is complaining about a missing > check here is that the client is able to manipulate sock_count (via > num) in a previous call to this function, Uh... they should never be able to make sock_count outside of [0, ARRAY_SIZE(socks)). If they can, that's a bug elsewhere. > so making us crash is exactly > what we want to avoid: > > /home/sbrivio/passt/conf.c:2024:4: > Type: Untrusted value as argument (TAINTED_SCALAR) > > /home/sbrivio/passt/conf.c:1992:3: Tainted data flows to a taint sink > 1. path: Condition "read_u8(fd, &pif)", taking false branch. > /home/sbrivio/passt/conf.c:1995:3: > 2. path: Condition "pif == 0", taking false branch. > /home/sbrivio/passt/conf.c:1998:3: > 3. path: Condition "pif >= 4 /* (int)(sizeof (c->fwd_pending) / sizeof (c->fwd_pending[0])) */", taking false branch. > /home/sbrivio/passt/conf.c:1998:3: > 4. path: Condition "!(fwd = c->fwd_pending[pif])", taking false branch. > /home/sbrivio/passt/conf.c:2004:3: > 5. path: Condition "read_u32(fd, &count)", taking false branch. > /home/sbrivio/passt/conf.c:2007:3: > 6. path: Condition "count > 255U /* (1U << 8) - 1 */", taking false branch. > /home/sbrivio/passt/conf.c:2013:3: > 7. path: Condition "i < count", taking true branch. > /home/sbrivio/passt/conf.c:2014:4: > 8. tainted_argument: Calling function "fwd_rule_read" taints argument "r". > /home/sbrivio/passt/fwd_rule.c:659:2: Tainted data flows to a taint sink > 8.1. tainted_data_argument: Calling function "read_all_buf" taints parameter "*rule". > /home/sbrivio/passt/serialise.c:39:2: Tainted data flows to a taint sink > 8.1.1. var_assign_parm: Assigning: "p" = "buf". > /home/sbrivio/passt/serialise.c:41:2: > 8.1.2. path: Condition "left", taking true branch. > /home/sbrivio/passt/serialise.c:44:3: > 8.1.3. path: Condition "left <= len", taking true branch. > /home/sbrivio/passt/serialise.c:47:4: > 8.1.4. tainted_data_argument: Calling function "read" taints parameter "*p". [Note: The source code implementation of the function has been overridden by a builtin model.] > /home/sbrivio/passt/serialise.c:48:37: > 8.1.5. path: Condition "rc < 0", taking false branch. > /home/sbrivio/passt/serialise.c:50:3: > 8.1.6. path: Condition "rc < 0", taking false branch. > /home/sbrivio/passt/serialise.c:53:3: > 8.1.7. path: Condition "rc == 0", taking false branch. > /home/sbrivio/passt/serialise.c:60:2: > 8.1.8. path: Jumping back to the beginning of the loop. > /home/sbrivio/passt/serialise.c:41:2: > 8.1.9. path: Condition "left", taking true branch. > /home/sbrivio/passt/serialise.c:44:3: > 8.1.10. path: Condition "left <= len", taking true branch. > /home/sbrivio/passt/serialise.c:48:37: > 8.1.11. path: Condition "rc < 0", taking false branch. > /home/sbrivio/passt/serialise.c:50:3: > 8.1.12. path: Condition "rc < 0", taking false branch. > /home/sbrivio/passt/serialise.c:53:3: > 8.1.13. path: Condition "rc == 0", taking false branch. > /home/sbrivio/passt/serialise.c:60:2: > 8.1.14. path: Jumping back to the beginning of the loop. > /home/sbrivio/passt/serialise.c:41:2: > 8.1.15. path: Condition "left", taking false branch. > /home/sbrivio/passt/fwd_rule.c:659:2: > 8.2. path: Condition "read_all_buf(fd, rule, 40UL /* sizeof (*rule) */)", taking false branch. > /home/sbrivio/passt/conf.c:2014:4: > 9. path: Condition "fwd_rule_read(fd, &r)", taking false branch. > /home/sbrivio/passt/conf.c:2017:4: > 10. path: Condition "r.ifname[15UL /* sizeof (r.ifname) - 1 */]", taking false branch. > /home/sbrivio/passt/conf.c:2024:4: > 11. tainted_data_transitive: Call to function "fwd_rule_add" with tainted argument "r.last" transitively taints "fwd->sock_count". > /home/sbrivio/passt/fwd_rule.c:197:2: Tainted data flows to a taint sink > 11.1. path: Condition "new->first > new->last", taking false branch. > /home/sbrivio/passt/fwd_rule.c:202:2: > 11.2. path: Condition "!new->first", taking false branch. > /home/sbrivio/passt/fwd_rule.c:206:2: > 11.3. path: Condition "!new->to", taking false branch. > /home/sbrivio/passt/fwd_rule.c:206:2: > 11.4. path: Condition "(in_port_t)(new->to + new->last - new->first) < new->to", taking false branch. > /home/sbrivio/passt/fwd_rule.c:211:2: > 11.5. path: Condition "new->flags & -8 /* ~allowed_flags */", taking false branch. > /home/sbrivio/passt/fwd_rule.c:216:2: > 11.6. path: Condition "new->flags & (1UL /* 1UL << 0 */)", taking true branch. > /home/sbrivio/passt/fwd_rule.c:217:3: > 11.7. path: Condition "!inany_equals(&new->addr, (union inany_addr const *)&in6addr_any)", taking false branch. > /home/sbrivio/passt/fwd_rule.c:224:3: > 11.8. path: Condition "!(fwd->caps & (1UL /* 1UL << 0 */))", taking false branch. > /home/sbrivio/passt/fwd_rule.c:228:3: > 11.9. path: Condition "!(fwd->caps & (2UL /* 1UL << 1 */))", taking false branch. > /home/sbrivio/passt/fwd_rule.c:232:2: > 11.10. path: Falling through to end of if statement. > /home/sbrivio/passt/fwd_rule.c:242:2: > 11.11. path: Condition "new->proto == IPPROTO_TCP", taking true branch. > /home/sbrivio/passt/fwd_rule.c:243:3: > 11.12. path: Condition "!(fwd->caps & (4UL /* 1UL << 2 */))", taking false branch. > /home/sbrivio/passt/fwd_rule.c:247:2: > 11.13. path: Falling through to end of if statement. > /home/sbrivio/passt/fwd_rule.c:258:2: > 11.14. path: Condition "i < fwd->count", taking true branch. > /home/sbrivio/passt/fwd_rule.c:261:3: > 11.15. path: Condition "!fwd_rule_conflicts(new, &fwd->rules[i])", taking true branch. > /home/sbrivio/passt/fwd_rule.c:262:4: > 11.16. path: Continuing loop. > /home/sbrivio/passt/fwd_rule.c:258:2: > 11.17. path: Condition "i < fwd->count", taking true branch. > /home/sbrivio/passt/fwd_rule.c:261:3: > 11.18. path: Condition "!fwd_rule_conflicts(new, &fwd->rules[i])", taking true branch. > /home/sbrivio/passt/fwd_rule.c:262:4: > 11.19. path: Continuing loop. > /home/sbrivio/passt/fwd_rule.c:258:2: > 11.20. path: Condition "i < fwd->count", taking true branch. > /home/sbrivio/passt/fwd_rule.c:261:3: > 11.21. path: Condition "!fwd_rule_conflicts(new, &fwd->rules[i])", taking true branch. > /home/sbrivio/passt/fwd_rule.c:262:4: > 11.22. path: Continuing loop. > /home/sbrivio/passt/fwd_rule.c:258:2: > 11.23. path: Condition "i < fwd->count", taking false branch. > /home/sbrivio/passt/fwd_rule.c:270:2: > 11.24. path: Condition "fwd->count >= 255U /* (int)(sizeof (fwd->rules) / sizeof (fwd->rules[0])) */", taking false branch. > /home/sbrivio/passt/fwd_rule.c:274:2: > 11.25. path: Condition "fwd->sock_count + num > 327680U /* (int)(sizeof (fwd->socks) / sizeof (fwd->socks[0])) */", taking false branch. > /home/sbrivio/passt/fwd_rule.c:281:2: > 11.26. path: Condition "port <= new->last", taking true branch. > /home/sbrivio/passt/fwd_rule.c:282:53: > 11.27. path: Jumping back to the beginning of the loop. > /home/sbrivio/passt/fwd_rule.c:281:2: > 11.28. path: Condition "port <= new->last", taking false branch. > /home/sbrivio/passt/fwd_rule.c:285:2: > 11.29. parm_assign: Assigning: "fwd->sock_count" += "num", which taints "fwd->sock_count". > /home/sbrivio/passt/conf.c:2024:4: > 12. path: Condition "fwd_rule_add(fwd, &r) < 0", taking false branch. > /home/sbrivio/passt/conf.c:2026:3: > 13. path: Jumping back to the beginning of the loop. > /home/sbrivio/passt/conf.c:2013:3: > 14. path: Condition "i < count", taking true branch. > /home/sbrivio/passt/conf.c:2014:4: > 15. path: Condition "fwd_rule_read(fd, &r)", taking false branch. > /home/sbrivio/passt/conf.c:2017:4: > 16. path: Condition "r.ifname[15UL /* sizeof (r.ifname) - 1 */]", taking false branch. > /home/sbrivio/passt/conf.c:2024:4: > 17. tainted_data: Passing tainted expression "fwd->sock_count" to "fwd_rule_add", which uses it as an offset. > /home/sbrivio/passt/fwd_rule.c:197:2: Tainted data flows to a taint sink > 17.1. path: Condition "new->first > new->last", taking false branch. > /home/sbrivio/passt/fwd_rule.c:202:2: > 17.2. path: Condition "!new->first", taking false branch. > /home/sbrivio/passt/fwd_rule.c:206:2: > 17.3. path: Condition "!new->to", taking false branch. > /home/sbrivio/passt/fwd_rule.c:206:2: > 17.4. path: Condition "(in_port_t)(new->to + new->last - new->first) < new->to", taking false branch. > /home/sbrivio/passt/fwd_rule.c:211:2: > 17.5. path: Condition "new->flags & -8 /* ~allowed_flags */", taking false branch. > /home/sbrivio/passt/fwd_rule.c:216:2: > 17.6. path: Condition "new->flags & (1UL /* 1UL << 0 */)", taking true branch. > /home/sbrivio/passt/fwd_rule.c:217:3: > 17.7. path: Condition "!inany_equals(&new->addr, (union inany_addr const *)&in6addr_any)", taking false branch. > /home/sbrivio/passt/fwd_rule.c:224:3: > 17.8. path: Condition "!(fwd->caps & (1UL /* 1UL << 0 */))", taking false branch. > /home/sbrivio/passt/fwd_rule.c:228:3: > 17.9. path: Condition "!(fwd->caps & (2UL /* 1UL << 1 */))", taking false branch. > /home/sbrivio/passt/fwd_rule.c:232:2: > 17.10. path: Falling through to end of if statement. > /home/sbrivio/passt/fwd_rule.c:242:2: > 17.11. path: Condition "new->proto == IPPROTO_TCP", taking true branch. > /home/sbrivio/passt/fwd_rule.c:243:3: > 17.12. path: Condition "!(fwd->caps & (4UL /* 1UL << 2 */))", taking false branch. > /home/sbrivio/passt/fwd_rule.c:247:2: > 17.13. path: Falling through to end of if statement. > /home/sbrivio/passt/fwd_rule.c:258:2: > 17.14. path: Condition "i < fwd->count", taking true branch. > /home/sbrivio/passt/fwd_rule.c:261:3: > 17.15. path: Condition "!fwd_rule_conflicts(new, &fwd->rules[i])", taking true branch. > /home/sbrivio/passt/fwd_rule.c:262:4: > 17.16. path: Continuing loop. > /home/sbrivio/passt/fwd_rule.c:258:2: > 17.17. path: Condition "i < fwd->count", taking true branch. > /home/sbrivio/passt/fwd_rule.c:261:3: > 17.18. path: Condition "!fwd_rule_conflicts(new, &fwd->rules[i])", taking true branch. > /home/sbrivio/passt/fwd_rule.c:262:4: > 17.19. path: Continuing loop. > /home/sbrivio/passt/fwd_rule.c:258:2: > 17.20. path: Condition "i < fwd->count", taking true branch. > /home/sbrivio/passt/fwd_rule.c:261:3: > 17.21. path: Condition "!fwd_rule_conflicts(new, &fwd->rules[i])", taking true branch. > /home/sbrivio/passt/fwd_rule.c:262:4: > 17.22. path: Continuing loop. > /home/sbrivio/passt/fwd_rule.c:258:2: > 17.23. path: Condition "i < fwd->count", taking false branch. > /home/sbrivio/passt/fwd_rule.c:270:2: > 17.24. path: Condition "fwd->count >= 255U /* (int)(sizeof (fwd->rules) / sizeof (fwd->rules[0])) */", taking false branch. > /home/sbrivio/passt/fwd_rule.c:274:2: > 17.25. path: Condition "fwd->sock_count + num > 327680U /* (int)(sizeof (fwd->socks) / sizeof (fwd->socks[0])) */", taking false branch. > /home/sbrivio/passt/fwd_rule.c:280:2: > 17.26. data_index: Using tainted expression "fwd->sock_count" as an index to array "fwd->socks". > /home/sbrivio/passt/conf.c:2024:4: > 18. remediation: Ensure that tainted values are properly sanitized, by checking that their values are within a permissible range. > > At the same time, Coverity Scan doesn't realise that 'num' is positive, Uh.. what? num is unsigned, so it must know it's positive. > but we know that, so this will never trigger. As I wrote in the > comment, this check is redundant and it won't trigger. It's just good > to have to avoid noise from false positives. Agreed, but sock_count < ARRAY_SIZE() is a fundamental invariant of the data structure, so that makes more sense to document/enforce with a check. > > > > > 2) (fwd->sock_count + num) overflows > > > > > > > > That's a closer-to-real concern. I'm pretty sure we can't hit it for > > > > real, because num is necessarily <= 65536, so as long as (1) is true > > > > this can't overflow. But that relies on the specific value of > > > > ARRAY_SIZE(fwd->socks), so it's kind of fragile. > > > > > > > > I think an explicit check for this is a good idea, but it should > > > > actually check for this, not just side-effects of it, so: > > > > if (fwd->sock_count + num <= fwd->sock_count) { > > > > warn("Blah blah overflow"); > > > > return -EFAULT; /* or whatever */ > > > > } > > > > > > > > > fwd->rulesocks[fwd->count] = &fwd->socks[fwd->sock_count]; > > > > > + > > > > > + /* Redundant ('num' checked above), but not for static checkers */ > > > > > + if (new->last > ARRAY_SIZE(fwd->socks) + new->first) > > > > > + return -ENOSPC; > > > > > > > > This way of organising the check is very confusing to me. I'm not > > > > really sure what it's trying to catch. > > > > > > Same as above. > > > > I'm not sure which "above" you mean. > > Same here: the warning is pretty clear: > > /home/sbrivio/passt/conf.c:2024:4: > Type: Untrusted loop bound (TAINTED_SCALAR) > > /home/sbrivio/passt/conf.c:1992:3: Tainted data flows to a taint sink > 1. path: Condition "read_u8(fd, &pif)", taking false branch. > /home/sbrivio/passt/conf.c:1995:3: > 2. path: Condition "pif == 0", taking false branch. > /home/sbrivio/passt/conf.c:1998:3: > 3. path: Condition "pif >= 4 /* (int)(sizeof (c->fwd_pending) / sizeof (c->fwd_pending[0])) */", taking false branch. > /home/sbrivio/passt/conf.c:1998:3: > 4. path: Condition "!(fwd = c->fwd_pending[pif])", taking false branch. > /home/sbrivio/passt/conf.c:2004:3: > 5. path: Condition "read_u32(fd, &count)", taking false branch. > /home/sbrivio/passt/conf.c:2007:3: > 6. path: Condition "count > 255U /* (1U << 8) - 1 */", taking false branch. > /home/sbrivio/passt/conf.c:2013:3: > 7. path: Condition "i < count", taking true branch. > /home/sbrivio/passt/conf.c:2014:4: > 8. path: Condition "fwd_rule_read(fd, &r)", taking false branch. > /home/sbrivio/passt/conf.c:2017:4: > 9. path: Condition "r.ifname[15UL /* sizeof (r.ifname) - 1 */]", taking false branch. > /home/sbrivio/passt/conf.c:2024:4: > 10. path: Condition "fwd_rule_add(fwd, &r) < 0", taking false branch. > /home/sbrivio/passt/conf.c:2026:3: > 11. path: Jumping back to the beginning of the loop. > /home/sbrivio/passt/conf.c:2013:3: > 12. path: Condition "i < count", taking true branch. > /home/sbrivio/passt/conf.c:2014:4: > 13. tainted_argument: Calling function "fwd_rule_read" taints argument "r". > /home/sbrivio/passt/fwd_rule.c:659:2: Tainted data flows to a taint sink > 13.1. tainted_data_argument: Calling function "read_all_buf" taints parameter "*rule". > /home/sbrivio/passt/serialise.c:39:2: Tainted data flows to a taint sink > 13.1.1. var_assign_parm: Assigning: "p" = "buf". > /home/sbrivio/passt/serialise.c:41:2: > 13.1.2. path: Condition "left", taking true branch. > /home/sbrivio/passt/serialise.c:44:3: > 13.1.3. path: Condition "left <= len", taking true branch. > /home/sbrivio/passt/serialise.c:47:4: > 13.1.4. tainted_data_argument: Calling function "read" taints parameter "*p". [Note: The source code implementation of the function has been overridden by a builtin model.] > /home/sbrivio/passt/serialise.c:48:37: > 13.1.5. path: Condition "rc < 0", taking false branch. > /home/sbrivio/passt/serialise.c:50:3: > 13.1.6. path: Condition "rc < 0", taking false branch. > /home/sbrivio/passt/serialise.c:53:3: > 13.1.7. path: Condition "rc == 0", taking false branch. > /home/sbrivio/passt/serialise.c:60:2: > 13.1.8. path: Jumping back to the beginning of the loop. > /home/sbrivio/passt/serialise.c:41:2: > 13.1.9. path: Condition "left", taking true branch. > /home/sbrivio/passt/serialise.c:44:3: > 13.1.10. path: Condition "left <= len", taking true branch. > /home/sbrivio/passt/serialise.c:48:37: > 13.1.11. path: Condition "rc < 0", taking false branch. > /home/sbrivio/passt/serialise.c:50:3: > 13.1.12. path: Condition "rc < 0", taking false branch. > /home/sbrivio/passt/serialise.c:53:3: > 13.1.13. path: Condition "rc == 0", taking false branch. > /home/sbrivio/passt/serialise.c:60:2: > 13.1.14. path: Jumping back to the beginning of the loop. > /home/sbrivio/passt/serialise.c:41:2: > 13.1.15. path: Condition "left", taking false branch. > /home/sbrivio/passt/fwd_rule.c:659:2: > 13.2. path: Condition "read_all_buf(fd, rule, 40UL /* sizeof (*rule) */)", taking false branch. > /home/sbrivio/passt/conf.c:2014:4: > 14. path: Condition "fwd_rule_read(fd, &r)", taking false branch. > /home/sbrivio/passt/conf.c:2017:4: > 15. path: Condition "r.ifname[15UL /* sizeof (r.ifname) - 1 */]", taking false branch. > /home/sbrivio/passt/conf.c:2024:4: > 16. tainted_data: Passing tainted expression "r.last" to "fwd_rule_add", which uses it as a loop boundary. > /home/sbrivio/passt/fwd_rule.c:197:2: Tainted data flows to a taint sink > 16.1. lower_bounds: Casting narrower unsigned "new->last" to wider signed type "int" effectively tests its lower bound. > /home/sbrivio/passt/fwd_rule.c:197:2: > 16.2. path: Condition "new->first > new->last", taking false branch. > /home/sbrivio/passt/fwd_rule.c:197:2: > 16.3. lower_bounds: Checking lower bounds of unsigned scalar "new->last" by taking the false branch of "new->first > new->last". > /home/sbrivio/passt/fwd_rule.c:202:2: > 16.4. path: Condition "!new->first", taking false branch. > /home/sbrivio/passt/fwd_rule.c:206:2: > 16.5. path: Condition "!new->to", taking false branch. > /home/sbrivio/passt/fwd_rule.c:206:2: > 16.6. lower_bounds: Casting narrower unsigned "new->last" to wider signed type "int" effectively tests its lower bound. > /home/sbrivio/passt/fwd_rule.c:206:2: > 16.7. path: Condition "(in_port_t)(new->to + new->last - new->first) < new->to", taking false branch. > /home/sbrivio/passt/fwd_rule.c:211:2: > 16.8. path: Condition "new->flags & -8 /* ~allowed_flags */", taking false branch. > /home/sbrivio/passt/fwd_rule.c:216:2: > 16.9. path: Condition "new->flags & (1UL /* 1UL << 0 */)", taking true branch. > /home/sbrivio/passt/fwd_rule.c:217:3: > 16.10. path: Condition "!inany_equals(&new->addr, (union inany_addr const *)&in6addr_any)", taking false branch. > /home/sbrivio/passt/fwd_rule.c:224:3: > 16.11. path: Condition "!(fwd->caps & (1UL /* 1UL << 0 */))", taking false branch. > /home/sbrivio/passt/fwd_rule.c:228:3: > 16.12. path: Condition "!(fwd->caps & (2UL /* 1UL << 1 */))", taking false branch. > /home/sbrivio/passt/fwd_rule.c:232:2: > 16.13. path: Falling through to end of if statement. > /home/sbrivio/passt/fwd_rule.c:242:2: > 16.14. path: Condition "new->proto == IPPROTO_TCP", taking true branch. > /home/sbrivio/passt/fwd_rule.c:243:3: > 16.15. path: Condition "!(fwd->caps & (4UL /* 1UL << 2 */))", taking false branch. > /home/sbrivio/passt/fwd_rule.c:247:2: > 16.16. path: Falling through to end of if statement. > /home/sbrivio/passt/fwd_rule.c:258:2: > 16.17. path: Condition "i < fwd->count", taking true branch. > /home/sbrivio/passt/fwd_rule.c:261:3: > 16.18. path: Condition "!fwd_rule_conflicts(new, &fwd->rules[i])", taking true branch. > /home/sbrivio/passt/fwd_rule.c:262:4: > 16.19. path: Continuing loop. > /home/sbrivio/passt/fwd_rule.c:258:2: > 16.20. path: Condition "i < fwd->count", taking true branch. > /home/sbrivio/passt/fwd_rule.c:261:3: > 16.21. path: Condition "!fwd_rule_conflicts(new, &fwd->rules[i])", taking true branch. > /home/sbrivio/passt/fwd_rule.c:262:4: > 16.22. path: Continuing loop. > /home/sbrivio/passt/fwd_rule.c:258:2: > 16.23. path: Condition "i < fwd->count", taking false branch. > /home/sbrivio/passt/fwd_rule.c:270:2: > 16.24. path: Condition "fwd->count >= 255U /* (int)(sizeof (fwd->rules) / sizeof (fwd->rules[0])) */", taking false branch. > /home/sbrivio/passt/fwd_rule.c:274:2: > 16.25. path: Condition "fwd->sock_count + num > 327680U /* (int)(sizeof (fwd->socks) / sizeof (fwd->socks[0])) */", taking false branch. > /home/sbrivio/passt/fwd_rule.c:281:2: > 16.26. loop_bound_upper: Using tainted expression "new->last" as a loop boundary. > /home/sbrivio/passt/conf.c:2024:4: > 17. remediation: Ensure that tainted values are properly sanitized, by checking that their values are within a permissible range. > > I'll expand the comment and remove the newline between this and the for > loop. > > > > > We've already checked that > > > > last >= first, so using num is safer to deal with at this > > > > point than ARRAY_SIZE() + first, which could in principle overflow > > > > even if sock_count + num is perfectly ok. > > > > > > Using 'num' won't work. It shouldn't overflow anyway because the > > > addition happens in 'int'. > > > > It shouldn't overflow, but proving that requires knowing that: > > a. sock_count is bounded by ARRAY_SIZE(socks) > > b. first, last and num are bounded by 2^16 > > c. ARRAY_SIZE(socks) + 2^16 won't overflow (in unsigned int) > > > > I'm not sure which part coverity is missing. (c) at least requires > > knowledge which is not found in immediately adjacent code. > > It's missing the fact that 'num' is already checked above (that is, > a.), and due to that we know we're not overflowing fwd->rulesocks[x]. > > > Oh... and... I just remembered that ARRAY_SIZE() is int, not unsigned > > (or size_t). I thought about changing that at some point, but it > > seemed to cause more trouble than it was worth. Does keep tripping me > > though, since it seems like it logically ought to be unsigned. Signed > > overflows are much nastier (UB) that unsigned overflows. > > > > > I'll try to change the rest if I find some time but it doesn't really > > > look that critical to me. > > > > > > > > for (port = new->first; port <= new->last; port++) > > > > > fwd->rulesocks[fwd->count][port - new->first] = -1; > > > > > > > > > > -- > > > > > 2.43.0 > > > > > > > > > > > > > > > -- > > > Stefano > > > > > > > -- > Stefano > -- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson