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

On Wed, 6 May 2026 18:00:28 +1000 David Gibson 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 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.

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