In our arrays of DNS resolvers to pass to the guest we use a blank entry
to indicate the end of the list. We rely on this when scanning the array,
not having separate bounds checking. clang-tidy 21.1.7 has fancier
checking for array overruns in loops, but it's not able to reason that
there's always a terminating entry, so complains.
Indeed, it's correct to do so in this case. Although we allow space in the
arrays for the terminator (size MAXNS + 1), add_dns[46]() check only for
idx >= ARRAY_SIZE()
before adding an entry. This allows it to consume the last slot with a
"real" entry, meaning the places where we scan really could overrun.
Fix the bug, and make it easier to reason about (for both clang-tidy and
people) by using ARRAY_SIZE() base bounds checking. Treat the terminator
explicitly as an early exit case using 'break'.
Signed-off-by: David Gibson
---
conf.c | 8 ++++++--
dhcp.c | 4 +++-
dhcpv6.c | 4 +++-
ndp.c | 4 +++-
passt.h | 4 ++--
5 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/conf.c b/conf.c
index 84ae12b2..ceb9aa55 100644
--- a/conf.c
+++ b/conf.c
@@ -1159,7 +1159,9 @@ static void conf_print(const struct ctx *c)
buf4, sizeof(buf4)));
}
- for (i = 0; !IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns[i]); i++) {
+ for (i = 0; i < ARRAY_SIZE(c->ip4.dns); i++) {
+ if (IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns[i]))
+ break;
if (!i)
info("DNS:");
inet_ntop(AF_INET, &c->ip4.dns[i], buf4, sizeof(buf4));
@@ -1197,7 +1199,9 @@ static void conf_print(const struct ctx *c)
buf6, sizeof(buf6)));
dns6:
- for (i = 0; !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[i]); i++) {
+ for (i = 0; i < ARRAY_SIZE(c->ip6.dns); i++) {
+ if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[i]))
+ break;
if (!i)
info("DNS:");
inet_ntop(AF_INET6, &c->ip6.dns[i], buf6, sizeof(buf6));
diff --git a/dhcp.c b/dhcp.c
index 6b9c2e3b..1ff8cba9 100644
--- a/dhcp.c
+++ b/dhcp.c
@@ -430,7 +430,9 @@ int dhcp(const struct ctx *c, struct iov_tail *data)
}
for (i = 0, opts[6].slen = 0;
- !c->no_dhcp_dns && !IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns[i]); i++) {
+ !c->no_dhcp_dns && i < ARRAY_SIZE(c->ip4.dns); i++) {
+ if (IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns[i]))
+ break;
((struct in_addr *)opts[6].s)[i] = c->ip4.dns[i];
opts[6].slen += sizeof(uint32_t);
}
diff --git a/dhcpv6.c b/dhcpv6.c
index e4df0db5..d94be23a 100644
--- a/dhcpv6.c
+++ b/dhcpv6.c
@@ -425,7 +425,9 @@ static size_t dhcpv6_dns_fill(const struct ctx *c, char *buf, int offset)
if (c->no_dhcp_dns)
goto search;
- for (i = 0; !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[i]); i++) {
+ for (i = 0; i < ARRAY_SIZE(c->ip6.dns); i++) {
+ if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[i]))
+ break;
if (!i) {
srv = (struct opt_dns_servers *)(buf + offset);
offset += sizeof(struct opt_hdr);
diff --git a/ndp.c b/ndp.c
index eb9e3139..1f2bcb0c 100644
--- a/ndp.c
+++ b/ndp.c
@@ -285,7 +285,9 @@ static void ndp_ra(const struct ctx *c, const struct in6_addr *dst)
size_t dns_s_len = 0;
int i, n;
- for (n = 0; !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[n]); n++);
+ for (n = 0; n < ARRAY_SIZE(c->ip6.dns); n++)
+ if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[n]))
+ break;
if (n) {
struct opt_rdnss *rdnss = (struct opt_rdnss *)ptr;
*rdnss = (struct opt_rdnss) {
diff --git a/passt.h b/passt.h
index 79d01ddb..87da76d3 100644
--- a/passt.h
+++ b/passt.h
@@ -91,7 +91,7 @@ struct ip4_ctx {
struct in_addr guest_gw;
struct in_addr map_host_loopback;
struct in_addr map_guest_addr;
- struct in_addr dns[MAXNS + 1];
+ struct in_addr dns[MAXNS];
struct in_addr dns_match;
struct in_addr our_tap_addr;
@@ -132,7 +132,7 @@ struct ip6_ctx {
struct in6_addr guest_gw;
struct in6_addr map_host_loopback;
struct in6_addr map_guest_addr;
- struct in6_addr dns[MAXNS + 1];
+ struct in6_addr dns[MAXNS];
struct in6_addr dns_match;
struct in6_addr our_tap_ll;
--
2.52.0