Re: [PATCH v7 18/18] fwd_rule: Fix static checkers warnings in fwd_rule_add()

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 6 May 2026 00:41:15 +1000
David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au> 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 <sbrivio@redhat.com>    
> > > 
> > > 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, 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,
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.

> > >   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