On Fri, Jul 06, 2018 at 03:11:50PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> > On Fri, Jul 06, 2018 at 02:53:25PM +0200, Florian Westphal wrote:
> > > Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> > > > On Wed, Jul 04, 2018 at 09:13:06PM +0200, Florian Westphal wrote:
> > > > > iptables-nft never requests these, but explicitly reject this.
> > > > >
> > > > > If it were requested, kernel will oops as ->target is NULL.
> > > > >
> > > > > Signed-off-by: Florian Westphal <fw@xxxxxxxxx>
> > > > > ---
> > > > > net/netfilter/nft_compat.c | 12 ++++++++++++
> > > > > 1 file changed, 12 insertions(+)
> > > > >
> > > > > diff --git a/net/netfilter/nft_compat.c b/net/netfilter/nft_compat.c
> > > > > index 8d1ff654e5af..198cef9c2906 100644
> > > > > --- a/net/netfilter/nft_compat.c
> > > > > +++ b/net/netfilter/nft_compat.c
> > > > > @@ -832,10 +832,16 @@ nft_target_select_ops(const struct nft_ctx *ctx,
> > > > > rev = ntohl(nla_get_be32(tb[NFTA_TARGET_REV]));
> > > > > family = ctx->family;
> > > > >
> > > > > + if (strcmp(tg_name, "ERROR") == 0)
> > > > > + return ERR_PTR(-EINVAL);
> > > >
> > > > We can just reject XT_ERROR_TARGET and XT_STANDARD_TARGET here, to
> > > > keep it simple, right? So we don't use this hardcoded "ERROR".
> > >
> > > Yes, that works too.
> > > Rejecting XT_STANDARD_TARGET might be a good idea too in case someone
> > > ever changes it to have a non-null evaluation function.
> >
> > Hm, I think we're already safe, right? I mean, the check for
> > target->target to be NULL and return -EINVAL is sufficient.
> >
> > But I can just apply what I'm attaching for clarity if you prefer.
>
> Rejecting from target_init should work.
>
> I had placed everything in select_ops as that allocates the middleman
> struct and I wanted to fail early.
>
> The ebtables standard target ("standard", sigh) also lacks target eval
> function so should fail because of ->target == NULL.
>
> I'm only mentioning this because I think it should either strcmp
> for XT_ERROR_TARGET only and handle rest via taget->target == NULL check,
> or consider XT_STANDARD_TARGET, XT_ERROR_TARGET, "standard", and the
> ->target == NULL check.
>
> As its slow path, I would go for the latter option.
Would you resubmit with what you propose your patch then?
Thanks!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
