From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202602 header.b=PzeR0Fg3; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 8913A5A0269 for ; Wed, 06 May 2026 10:09:05 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202602; t=1778054942; bh=qzi55jGkyuqa2I5rq3DGxAfAIXr123AXd44pJcWnChU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=PzeR0Fg3Fcr2N0cAmlfpo89lnjBIEl3llKNUWCSUNpcqBiZartuPd1g5Jw2MnI1b3 qaTKHAIS/dSh6LcZV6y4hbn31kNai0oiLB3Rhnzivr0P7M5ZCYb4njxM3WUytyXGoW CbtlLgm4fMOcYX8l7cEGN3sUQPWiKdVIuC65oJCyWHu0TWgFQdeikIO5GhppzDbEJs gXTVAZGNVRDDLKxDhf5dRq/louPkIgybTcQCCo5fDKrPD/em9eGI5HJqkcO2g2n2b7 dyaOBnSWBxmXOB5W+1b9LjI3WTs9zYTjmKfQkQ/odh8g4JnfV78DpeBdwbiuJ3PvWd KWGaiaxW+kCng== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4g9ShG4MrJz4wK7; Wed, 06 May 2026 18:09:02 +1000 (AEST) Date: Wed, 6 May 2026 18:00:28 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v7 18/18] fwd_rule: Fix static checkers warnings in fwd_rule_add() Message-ID: References: <20260504231142.1118652-1-sbrivio@redhat.com> <20260504231142.1118652-19-sbrivio@redhat.com> <20260505121340.3a548603@elisabeth> <20260506094644.18499890@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="vn3EagKx/0DoaBN5" Content-Disposition: inline In-Reply-To: <20260506094644.18499890@elisabeth> Message-ID-Hash: H7G6LQLOCZ6NWVWAVUK4W4WIAURT5UTZ X-Message-ID-Hash: H7G6LQLOCZ6NWVWAVUK4W4WIAURT5UTZ X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, Jon Maloy , Laurent Vivier X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --vn3EagKx/0DoaBN5 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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: >=20 > > 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: > > > =20 > > > > On Tue, May 05, 2026 at 01:11:42AM +0200, Stefano Brivio wrote: =20 > > > > > The new checks are actually sufficient but not enough for Coverity > > > > > Scan. Now that fwd->sock_count and new->last are affected or supp= lied > > > > > by clients, we need explicit (albeit redundant) checks on them. > > > > >=20 > > > > > Signed-off-by: Stefano Brivio =20 > > > >=20 > > > > I'm assuming this does squash the warnings, but I think it does so = in > > > > a somewhat confusing way. =20 > > >=20 > > > 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. =20 > >=20 > > 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. > >=20 > > > =20 > > > > > --- > > > > > fwd_rule.c | 9 +++++++++ > > > > > 1 file changed, 9 insertions(+) > > > > >=20 > > > > > 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, con= st 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; =20 > > > >=20 > > > > So there's actually two conditions that this is kind of relevant to: > > > >=20 > > > > 1) (fwd->sock_count > ARRAY_SIZE(fwd->socks)) on entry > > > >=20 > > > > That means something is horribly wrong before we were even called. > > > > So, I think that would be better as an assert(). >=20 > 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: >=20 > /home/sbrivio/passt/conf.c:2024:4: > Type: Untrusted value as argument (TAINTED_SCALAR) >=20 > /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 =3D=3D 0", taking false branch. > /home/sbrivio/passt/conf.c:1998:3: > 3. path: Condition "pif >=3D 4 /* (int)(sizeof (c->fwd_pending) / sizeo= f (c->fwd_pending[0])) */", taking false branch. > /home/sbrivio/passt/conf.c:1998:3: > 4. path: Condition "!(fwd =3D c->fwd_pending[pif])", taking false branc= h. > /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 bra= nch. > /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 para= meter "*rule". > /home/sbrivio/passt/serialise.c:39:2: Tainted data flows to a taint sink > 8.1.1. var_assign_parm: Assigning: "p" =3D "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 <=3D 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 overri= dden 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 =3D=3D 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 <=3D 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 =3D=3D 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 */]", takin= g false branch. > /home/sbrivio/passt/conf.c:2024:4: > 11. tainted_data_transitive: Call to function "fwd_rule_add" with taint= ed 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 fa= lse 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 cons= t *)&in6addr_any)", taking false branch. > /home/sbrivio/passt/fwd_rule.c:224:3: > 11.8. path: Condition "!(fwd->caps & (1UL /* 1UL << 0 */))", taking fal= se branch. > /home/sbrivio/passt/fwd_rule.c:228:3: > 11.9. path: Condition "!(fwd->caps & (2UL /* 1UL << 1 */))", taking fal= se 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 =3D=3D IPPROTO_TCP", taking true bra= nch. > /home/sbrivio/passt/fwd_rule.c:243:3: > 11.12. path: Condition "!(fwd->caps & (4UL /* 1UL << 2 */))", taking fa= lse 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])", taki= ng 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])", taki= ng 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])", taki= ng 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 >=3D 255U /* (int)(sizeof (fwd->rule= s) / 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 <=3D 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 <=3D new->last", taking false branch. > /home/sbrivio/passt/fwd_rule.c:285:2: > 11.29. parm_assign: Assigning: "fwd->sock_count" +=3D "num", which tain= ts "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 */]", takin= g 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 fa= lse 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 cons= t *)&in6addr_any)", taking false branch. > /home/sbrivio/passt/fwd_rule.c:224:3: > 17.8. path: Condition "!(fwd->caps & (1UL /* 1UL << 0 */))", taking fal= se branch. > /home/sbrivio/passt/fwd_rule.c:228:3: > 17.9. path: Condition "!(fwd->caps & (2UL /* 1UL << 1 */))", taking fal= se 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 =3D=3D IPPROTO_TCP", taking true bra= nch. > /home/sbrivio/passt/fwd_rule.c:243:3: > 17.12. path: Condition "!(fwd->caps & (4UL /* 1UL << 2 */))", taking fa= lse 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])", taki= ng 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])", taki= ng 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])", taki= ng 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 >=3D 255U /* (int)(sizeof (fwd->rule= s) / 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 ind= ex 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. >=20 > 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. >=20 > > > > 2) (fwd->sock_count + num) overflows > > > >=20 > > > > That's a closer-to-real concern. I'm pretty sure we can't hit it f= or > > > > real, because num is necessarily <=3D 65536, so as long as (1) is t= rue > > > > this can't overflow. But that relies on the specific value of > > > > ARRAY_SIZE(fwd->socks), so it's kind of fragile. > > > >=20 > > > > 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 <=3D fwd->sock_count) { > > > > warn("Blah blah overflow"); > > > > return -EFAULT; /* or whatever */ > > > > } > > > > =20 > > > > > fwd->rulesocks[fwd->count] =3D &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; =20 > > > >=20 > > > > This way of organising the check is very confusing to me. I'm not > > > > really sure what it's trying to catch. =20 > > >=20 > > > Same as above. =20 > >=20 > > I'm not sure which "above" you mean. >=20 > Same here: the warning is pretty clear: >=20 > /home/sbrivio/passt/conf.c:2024:4: > Type: Untrusted loop bound (TAINTED_SCALAR) >=20 > /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 =3D=3D 0", taking false branch. > /home/sbrivio/passt/conf.c:1998:3: > 3. path: Condition "pif >=3D 4 /* (int)(sizeof (c->fwd_pending) / sizeo= f (c->fwd_pending[0])) */", taking false branch. > /home/sbrivio/passt/conf.c:1998:3: > 4. path: Condition "!(fwd =3D c->fwd_pending[pif])", taking false branc= h. > /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 bra= nch. > /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 par= ameter "*rule". > /home/sbrivio/passt/serialise.c:39:2: Tainted data flows to a taint sink > 13.1.1. var_assign_parm: Assigning: "p" =3D "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 <=3D 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 overr= idden 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 =3D=3D 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 <=3D 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 =3D=3D 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 */]", takin= g 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 sign= ed 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 sign= ed 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 fa= lse 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 con= st *)&in6addr_any)", taking false branch. > /home/sbrivio/passt/fwd_rule.c:224:3: > 16.11. path: Condition "!(fwd->caps & (1UL /* 1UL << 0 */))", taking fa= lse branch. > /home/sbrivio/passt/fwd_rule.c:228:3: > 16.12. path: Condition "!(fwd->caps & (2UL /* 1UL << 1 */))", taking fa= lse 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 =3D=3D IPPROTO_TCP", taking true bra= nch. > /home/sbrivio/passt/fwd_rule.c:243:3: > 16.15. path: Condition "!(fwd->caps & (4UL /* 1UL << 2 */))", taking fa= lse 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])", taki= ng 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])", taki= ng 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 >=3D 255U /* (int)(sizeof (fwd->rule= s) / 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. >=20 > I'll expand the comment and remove the newline between this and the for > loop. >=20 > > > > We've already checked that > > > > last >=3D 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. =20 > > >=20 > > > Using 'num' won't work. It shouldn't overflow anyway because the > > > addition happens in 'int'. =20 > >=20 > > 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) > >=20 > > I'm not sure which part coverity is missing. (c) at least requires > > knowledge which is not found in immediately adjacent code. >=20 > 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]. >=20 > > 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. > >=20 > > > I'll try to change the rest if I find some time but it doesn't really > > > look that critical to me. > > > =20 > > > > > for (port =3D new->first; port <=3D new->last; port++) > > > > > fwd->rulesocks[fwd->count][port - new->first] =3D -1; > > > > > =20 > > > > > --=20 > > > > > 2.43.0 > > > > > =20 > > > > =20 > > >=20 > > > --=20 > > > Stefano > > > =20 > >=20 >=20 > --=20 > Stefano >=20 --=20 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 --vn3EagKx/0DoaBN5 Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmn69REACgkQzQJF27ox 2GfhtQ//ZaYDvivH6H7HL74KEY3rs128eVFHF67R57RkoKBV3Wecy6LRnG5Usopv ri6a9tM7/85giDfKSFJR/6YCaiR72V/npYNTaijB11EfhgbBhQ8RdNrix+yTQGqS OY+r6WDGZhU3+O5ieph7Xq0BqCKs63StMerccNtFJDmumaLsqZeM8q5tqRPNQbGH a+9PuvUCS1jhOxvAdi04S9v/iu7WfpdHWH8XkMoZtlsjlSMXl1aOmK/y6tuS4M8G 1af5BBKGjom+am/NoTXges0oY+FU1xTy+P5fe+waNMeseUilDwwYPnPRSYVvZL+v kmxwc2n9guDqf1/axt8xOYz3IVNDiDcVDNqEhftWLK25FJMdvaKX98Kvna8Vv1iQ ZQJDL/oCHqoHmsAMtA0QPr2TJHKMr7lDO7hl19U3/n1rfGm44SEeizFY1XEguHJE x3Jrj7eO2503htiHi3fYkkvBVgcNMW7BNV+8ldxp6hyoruQ7SdO0cwQdj/4vp3iy jLfctYkMQdhSK6bqR9JChXIvl1rAYTnmsqocRsB14JmYwsd/JAeMghnlL2NBCkWR G4stRjSrBI5VMmFvFktLeauqYrTttH/3PmvPIuC6zAJczQc6k7dhjBYiVv6jN/DG B1Vs57eEBvnR3oLfeccDHE5hL6zf9y9JMXs9wcu5E7mvTBcfIfM= =rXCy -----END PGP SIGNATURE----- --vn3EagKx/0DoaBN5--