From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=MCkwW92/; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTPS id 245D05A0265 for ; Wed, 06 May 2026 09:46:51 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1778053610; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=MGr9n5hO7jE6kYwg39R3UZOWq77D2+t43mms1F8Z+Ic=; b=MCkwW92/Wpw7CCPavZw3gpg1RRE9BX1WvW+SFpFFeyxqHrswqh21AruWwwJQYsSihapmNm haNIG6H2JaP/VSQa8xrNWYvJI0l8neTCs4IoaFUavCBI1Cq5ExQLAKRDNHQyIO7UMfmj9O GdtaSLrnYgnMImXk4GE9yGyOhidxRhk= Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-390-J9_aahOLPKW8c9N4sMIl4w-1; Wed, 06 May 2026 03:46:48 -0400 X-MC-Unique: J9_aahOLPKW8c9N4sMIl4w-1 X-Mimecast-MFC-AGG-ID: J9_aahOLPKW8c9N4sMIl4w_1778053608 Received: by mail-wr1-f71.google.com with SMTP id ffacd0b85a97d-43d7730e9e3so3818418f8f.2 for ; Wed, 06 May 2026 00:46:48 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1778053607; x=1778658407; h=date:content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=MGr9n5hO7jE6kYwg39R3UZOWq77D2+t43mms1F8Z+Ic=; b=jx9ssP9XGbdqxTa7md3JP5wtAOsEbiHk8LqAB9t+qqJerUhIY6mvsjwKEaFzlZNWRP rrzgjQF/nUUS9L1vPiei79671R98ALsCBPib8KvtiqD2nV9BcU6zN2B0synDKcOwpiay znzh6+Z0unGrdZ/HRGpTXHy/9kvbKaQ1AjJOOgTRqVEQN4Qa53xGF6ygrrgn9LnsYjHv 5eoZlREaO5jufrXuUDTDqn1O5GIpqppOe6aT9YYJHuayGM0kH+gO6Glk0kp2k0rhL2Tx pUo9LVVdhDxapaHNSnZyjJspk1FhzBJayAlWV3TSGhTfpF+J/2MO6HXwbGAnCI5Tqv5L jW6g== X-Gm-Message-State: AOJu0Yw9Wu2siUQMJnZ+ioMj/WbxtftnC79EPCIuQGEamfyYKqYzJ9mY YwsBTQnU0x6gynC/x1uZoI4jARy37Jgku/v2Up909IOpf1hYimsY1zTr+371wNGsUhce2Xwfdpj XH5n0ANsSrd4GnnN8pddLaLpRq4A3S1gsyFXbS5LABjvRYYI7MkqgHwqBCgqpdA== X-Gm-Gg: AeBDieuhJy+G/VxXdCWH++22CIUviB4Tn9bCx79lCi6PZtcQKLUTgZJbmUPka78AsCv MW37usIFyOo+PBBD7iwXUNESYopfPfjYH0v9HW12bdGqD5KZAH3CKUIBGDOLo1YPYw611ZINUF1 YNweOIRHaAdwLVeAYpzLUlt5w3Hh/rn3a3qlI57wj3P4TCnGbxPFbLXNE/rYVk0mNdJAISrXosq PT4fzfuVkUJPUWj6n9N8aN9L2NRvh1hvJYJnUnKBd1Welbfr11Gdsmo04uRMBAQsJVJSvFBE1gQ nR0GXu6u8gemsEt1iNMGNQKjpiv0JmaFKXcMHFx2LhZSHdzYurA1BgCUHLv73iTKNhSuVdF6VKf G4LEuc1ZzCW7gS7kEvZ8/9n3wmpcJXM0FlzNg2XEPkHs= X-Received: by 2002:a05:6000:2509:b0:43d:733f:aee6 with SMTP id ffacd0b85a97d-4515aa98164mr4145317f8f.10.1778053606617; Wed, 06 May 2026 00:46:46 -0700 (PDT) X-Received: by 2002:a05:6000:2509:b0:43d:733f:aee6 with SMTP id ffacd0b85a97d-4515aa98164mr4145251f8f.10.1778053605865; Wed, 06 May 2026 00:46:45 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-4505238e6e0sm10232523f8f.6.2026.05.06.00.46.45 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 06 May 2026 00:46:45 -0700 (PDT) From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v7 18/18] fwd_rule: Fix static checkers warnings in fwd_rule_add() Message-ID: <20260506094644.18499890@elisabeth> In-Reply-To: References: <20260504231142.1118652-1-sbrivio@redhat.com> <20260504231142.1118652-19-sbrivio@redhat.com> <20260505121340.3a548603@elisabeth> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 Date: Wed, 06 May 2026 09:46:44 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: cb-AxXu6fHJH_i5nBOAiu5wugQq00uhEURWqOerbfms_1778053608 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: G7M4ECNDNECQPDISNFICGXQ7HZGFG534 X-Message-ID-Hash: G7M4ECNDNECQPDISNFICGXQ7HZGFG534 X-MailFrom: sbrivio@redhat.com 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: 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, 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