Minor improvements to IPv4 netmask handling

There are several minor problems with our parsing of IPv4 netmasks (-n).

First, we don't reject nonsensical netmasks like 0.255.0.255.  Address this
structurally by using prefix length instead of netmask as the primary
variable, only converting (and validating) when we need to.  This has the
added benefit of making some things more uniform with the IPv6 path.

Second, when the user specifies a prefix length, we truncate the output
from strtol() to an integer, which means we would treat -n 4294967320 as
valid (equivalent to 24).  Fix types to check for this.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

4 files changed, 47 insertions, 33 deletions

diff --git a/conf.c b/conf.c
index 4a622fc..c6d06ea 100644
--- a/conf.c
+++ b/conf.c
@@ -522,6 +522,31 @@ static int conf_pasta_ns(int *netns_only, char *userns, char *netns,
 	return 0;
 }
 
+/** conf_ip4_prefix() - Parse an IPv4 prefix length or netmask
+ * @arg:	Netmask in dotted decimal or prefix length
+ *
+ * Return: Validated prefix length on success, -1 on failure
+ */
+static int conf_ip4_prefix(const char *arg)
+{
+	struct in_addr mask;
+	unsigned long len;
+
+	if (inet_pton(AF_INET, arg, &mask)) {
+		in_addr_t hmask = ntohl(mask.s_addr);
+		len = __builtin_popcount(hmask);
+		if ((hmask << len) != 0)
+			return -1;
+	} else {
+		errno = 0;
+		len = strtoul(optarg, NULL, 0);
+		if (len > 32 || errno)
+			return -1;
+	}
+
+	return len;
+}
+
 /**
  * conf_ip4() - Verify or detect IPv4 support, get relevant addresses
  * @ifi:	Host interface to attempt (0 to determine one)
@@ -544,22 +569,18 @@ static unsigned int conf_ip4(unsigned int ifi,
 	if (!ip4->gw)
 		nl_route(0, ifi, AF_INET, &ip4->gw);
 
-	if (!ip4->addr) {
-		int mask_len = 0;
+	if (!ip4->addr)
+		nl_addr(0, ifi, AF_INET, &ip4->addr, &ip4->prefix_len, NULL);
 
-		nl_addr(0, ifi, AF_INET, &ip4->addr, &mask_len, NULL);
-		ip4->mask = htonl(0xffffffff << (32 - mask_len));
-	}
-
-	if (!ip4->mask) {
+	if (!ip4->prefix_len) {
 		if (IN_CLASSA(ntohl(ip4->addr)))
-			ip4->mask = htonl(IN_CLASSA_NET);
+			ip4->prefix_len = (32 - IN_CLASSA_NSHIFT);
 		else if (IN_CLASSB(ntohl(ip4->addr)))
-			ip4->mask = htonl(IN_CLASSB_NET);
+			ip4->prefix_len = (32 - IN_CLASSB_NSHIFT);
 		else if (IN_CLASSC(ntohl(ip4->addr)))
-			ip4->mask = htonl(IN_CLASSC_NET);
+			ip4->prefix_len = (32 - IN_CLASSC_NSHIFT);
 		else
-			ip4->mask = 0xffffffff;
+			ip4->prefix_len = 32;
 	}
 
 	memcpy(&ip4->addr_seen, &ip4->addr, sizeof(ip4->addr_seen));
@@ -819,11 +840,12 @@ static void conf_print(const struct ctx *c)
 
 	if (c->ifi4) {
 		if (!c->no_dhcp) {
+			uint32_t mask = htonl(0xffffffff << c->ip4.prefix_len);
 			info("DHCP:");
 			info("    assign: %s",
 			     inet_ntop(AF_INET, &c->ip4.addr, buf4, sizeof(buf4)));
 			info("    mask: %s",
-			     inet_ntop(AF_INET, &c->ip4.mask, buf4, sizeof(buf4)));
+			     inet_ntop(AF_INET, &mask,        buf4, sizeof(buf4)));
 			info("    router: %s",
 			     inet_ntop(AF_INET, &c->ip4.gw,   buf4, sizeof(buf4)));
 		}
@@ -1067,9 +1089,9 @@ void conf(struct ctx *c, int argc, char **argv)
 	struct in6_addr *dns6 = c->ip6.dns;
 	struct fqdn *dnss = c->dns_search;
 	uint32_t *dns4 = c->ip4.dns;
-	int name, ret, mask, b, i;
 	const char *optstring;
 	unsigned int ifi = 0;
+	int name, ret, b, i;
 	size_t logsize = 0;
 	uid_t uid;
 	gid_t gid;
@@ -1374,18 +1396,11 @@ void conf(struct ctx *c, int argc, char **argv)
 			usage(argv[0]);
 			break;
 		case 'n':
-			if (inet_pton(AF_INET, optarg, &c->ip4.mask))
-				break;
-
-			errno = 0;
-			mask = strtol(optarg, NULL, 0);
-			if (mask > 0 && mask <= 32 && !errno) {
-				c->ip4.mask = htonl(0xffffffff << (32 - mask));
-				break;
+			c->ip4.prefix_len = conf_ip4_prefix(optarg);
+			if (c->ip4.prefix_len < 0) {
+				err("Invalid netmask: %s", optarg);
+				usage(argv[0]);
 			}
-
-			err("Invalid netmask: %s", optarg);
-			usage(argv[0]);
 			break;
 		case 'M':
 			for (i = 0; i < ETH_ALEN; i++) {
diff --git a/dhcp.c b/dhcp.c
index d22698a..076e9b6 100644
--- a/dhcp.c
+++ b/dhcp.c
@@ -268,6 +268,7 @@ static void opt_set_dns_search(const struct ctx *c, size_t max_len)
 int dhcp(const struct ctx *c, const struct pool *p)
 {
 	size_t mlen, len, offset = 0, opt_len, opt_off = 0;
+	struct in_addr mask;
 	struct ethhdr *eh;
 	struct iphdr *iph;
 	struct udphdr *uh;
@@ -334,14 +335,15 @@ int dhcp(const struct ctx *c, const struct pool *p)
 	     m->chaddr[3], m->chaddr[4], m->chaddr[5]);
 
 	m->yiaddr = c->ip4.addr;
-	memcpy(opts[1].s,  &c->ip4.mask, sizeof(c->ip4.mask));
+	mask.s_addr = htonl(0xffffffff << c->ip4.prefix_len);
+	memcpy(opts[1].s,  &mask,        sizeof(mask));
 	memcpy(opts[3].s,  &c->ip4.gw,   sizeof(c->ip4.gw));
 	memcpy(opts[54].s, &c->ip4.gw,   sizeof(c->ip4.gw));
 
 	/* If the gateway is not on the assigned subnet, send an option 121
 	 * (Classless Static Routing) adding a dummy route to it.
 	 */
