Re: [PATCH nft 2/2] evaluate: restrict allowed subtypes of concatenations

[Florian Westphal <fw@strlen.de>]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > diff --git a/src/evaluate.c b/src/evaluate.c
> > index d099be137cb3..0c8af09492d1 100644
> > --- a/src/evaluate.c
> > +++ b/src/evaluate.c
> [...]
> > @@ -1704,10 +1706,48 @@ static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr)
> >  		if (list_member_evaluate(ctx, &i) < 0)
> >  			return -1;
> >  
> > -		if (i->etype == EXPR_SET)
> > +		switch (i->etype) {
> > +		case EXPR_VALUE:
> > +		case EXPR_UNARY:
> > +		case EXPR_BINOP:
> > +		case EXPR_RELATIONAL:
> > +		case EXPR_CONCAT:
> > +		case EXPR_MAP:
> > +		case EXPR_PAYLOAD:
> > +		case EXPR_EXTHDR:
> > +		case EXPR_META:
> > +		case EXPR_RT:
> > +		case EXPR_CT:
> > +		case EXPR_SET_ELEM:
> > +		case EXPR_NUMGEN:
> > +		case EXPR_HASH:
> > +		case EXPR_FIB:
> > +		case EXPR_SOCKET:
> > +		case EXPR_OSF:
> > +		case EXPR_XFRM:
> 
> I am expecting more new selector expressions here that would need to
> be added and I think it is less likely to see new constant expressions
> in the future, so maybe reverse this logic ...
> 
> 		if (i->etype == EXPR_RANGE ||
>                     i->etype == EXPR_PREFIX) {
> 			/* allowed on RHS (e.g. th dport . mark { 1-65535 . 42 }
> 			 *                                       ~~~~~~~~ allowed
> 			 * but not on LHS (e.g  1-4 . mark { ...}
> 			 *                      ~~~ illegal
>                         ...
> 
> ... and let anything else be accepted?

I prefer "accept whats safe and reject rest" but I can invert
if you want.

> > +			 * EXPR_SET_ELEM (is used as RHS).
> > +			 */
> > +			if (ctx->recursion.list > 0)
> > +				break;
> 
> So recursion.list is used to provide context to identify this is rhs,
> correct?

Yes.

> Is your intention is to use this recursion.list to control to
> deeper recursions in a follow up patch?

No, what did you have in mind?

I could see adding new members to ctx->recursion to control other
possible recursions in addition to what we have now.

But I don't see other uses for .list at this time.

> Not related, but if goal is to provide context then I also need more
> explicit context hints for bitfield payload and bitwise expressions
> where the evaluation needs to be different depending on where the
> expression is located (not the same if the expression is either used
> as selector or as lhs/rhs of assignment).
> 
> I don't know yet how such new context enum to modify evaluation
> behaviour will look, so we can just use recursion.list by now, I don't
> want to block this fix.

OK.  Yes, it would also work if there was some different "where am I"
indicator, e.g. if (ctx->expr_side == CTX_EXPR_LHS) or whatever.

This fix isn't urgent, we can keep it back and come back to this
if you prefer to first work on the ctx hint extensions.