Re: [PATCH v3 07/11] conf, fwd: Stricter rule checking in fwd_rule_add()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5931 bytes --]

On Mon, Apr 20, 2026 at 06:48:48PM +0200, Laurent Vivier wrote:
> On 4/17/26 07:05, David Gibson wrote:
> > 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>
> > ---
> >   conf.c | 19 -------------------
> >   fwd.c  | 51 ++++++++++++++++++++++++++++++++++++++++++++++-----
> >   2 files changed, 46 insertions(+), 24 deletions(-)
> > 
> > diff --git a/conf.c b/conf.c
> > index ecc3a342..3b373b22 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -310,10 +310,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);
> > -		}
> > -
> 
> We remove the die() here but we keep the "assert(first != 0)" in
> conf_ports_range_except(), so the user can trigger it with "-t 0" before the
> call to fwd_rule_add().

Oops.  Fixed.

> 
> 
> >   		conf_ports_range_except(fwd, proto, addr, ifname,
> >   					orig_range.first, orig_range.last,
> >   					exclude,
> > @@ -356,11 +352,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 +396,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 c7fd1a9d..aa966731 100644
> > --- a/fwd.c
> > +++ b/fwd.c
> > @@ -367,17 +367,58 @@ 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 || (new->to + new->last - new->first) < new->to) {
> 
> Why do we need the second part?

To check for the case where we have a (valid) source range of ports
mapped to a target range starting high enough that there's no room, so
it wraps around covering port 0

> We know new->first < new->last and this cannot overflow as values are
> uint16_t and arithmetic uses int.

Bother, I always forget that minimum promotion rule.  I've added a
cast so it *will* overflow in the case I'm looking for.

> 
> FWIW:
> (gdb) print (unsigned short)65535
> $1 = 65535
> (gdb) print (unsigned short)65536
> $2 = 0
> (gdb) print (unsigned short)65535 + (unsigned short)1
> $3 = 65536
> 
> 
> > +		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;
> >   	}
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]