Re: [PATCH v6 01/18] conf, fwd: Stricter rule checking in fwd_rule_add()

[Laurent Vivier <lvivier@redhat.com>]

On 5/3/26 23:55, Stefano Brivio wrote:
> From: David Gibson <david@gibson.dropbear.id.au>
> 
> Although fwd_rule_add() performs some sanity checks on the rule it is
> given, there are invalid rules we don't check for, assuming that its
> callers will do that.
> 
> That won't be enough when we can get rules inserted by a dynamic update
> client without going through the existing parsing code.  So, add stricter
> checks to fwd_rule_add(), which is now possible thanks to the capabilities
> bits in the struct fwd_table.  Where those duplicate existing checks in the
> callers, remove the old copies.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

> ---
>   conf.c | 21 ---------------------
>   fwd.c  | 52 +++++++++++++++++++++++++++++++++++++++++++++++-----
>   2 files changed, 47 insertions(+), 26 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 6e884e5..b470b0d 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -176,8 +176,6 @@ static void conf_ports_range_except(struct fwd_table *fwd, uint8_t proto,
>   			die("Invalid interface name: %s", ifname);
>   	}
>   
> -	assert(first != 0);
> -
>   	for (base = first; base <= last; base++) {
>   		if (exclude && bitmap_isset(exclude, base))
>   			continue;
> @@ -310,10 +308,6 @@ static void conf_ports_spec(struct fwd_table *fwd, uint8_t proto,
>   		if (p != ep) /* Garbage after the ranges */
>   			goto bad;
>   
> -		if (orig_range.first == 0) {
> -			die("Can't forward port 0 included in '%s'", spec);
> -		}
> -
>   		conf_ports_range_except(fwd, proto, addr, ifname,
>   					orig_range.first, orig_range.last,
>   					exclude,
> @@ -356,11 +350,6 @@ static void conf_ports(char optname, const char *optarg, struct fwd_table *fwd)
>   		return;
>   	}
>   
> -	if (proto == IPPROTO_TCP && !(fwd->caps & FWD_CAP_TCP))
> -		die("TCP port forwarding requested but TCP is disabled");
> -	if (proto == IPPROTO_UDP && !(fwd->caps & FWD_CAP_UDP))
> -		die("UDP port forwarding requested but UDP is disabled");
> -
>   	strncpy(buf, optarg, sizeof(buf) - 1);
>   
>   	if ((spec = strchr(buf, '/'))) {
> @@ -405,16 +394,6 @@ static void conf_ports(char optname, const char *optarg, struct fwd_table *fwd)
>   		addr = NULL;
>   	}
>   
> -	if (addr) {
> -		if (!(fwd->caps & FWD_CAP_IPV4) && inany_v4(addr)) {
> -			die("IPv4 is disabled, can't use -%c %s",
> -			    optname, optarg);
> -		} else if (!(fwd->caps & FWD_CAP_IPV6) && !inany_v4(addr)) {
> -			die("IPv6 is disabled, can't use -%c %s",
> -			    optname, optarg);
> -		}
> -	}
> -
>   	if (optname == 'T' || optname == 'U') {
>   		assert(!addr && !ifname);
>   
> diff --git a/fwd.c b/fwd.c
> index c7fd1a9..979c149 100644
> --- a/fwd.c
> +++ b/fwd.c
> @@ -367,17 +367,59 @@ int fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new)
>   		     new->first, new->last);
>   		return -EINVAL;
>   	}
> +	if (!new->first) {
> +		warn("Forwarding rule attempts to map from port 0");
> +		return -EINVAL;
> +	}
> +	if (!new->to ||
> +	    (in_port_t)(new->to + new->last - new->first) < new->to) {
> +		warn("Forwarding rule attempts to map to port 0");
> +		return -EINVAL;
> +	}
>   	if (new->flags & ~allowed_flags) {
>   		warn("Rule has invalid flags 0x%hhx",
>   		     new->flags & ~allowed_flags);
>   		return -EINVAL;
>   	}
> -	if (new->flags & FWD_DUAL_STACK_ANY &&
> -	    !inany_equals(&new->addr, &inany_any6)) {
> -		char astr[INANY_ADDRSTRLEN];
> +	if (new->flags & FWD_DUAL_STACK_ANY) {
> +		if (!inany_equals(&new->addr, &inany_any6)) {
> +			char astr[INANY_ADDRSTRLEN];
>   
> -		warn("Dual stack rule has non-wildcard address %s",
> -		     inany_ntop(&new->addr, astr, sizeof(astr)));
> +			warn("Dual stack rule has non-wildcard address %s",
> +			     inany_ntop(&new->addr, astr, sizeof(astr)));
> +			return -EINVAL;
> +		}
> +		if (!(fwd->caps & FWD_CAP_IPV4)) {
> +			warn("Dual stack forward, but IPv4 not enabled");
> +			return -EINVAL;
> +		}
> +		if (!(fwd->caps & FWD_CAP_IPV6)) {
> +			warn("Dual stack forward, but IPv6 not enabled");
> +			return -EINVAL;
> +		}
> +	} else {
> +		if (inany_v4(&new->addr) && !(fwd->caps & FWD_CAP_IPV4)) {
> +			warn("IPv4 forward, but IPv4 not enabled");
> +			return -EINVAL;
> +		}
> +		if (!inany_v4(&new->addr) && !(fwd->caps & FWD_CAP_IPV6)) {
> +			warn("IPv6 forward, but IPv6 not enabled");
> +			return -EINVAL;
> +		}
> +	}
> +	if (new->proto == IPPROTO_TCP) {
> +		if (!(fwd->caps & FWD_CAP_TCP)) {
> +			warn("Can't add TCP forwarding rule, TCP not enabled");
> +			return -EINVAL;
> +		}
> +	} else if (new->proto == IPPROTO_UDP) {
> +		if (!(fwd->caps & FWD_CAP_UDP)) {
> +			warn("Can't add UDP forwarding rule, UDP not enabled");
> +			return -EINVAL;
> +		}
> +	} else {
> +		warn("Unsupported protocol 0x%hhx (%s) for forwarding rule",
> +		     new->proto, ipproto_name(new->proto));
>   		return -EINVAL;
>   	}
>