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

[Phil Sutter <phil@nwl.cc>]

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 <pablo@netfilter.org>
> Signed-off-by: Avinash Duduskar <avinash.duduskar@gmail.com>
> ---
>  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?

Thanks, Phil

> +		valstr = mpz_get_str(NULL, 10, expr->value);
> +		expr_error(ctx->msgs, expr,
> +			   "Value %s is negative, expecting an unsigned value",
> +			   valstr);
> +		nft_gmp_free(valstr);
> +		return -1;
> +	}
> +
>  	if (ctx->stmt_len > ctx->ectx.len)
>  		masklen = ctx->stmt_len;
>  	else
> diff --git a/tests/py/any/meta.t b/tests/py/any/meta.t
> index c5ab2ad9..4f486307 100644
> --- a/tests/py/any/meta.t
> +++ b/tests/py/any/meta.t
> @@ -56,6 +56,8 @@ meta mark and 0x03 == 0x01;ok;meta mark & 0x00000003 == 0x00000001
>  meta mark and 0x03 != 0x01;ok;meta mark & 0x00000003 != 0x00000001
>  meta mark 0x10;ok;meta mark 0x00000010
>  meta mark != 0x10;ok;meta mark != 0x00000010
> +meta mark "-1";fail
> +meta mark "-4294967295";fail
>  meta mark 0xffffff00/24;ok;meta mark & 0xffffff00 == 0xffffff00
>  
>  meta mark or 0x03 == 0x01;ok;meta mark | 0x00000003 == 0x00000001
> diff --git a/tests/shell/testcases/parsing/dumps/negative_values_0.nodump b/tests/shell/testcases/parsing/dumps/negative_values_0.nodump
> new file mode 100644
> index 00000000..e69de29b
> diff --git a/tests/shell/testcases/parsing/negative_values_0 b/tests/shell/testcases/parsing/negative_values_0
> new file mode 100755
> index 00000000..663579a0
> --- /dev/null
> +++ b/tests/shell/testcases/parsing/negative_values_0
> @@ -0,0 +1,46 @@
> +#!/bin/bash
> +
> +# mpz_export() drops the sign, so a negative value used to land as its
> +# absolute value: "-1" became 1. Chain priorities are signed and must keep
> +# working.
> +
> +set -e
> +
> +$NFT add table ip t
> +$NFT add set ip t s '{ type mark; }'
> +
> +if $NFT add element ip t s '{ "-1" }' 2>/dev/null; then
> +	echo "E: accepted a negative set element" >&2
> +	$NFT list set ip t s >&2
> +	exit 1
> +fi
> +
> +# a rejected add must not have committed anything
> +out=$($NFT list set ip t s)
> +case "$out" in
> +*elements*)
> +	echo "E: something was stored by the failed add" >&2
> +	echo "$out" >&2
> +	exit 1
> +	;;
> +esac
> +
> +$NFT add chain ip t c
> +
> +if $NFT add rule ip t c meta mark '"-1"' 2>/dev/null; then
> +	echo "E: accepted a negative value in a rule" >&2
> +	exit 1
> +fi
> +
> +# the signed exception: every spelling of a negative chain priority
> +$NFT add chain ip t c1 '{ type filter hook prerouting priority -300; }'
> +$NFT add chain ip t c2 '{ type filter hook prerouting priority filter - 10; }'
> +
> +$NFT -f - <<'NFT'
> +define p = -300
> +table ip t2 {
> +	chain c { type filter hook prerouting priority $p; policy accept; }
> +}
> +NFT
> +
> +exit 0
> 
> base-commit: 49e418238ece947e92f87a35ef6cf50755485370
> -- 
> 2.55.0
> 
>