-	if ((c->ip4.addr & c->ip4.mask) != (c->ip4.gw & c->ip4.mask)) {
+	if ((c->ip4.addr & mask.s_addr) != (c->ip4.gw & mask.s_addr)) {
 		/* a.b.c.d/32:0.0.0.0, 0:a.b.c.d */
 		opts[121].slen = 14;
 		opts[121].s[0] = 32;
diff --git a/passt.h b/passt.h
index 67281db..4cf6078 100644
--- a/passt.h
+++ b/passt.h
@@ -99,7 +99,7 @@ enum passt_modes {
  * struct ip4_ctx - IPv4 execution context
  * @addr:		IPv4 address for external, routable interface
  * @addr_seen:		Latest IPv4 address seen as source from tap
- * @mask:		IPv4 netmask, network order
+ * @prefixlen:		IPv4 prefix length (netmask)
  * @gw:			Default IPv4 gateway, network order
  * @dns:		IPv4 DNS addresses, zero-terminated, network order
  * @dns_fwd:		Address forwarded (UDP) to first IPv4 DNS, network order
@@ -107,7 +107,7 @@ enum passt_modes {
 struct ip4_ctx {
 	uint32_t addr;
 	uint32_t addr_seen;
-	uint32_t mask;
+	int prefix_len;
 	uint32_t gw;
 	uint32_t dns[MAXNS + 1];
 	uint32_t dns_fwd;
diff --git a/pasta.c b/pasta.c
index ae695e2..db86317 100644
--- a/pasta.c
+++ b/pasta.c
@@ -249,19 +249,16 @@ void pasta_ns_conf(struct ctx *c)
 	nl_link(1, 1 /* lo */, MAC_ZERO, 1, 0);
 
 	if (c->pasta_conf_ns) {
-		int prefix_len;
-
 		nl_link(1, c->pasta_ifi, c->mac_guest, 1, c->mtu);
 
 		if (c->ifi4) {
-			prefix_len = __builtin_popcount(c->ip4.mask);
 			nl_addr(1, c->pasta_ifi, AF_INET, &c->ip4.addr,
-				&prefix_len, NULL);
+				&c->ip4.prefix_len, NULL);
 			nl_route(1, c->pasta_ifi, AF_INET, &c->ip4.gw);
 		}
 
 		if (c->ifi6) {
-			prefix_len = 64;
+			int prefix_len = 64;
 			nl_addr(1, c->pasta_ifi, AF_INET6, &c->ip6.addr,
 				&prefix_len, NULL);
 			nl_route(1, c->pasta_ifi, AF_INET6, &c->ip6.gw);