Re: [PATCH nf-next] netfilter: nft_ct: validate hook and priority for ct zone set

[Florian Westphal <fw@strlen.de>]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Fixes: edee4f1e9245 ("netfilter: nft_ct: add zone id set support")
> > Assisted-by: LLM
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> >  This isn't nice/ideal, because this causes a test failure in
> >  tests/py (already resolved in master branch now).
> >  So, there is a risk of breakage.
> >  However, attaching the zone template after conntrack
> >  makes no sense at all and is actually harmful.
> 
> Would you consider simply turning this into noop if called used from
> the wrong hook if that is possible? Maybe that is a small patch?

Don't think its a smaller patch.  Unless I' missing something.
nf_hook_state only carries the hook location, not the priority.

AFAICS it would be a larger change. I'd have to:

> >  #ifdef CONFIG_NF_CONNTRACK_ZONES
> > +static int nft_ct_set_zone_validate(const struct nft_ctx *ctx,
> > +				    const struct nft_expr *expr)
> > +{
> > +	unsigned int hooks;
> > +	int err;
> > +
> > +	/* Setting the zone template only makes sense before conntrack has
> > +	 * assigned a zone to the packet, i.e. in prerouting and output,
> > +	 * and only if this rule runs before the conntrack hook itself.
> > +	 * Locally originated bridge traffic is conntrack'd by the IP stack,
> > +	 * not on the bridge, so only prerouting applies there.
> > +	 */
> > +	switch (ctx->family) {
> > +	case NFPROTO_IPV4:
> > +	case NFPROTO_IPV6:
> > +	case NFPROTO_INET:
> > +		hooks = (1 << NF_INET_PRE_ROUTING) | (1 << NF_INET_LOCAL_OUT);
> > +		break;
> > +	case NFPROTO_BRIDGE:
> > +		hooks = (1 << NF_INET_PRE_ROUTING);
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	err = nft_chain_validate_hooks(ctx->chain, hooks);
> > +	if (err)
> > +		return err;
> > +
> > +	if (nft_is_base_chain(ctx->chain)) {
> > +		const struct nft_base_chain *basechain = nft_base_chain(ctx->chain);
> > +
> > +		if (basechain->type->type == NFT_CHAIN_T_NAT ||
> > +		    basechain->ops.priority >= NF_IP_PRI_CONNTRACK)
> > +			return -EOPNOTSUPP;

Change all the errs here to remove the 'const', then toggle expr->priv
to say 'do not do anything'.

I'd also have to alter nf_tables_api.c so that it treats the validation
as soft-fail and not cache the successful result if the hook or prio
was bad.

At that point I wonder if it would make more sense to explore a
'ditch this expression, pretend it never happened'...