Re: [PATCH nft] evaluate: reject negative values for unsigned datatypes

On Thu, Aug 13, 2026 at 01:21:53PM +0200, Phil Sutter wrote:
> On Sat, Aug 08, 2026 at 07:03:18AM +0530, Avinash Duduskar wrote:
> > expr_evaluate_integer() only tests the upper bound, so a negative value
> > passes the range check and mpz_export() then drops the sign:
> > 
> >   # nft add element ip t m { "-1" }
> >   # nft list set ip t m
> >   table ip t {
> >           set m {
> >                   type mark
> >                   elements = { 0x00000001 }
> >           }
> >   }
> > 
> > The error string has read "Value %s exceeds valid range 0-%s" since the
> > check was added, so the contract was already unsigned; only the upper
> > half of it was enforced. The result is not a wrap either: "-1" gives 1
> > while "-4294967295" gives 0xffffffff.
> > 
> > Reject a negative value, unless the evaluation context is a chain
> > priority, which is signed.
> > 
> > Fixes: cb7cb885d65e ("evaluate: add expr_evaluate_integer()")
> > Suggested-by: Pablo Neira Ayuso <[email protected]>
> > Signed-off-by: Avinash Duduskar <[email protected]>
> > ---
> >  src/evaluate.c                                | 12 +++++
> >  tests/py/any/meta.t                           |  2 +
> >  .../parsing/dumps/negative_values_0.nodump    |  0
> >  .../shell/testcases/parsing/negative_values_0 | 46 +++++++++++++++++++
> >  4 files changed, 60 insertions(+)
> >  create mode 100644 tests/shell/testcases/parsing/dumps/negative_values_0.nodump
> >  create mode 100755 tests/shell/testcases/parsing/negative_values_0
> > 
> > diff --git a/src/evaluate.c b/src/evaluate.c
> > index 8bb7b609..f5b88c0b 100644
> > --- a/src/evaluate.c
> > +++ b/src/evaluate.c
> > @@ -447,6 +447,18 @@ static int expr_evaluate_integer(struct eval_ctx *ctx, struct expr **exprp)
> >  		return -1;
> >  	}
> >  
> > +	/* chain priorities are signed, everything else is an unsigned key:
> > +	 * mpz_export() drops the sign, so "-1" would silently become 1.
> > +	 */
> > +	if (mpz_sgn(expr->value) < 0 && ctx->ectx.dtype != &priority_type) {
> 
> Although it might remain the only case, I don't like the special-casing
> here. Looking at struct datatype, we have a 32bit field for flags with
> only two bits used yet. Maybe introduce an 'is_signed' bit to check
> here?

Yes, I wonder if we can do this in a more generic way, like specifying
in the datatype itself the min and maximum value expected from the
integer. I don't expect "signed 128-bit only". Maybe add a new
interface to struct datatype to validate the value is within the
boundaries? Maybe something like:

 if (ctx->ectx.dtype && ctx->ectx.dtype->validate) {
        erec = ctx->ectx.dtype->validate(expr->value));
        if (erec)
                return -1;
 }