Re: [PATCH nf-next 05/13] netfilter: nft_nat: add support for shifted port-ranges

[Florian Westphal <fw@xxxxxxxxx>]

Jeremy Sowden <jeremy@xxxxxxxxxx> wrote:
> index 5c29915ab028..0517a3efb259 100644
> --- a/net/netfilter/nft_nat.c
> +++ b/net/netfilter/nft_nat.c
> @@ -25,6 +25,7 @@ struct nft_nat {
>  	u8			sreg_addr_max;
>  	u8			sreg_proto_min;
>  	u8			sreg_proto_max;
> +	u8			sreg_proto_base;
>  	enum nf_nat_manip_type  type:8;
>  	u8			family;
>  	u16			flags;
> @@ -58,6 +59,8 @@ static void nft_nat_setup_proto(struct nf_nat_range2 *range,
>  		nft_reg_load16(&regs->data[priv->sreg_proto_min]);
>  	range->max_proto.all = (__force __be16)
>  		nft_reg_load16(&regs->data[priv->sreg_proto_max]);
> +	range->base_proto.all = (__force __be16)
> +		nft_reg_load16(&regs->data[priv->sreg_proto_base]);

Hmmm!  See below.

> -	plen = sizeof_field(struct nf_nat_range, min_proto.all);
> +	plen = sizeof_field(struct nf_nat_range2, min_proto.all);
>  	if (tb[NFTA_NAT_REG_PROTO_MIN]) {
>  		err = nft_parse_register_load(tb[NFTA_NAT_REG_PROTO_MIN],
>  					      &priv->sreg_proto_min, plen);
> @@ -239,6 +243,16 @@ static int nft_nat_init(const struct nft_ctx *ctx, const struct nft_expr *expr,
>  						      plen);
>  			if (err < 0)
>  				return err;
> +
> +			if (tb[NFTA_NAT_REG_PROTO_BASE]) {
> +				err = nft_parse_register_load
> +					(tb[NFTA_NAT_REG_PROTO_BASE],
> +					 &priv->sreg_proto_base, plen);
> +				if (err < 0)
> +					return err;
> +
> +				priv->flags |= NF_NAT_RANGE_PROTO_OFFSET;

So sreg_proto_base is only set if tb[NFTA_NAT_REG_PROTO_BASE] gets
passed.

So, I would expect that all accesses to priv->sreg_proto_base are
guarded with a 'if (priv->sreg_proto_base)' check.

> @@ -286,7 +300,9 @@ static int nft_nat_dump(struct sk_buff *skb,
>  		if (nft_dump_register(skb, NFTA_NAT_REG_PROTO_MIN,
>  				      priv->sreg_proto_min) ||
>  		    nft_dump_register(skb, NFTA_NAT_REG_PROTO_MAX,
> -				      priv->sreg_proto_max))
> +				      priv->sreg_proto_max) ||
> +		    nft_dump_register(skb, NFTA_NAT_REG_PROTO_BASE,
> +				      priv->sreg_proto_base))

sreg_proto_min/max are only dumped when set, so NFTA_NAT_REG_PROTO_BASE
should not be dumped unconditionally either?