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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 24825 bytes --]

On Wed, May 06, 2026 at 09:46:44AM +0200, Stefano Brivio wrote:
> 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,

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]