Re: [nft PATCH] Reject invalid chain priority values in user space

[Phil Sutter <phil@nwl.cc>]

On Fri, Mar 10, 2023 at 10:12:36AM +0100, Pablo Neira Ayuso wrote:
> On Fri, Mar 10, 2023 at 01:13:14AM +0100, Phil Sutter wrote:
> > The kernel doesn't accept nat type chains with a priority of -200 or
> > below. Catch this and provide a better error message than the kernel's
> > EOPNOTSUPP.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  src/evaluate.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/src/evaluate.c b/src/evaluate.c
> > index d24f8b66b0de8..af4844c1ef6cc 100644
> > --- a/src/evaluate.c
> > +++ b/src/evaluate.c
> > @@ -4842,6 +4842,8 @@ static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
> >  	}
> >  
> >  	if (chain->flags & CHAIN_F_BASECHAIN) {
> > +		int priority;
> > +
> >  		chain->hook.num = str2hooknum(chain->handle.family,
> >  					      chain->hook.name);
> >  		if (chain->hook.num == NF_INET_NUMHOOKS)
> > @@ -4854,6 +4856,14 @@ static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
> >  			return __stmt_binary_error(ctx, &chain->priority.loc, NULL,
> >  						   "invalid priority expression %s in this context.",
> >  						   expr_name(chain->priority.expr));
> > +
> 
> maybe get this here to declutter the branch?
> 
>                 mpz_export_data(&priority, chain->priority.expr->value,
>                                 BYTEORDER_HOST_ENDIAN, sizeof(int)));

Will do. Initially I wanted to use mpz_get_si() but it expects a larger
data size. It even works if one casts the return value to int, but I
guess it won't anymore on Big Endian (and is a hack anyway).

> this is in basechain context, so it should be fine.

Yes, indeed. Also the call to evaluate_priority() ensures
chain->priority.expr is as expected.

> > +		if (!strcmp(chain->type.str, "nat") &&
> > +		    (mpz_export_data(&priority, chain->priority.expr->value,
> > +				    BYTEORDER_HOST_ENDIAN, sizeof(int))) &&
> > +		    priority <= -200)
> > +			return __stmt_binary_error(ctx, &chain->priority.loc, NULL,
> > +						   "Nat type chains must have a priority value above -200.");
>                                                     ^^^
> 
> I'd suggest lower case 'nat' which is what the user specifies in the
> chain declaration.

I'll rewrite the sentence.

> Thanks for addressing my feedback.

Thanks for the quick review!