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 18:00:28 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

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

Indeed, but Coverity Scan doesn't seem to realise that.

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

It looks like it doesn't for some reason.

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

Feel free to propose a patch at a later point. I don't understand what
kind of changes you want. I'll document this better in v9 but that's
all I can do for the moment.

-- 
Stefano