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

[Jeremy Sowden <jeremy@xxxxxxxxxx>]

On 2023-03-07, at 13:27:51 +0100, Florian Westphal wrote:
> 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?
> 

Agreed.  Will fix.

J.

Attachment: signature.asc
Description: PGP signature