[PATCH nft] evaluate: don't eval unary arguments.

[PATCH nft] evaluate: don't eval unary arguments.

[Jeremy Sowden]

When a unary expression is inserted to implement a byte-order
conversion, the expression being converted has already been evaluated
and so expr_evaluate_unary doesn't need to do so.  For most types of
expression, the double evaluation doesn't matter since evaluation is
idempotent.  However, in the case of payload expressions which are
munged during evaluation, it can cause unexpected errors:

  # nft add table ip t
  # nft add chain ip t c '{ type filter hook input priority filter; }'
  # nft add rule ip t c ip dscp set 'ip dscp | 0x10'
  Error: Value 252 exceeds valid range 0-63
  add rule ip t c ip dscp set ip dscp | 0x10
                              ^^^^^^^

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 src/evaluate.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index e7881543d2de..9d5fdaf0ef3e 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -997,13 +997,9 @@ static int expr_evaluate_range(struct eval_ctx *ctx, struct expr **expr)
  */
 static int expr_evaluate_unary(struct eval_ctx *ctx, struct expr **expr)
 {
-	struct expr *unary = *expr, *arg;
+	struct expr *unary = *expr, *arg = unary->arg;
 	enum byteorder byteorder;
 
-	if (expr_evaluate(ctx, &unary->arg) < 0)
-		return -1;
-	arg = unary->arg;
-
 	assert(!expr_is_constant(arg));
 	assert(expr_basetype(arg)->type == TYPE_INTEGER);
 	assert(arg->etype != EXPR_UNARY);
-- 
2.24.1

Re: [PATCH nft] evaluate: don't eval unary arguments.

[Pablo Neira Ayuso]

On Sun, Jan 19, 2020 at 06:12:03PM +0000, Jeremy Sowden wrote:
> When a unary expression is inserted to implement a byte-order
> conversion, the expression being converted has already been evaluated
> and so expr_evaluate_unary doesn't need to do so.  For most types of
> expression, the double evaluation doesn't matter since evaluation is
> idempotent.  However, in the case of payload expressions which are
> munged during evaluation, it can cause unexpected errors:
> 
>   # nft add table ip t
>   # nft add chain ip t c '{ type filter hook input priority filter; }'
>   # nft add rule ip t c ip dscp set 'ip dscp | 0x10'
>   Error: Value 252 exceeds valid range 0-63
>   add rule ip t c ip dscp set ip dscp | 0x10
>                               ^^^^^^^

I'm still hitting this after applying this patch.

nft add rule ip t c ip dscp set ip dscp or 0x10
Error: Value 252 exceeds valid range 0-63
add rule ip t c ip dscp set ip dscp or 0x10
                            ^^^^^^

Probably problem is somewhere else? I'm not sure why we can assume
here that the argument of the unary expression should not be
evaluated.

Re: [PATCH nft] evaluate: don't eval unary arguments.

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 1256 bytes --]

On 2020-01-27, at 10:33:04 +0100, Pablo Neira Ayuso wrote:
> On Sun, Jan 19, 2020 at 06:12:03PM +0000, Jeremy Sowden wrote:
> > When a unary expression is inserted to implement a byte-order
> > conversion, the expression being converted has already been
> > evaluated and so expr_evaluate_unary doesn't need to do so.  For
> > most types of expression, the double evaluation doesn't matter since
> > evaluation is idempotent.  However, in the case of payload
> > expressions which are munged during evaluation, it can cause
> > unexpected errors:
> >
> >   # nft add table ip t
> >   # nft add chain ip t c '{ type filter hook input priority filter; }'
> >   # nft add rule ip t c ip dscp set 'ip dscp | 0x10'
> >   Error: Value 252 exceeds valid range 0-63
> >   add rule ip t c ip dscp set ip dscp | 0x10
> >                               ^^^^^^^
>
> I'm still hitting this after applying this patch.
>
> nft add rule ip t c ip dscp set ip dscp or 0x10
> Error: Value 252 exceeds valid range 0-63
> add rule ip t c ip dscp set ip dscp or 0x10
>                             ^^^^^^
> Probably problem is somewhere else? I'm not sure why we can assume
> here that the argument of the unary expression should not be
> evaluated.

I'll take another look.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH nft] evaluate: don't eval unary arguments.

[Pablo Neira Ayuso]

Hi Jeremy,

On Mon, Jan 27, 2020 at 11:13:43AM +0000, Jeremy Sowden wrote:
> On 2020-01-27, at 10:33:04 +0100, Pablo Neira Ayuso wrote:
> > On Sun, Jan 19, 2020 at 06:12:03PM +0000, Jeremy Sowden wrote:
> > > When a unary expression is inserted to implement a byte-order
> > > conversion, the expression being converted has already been
> > > evaluated and so expr_evaluate_unary doesn't need to do so.  For
> > > most types of expression, the double evaluation doesn't matter since
> > > evaluation is idempotent.  However, in the case of payload
> > > expressions which are munged during evaluation, it can cause
> > > unexpected errors:
> > >
> > >   # nft add table ip t
> > >   # nft add chain ip t c '{ type filter hook input priority filter; }'
> > >   # nft add rule ip t c ip dscp set 'ip dscp | 0x10'
> > >   Error: Value 252 exceeds valid range 0-63
> > >   add rule ip t c ip dscp set ip dscp | 0x10
> > >                               ^^^^^^^
> >
> > I'm still hitting this after applying this patch.
> >
> > nft add rule ip t c ip dscp set ip dscp or 0x10
> > Error: Value 252 exceeds valid range 0-63
> > add rule ip t c ip dscp set ip dscp or 0x10
> >                             ^^^^^^
> > Probably problem is somewhere else? I'm not sure why we can assume
> > here that the argument of the unary expression should not be
> > evaluated.
> 
> I'll take another look.

I think stmt_evaluate_payload() is incomplete, this function was not
made to deal with non-constant expression as values.

Look:
        tcp dport set tcp sport

works because it follows the 'easy path', ie. no adjustment to make
the checksum calculation happy (see payload_needs_adjustment() in
stmt_evaluate_payload().

However:

        ip dscp set ip dscp

bails out with:

        nft add rule ip t c ip dscp set ip dscp
        Error: Value 252 exceeds valid range 0-63
        add rule ip t c ip dscp set ip dscp
                                    ^^^^^^^

because this follows the more complicated path. Looking at this code,
this path assumes a constant value, ie. ip dscp set 10. A more complex
thing such a non-constant expression (as in the example above) will
need a bit of work.

Probably you can start making a patchset make this work:

        add rule ip t c tcp dport set tcp dport lshift 1

which triggers:

BUG: invalid binary operation 4
nft: netlink_linearize.c:592: netlink_gen_binop: Assertion `0' failed.

since it's missing the bytecode to generate the left-shift. Not very
useful for users, but we can get something already merged upstream and
you'll be half-way done. Merge also a few tests.

Then, once the more fundamental rshift/lshift bits are merged, look at
this 'harder' path. Just a proposal.

For reference, the expression tree that stmt_evaluate_payload() to
make the checksum adjustment looks like this:

           xor
          /   \
        and   value
        / \
payload_   mask
 bytes

payload_bytes extends the payload expression to get up to 16-bits.
The left hand side is there to fetch bits that need to be left
untouched. The right hand side represent the bits that need to be set.

In the new non-constant scenario, the 'value' tree is actually a
binary operation:

         shift
        /    \
   payload   imm

The unary should not really be there, it's likely related to some
incorrect byteorder issue that kicks in with non-constant expression.

So more work on stmt_evaluate_payload() is required.

Re: [PATCH nft] evaluate: don't eval unary arguments.

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 4204 bytes --]

On 2020-01-28, at 19:49:18 +0100, Pablo Neira Ayuso wrote:
> On Mon, Jan 27, 2020 at 11:13:43AM +0000, Jeremy Sowden wrote:
> > On 2020-01-27, at 10:33:04 +0100, Pablo Neira Ayuso wrote:
> > > On Sun, Jan 19, 2020 at 06:12:03PM +0000, Jeremy Sowden wrote:
> > > > When a unary expression is inserted to implement a byte-order
> > > > conversion, the expression being converted has already been
> > > > evaluated and so expr_evaluate_unary doesn't need to do so.  For
> > > > most types of expression, the double evaluation doesn't matter since
> > > > evaluation is idempotent.  However, in the case of payload
> > > > expressions which are munged during evaluation, it can cause
> > > > unexpected errors:
> > > >
> > > >   # nft add table ip t
> > > >   # nft add chain ip t c '{ type filter hook input priority filter; }'
> > > >   # nft add rule ip t c ip dscp set 'ip dscp | 0x10'
> > > >   Error: Value 252 exceeds valid range 0-63
> > > >   add rule ip t c ip dscp set ip dscp | 0x10
> > > >                               ^^^^^^^
> > >
> > > I'm still hitting this after applying this patch.
> > >
> > > nft add rule ip t c ip dscp set ip dscp or 0x10
> > > Error: Value 252 exceeds valid range 0-63
> > > add rule ip t c ip dscp set ip dscp or 0x10
> > >                             ^^^^^^
> > > Probably problem is somewhere else? I'm not sure why we can assume
> > > here that the argument of the unary expression should not be
> > > evaluated.
> >
> > I'll take another look.

I think what happened here is that I came across this problem while
working on using payload expressions to set CT and packet marks, and in
that specific case stopping the double-evaluation in expr_evaluate_unary
fixed it.  However, when I looked for another example that was allowed
by the current grammar to put into the commit message, the one I found
was caused by stmt_evaluate_payload instead, but on the assumption that
the cause was the same, I didn't actually verify it.  Whoops.

> I think stmt_evaluate_payload() is incomplete, this function was not
> made to deal with non-constant expression as values.
>
> Look:
>         tcp dport set tcp sport
>
> works because it follows the 'easy path', ie. no adjustment to make
> the checksum calculation happy (see payload_needs_adjustment() in
> stmt_evaluate_payload().
>
> However:
>
>         ip dscp set ip dscp
>
> bails out with:
>
>         nft add rule ip t c ip dscp set ip dscp
>         Error: Value 252 exceeds valid range 0-63
>         add rule ip t c ip dscp set ip dscp
>                                     ^^^^^^^
>
> because this follows the more complicated path. Looking at this code,
> this path assumes a constant value, ie. ip dscp set 10. A more complex
> thing such a non-constant expression (as in the example above) will
> need a bit of work.
>
> Probably you can start making a patchset make this work:
>
>         add rule ip t c tcp dport set tcp dport lshift 1
>
> which triggers:
>
> BUG: invalid binary operation 4
> nft: netlink_linearize.c:592: netlink_gen_binop: Assertion `0' failed.
>
> since it's missing the bytecode to generate the left-shift. Not very
> useful for users, but we can get something already merged upstream and
> you'll be half-way done. Merge also a few tests.
>
> Then, once the more fundamental rshift/lshift bits are merged, look at
> this 'harder' path. Just a proposal.
>
> For reference, the expression tree that stmt_evaluate_payload() to
> make the checksum adjustment looks like this:
>
>            xor
>           /   \
>         and   value
>         / \
> payload_   mask
>  bytes
>
> payload_bytes extends the payload expression to get up to 16-bits.
> The left hand side is there to fetch bits that need to be left
> untouched. The right hand side represent the bits that need to be set.
>
> In the new non-constant scenario, the 'value' tree is actually a
> binary operation:
>
>          shift
>         /    \
>    payload   imm
>
> The unary should not really be there, it's likely related to some
> incorrect byteorder issue that kicks in with non-constant expression.
>
> So more work on stmt_evaluate_payload() is required.

Thanks for the analysis, Pablo.  I'll get started.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH nft] evaluate: don't eval unary arguments.

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 4038 bytes --]

On 2020-01-28, at 19:49:18 +0100, Pablo Neira Ayuso wrote:
> On Mon, Jan 27, 2020 at 11:13:43AM +0000, Jeremy Sowden wrote:
> > On 2020-01-27, at 10:33:04 +0100, Pablo Neira Ayuso wrote:
> > > On Sun, Jan 19, 2020 at 06:12:03PM +0000, Jeremy Sowden wrote:
> > > > When a unary expression is inserted to implement a byte-order
> > > > conversion, the expression being converted has already been
> > > > evaluated and so expr_evaluate_unary doesn't need to do so.  For
> > > > most types of expression, the double evaluation doesn't matter
> > > > since evaluation is idempotent.  However, in the case of payload
> > > > expressions which are munged during evaluation, it can cause
> > > > unexpected errors:
> > > >
> > > >   # nft add table ip t
> > > >   # nft add chain ip t c '{ type filter hook input priority filter; }'
> > > >   # nft add rule ip t c ip dscp set 'ip dscp | 0x10'
> > > >   Error: Value 252 exceeds valid range 0-63
> > > >   add rule ip t c ip dscp set ip dscp | 0x10
> > > >                               ^^^^^^^
> > >
> > > I'm still hitting this after applying this patch.
> > >
> > > nft add rule ip t c ip dscp set ip dscp or 0x10
> > > Error: Value 252 exceeds valid range 0-63
> > > add rule ip t c ip dscp set ip dscp or 0x10
>
> [...]
>
> I think stmt_evaluate_payload() is incomplete, this function was not
> made to deal with non-constant expression as values.
>
> Look: tcp dport set tcp sport
>
> works because it follows the 'easy path', ie. no adjustment to make
> the checksum calculation happy (see payload_needs_adjustment() in
> stmt_evaluate_payload().
>
> However:
>
>         ip dscp set ip dscp
>
> bails out with:
>
>         nft add rule ip t c ip dscp set ip dscp
>         Error: Value 252 exceeds valid range 0-63
>         add rule ip t c ip dscp set ip dscp
>                                     ^^^^^^^
>
> because this follows the more complicated path. Looking at this code,
> this path assumes a constant value, ie. ip dscp set 10. A more complex
> thing such a non-constant expression (as in the example above) will
> need a bit of work.
>
> Probably you can start making a patchset make this work:
>
>         add rule ip t c tcp dport set tcp dport lshift 1
>
> which triggers:
>
> BUG: invalid binary operation 4
> nft: netlink_linearize.c:592: netlink_gen_binop: Assertion `0' failed.
>
> since it's missing the bytecode to generate the left-shift. Not very
> useful for users, but we can get something already merged upstream and
> you'll be half-way done. Merge also a few tests.

This assertion failure had already been fixed by the bitwise shift
patches you had recently applied.  However, the rule itself doesn't yet
quite work because `tcp dport lshift 1` has the wrong endianness.  Thus
given an original `tcp dport` of 40, we end up with 20480, instead of 80.

> Then, once the more fundamental rshift/lshift bits are merged, look at
> this 'harder' path. Just a proposal.
>
> For reference, the expression tree that stmt_evaluate_payload() to
> make the checksum adjustment looks like this:
>
>            xor
>           /   \
>         and   value
>         / \
> payload_   mask
>  bytes
>
> payload_bytes extends the payload expression to get up to 16-bits.
> The left hand side is there to fetch bits that need to be left
> untouched. The right hand side represent the bits that need to be set.
>
> In the new non-constant scenario, the 'value' tree is actually a
> binary operation:
>
>          shift
>         /    \
>    payload   imm
>
> The unary should not really be there, it's likely related to some
> incorrect byteorder issue that kicks in with non-constant expression.
>
> So more work on stmt_evaluate_payload() is required.

After giving this some thought, it occurred to me that this could be
fixed by extending bitwise boolean operations to support a variable
righthand operand (IIRC, before Christmas Florian suggested something
along these lines to me in another, related context), so I've gone down
that route.  Patches to follow shortly.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH nft] evaluate: don't eval unary arguments.

[Pablo Neira Ayuso]

On Sun, Feb 23, 2020 at 10:14:11PM +0000, Jeremy Sowden wrote:
> On 2020-01-28, at 19:49:18 +0100, Pablo Neira Ayuso wrote:
> > On Mon, Jan 27, 2020 at 11:13:43AM +0000, Jeremy Sowden wrote:
> > > On 2020-01-27, at 10:33:04 +0100, Pablo Neira Ayuso wrote:
> > > > On Sun, Jan 19, 2020 at 06:12:03PM +0000, Jeremy Sowden wrote:
> > > > > When a unary expression is inserted to implement a byte-order
> > > > > conversion, the expression being converted has already been
> > > > > evaluated and so expr_evaluate_unary doesn't need to do so.  For
> > > > > most types of expression, the double evaluation doesn't matter
> > > > > since evaluation is idempotent.  However, in the case of payload
> > > > > expressions which are munged during evaluation, it can cause
> > > > > unexpected errors:
> > > > >
> > > > >   # nft add table ip t
> > > > >   # nft add chain ip t c '{ type filter hook input priority filter; }'
> > > > >   # nft add rule ip t c ip dscp set 'ip dscp | 0x10'
> > > > >   Error: Value 252 exceeds valid range 0-63
> > > > >   add rule ip t c ip dscp set ip dscp | 0x10
> > > > >                               ^^^^^^^
> > > >
> > > > I'm still hitting this after applying this patch.
> > > >
> > > > nft add rule ip t c ip dscp set ip dscp or 0x10
> > > > Error: Value 252 exceeds valid range 0-63
> > > > add rule ip t c ip dscp set ip dscp or 0x10
> >
> > [...]
> >
> > I think stmt_evaluate_payload() is incomplete, this function was not
> > made to deal with non-constant expression as values.
> >
> > Look: tcp dport set tcp sport
> >
> > works because it follows the 'easy path', ie. no adjustment to make
> > the checksum calculation happy (see payload_needs_adjustment() in
> > stmt_evaluate_payload().
> >
> > However:
> >
> >         ip dscp set ip dscp
> >
> > bails out with:
> >
> >         nft add rule ip t c ip dscp set ip dscp
> >         Error: Value 252 exceeds valid range 0-63
> >         add rule ip t c ip dscp set ip dscp
> >                                     ^^^^^^^
> >
> > because this follows the more complicated path. Looking at this code,
> > this path assumes a constant value, ie. ip dscp set 10. A more complex
> > thing such a non-constant expression (as in the example above) will
> > need a bit of work.
> >
> > Probably you can start making a patchset make this work:
> >
> >         add rule ip t c tcp dport set tcp dport lshift 1
> >
> > which triggers:
> >
> > BUG: invalid binary operation 4
> > nft: netlink_linearize.c:592: netlink_gen_binop: Assertion `0' failed.
> >
> > since it's missing the bytecode to generate the left-shift. Not very
> > useful for users, but we can get something already merged upstream and
> > you'll be half-way done. Merge also a few tests.
> 
> This assertion failure had already been fixed by the bitwise shift
> patches you had recently applied.  However, the rule itself doesn't yet
> quite work because `tcp dport lshift 1` has the wrong endianness.  Thus
> given an original `tcp dport` of 40, we end up with 20480, instead of 80.

I think the generated bytecode should be like this:

        r1 <- payload to fetch value
        swap byteorder in r1
        shift value in r1
        cmp r1 and immediate value (in host byteorder)

> > Then, once the more fundamental rshift/lshift bits are merged, look at
> > this 'harder' path. Just a proposal.
> >
> > For reference, the expression tree that stmt_evaluate_payload() to
> > make the checksum adjustment looks like this:
> >
> >            xor
> >           /   \
> >         and   value
> >         / \
> > payload_   mask
> >  bytes
> >
> > payload_bytes extends the payload expression to get up to 16-bits.
> > The left hand side is there to fetch bits that need to be left
> > untouched. The right hand side represent the bits that need to be set.
> >
> > In the new non-constant scenario, the 'value' tree is actually a
> > binary operation:
> >
> >          shift
> >         /    \
> >    payload   imm
> >
> > The unary should not really be there, it's likely related to some
> > incorrect byteorder issue that kicks in with non-constant expression.
> >
> > So more work on stmt_evaluate_payload() is required.
> 
> After giving this some thought, it occurred to me that this could be
> fixed by extending bitwise boolean operations to support a variable
> righthand operand (IIRC, before Christmas Florian suggested something
> along these lines to me in another, related context), so I've gone down
> that route.  Patches to follow shortly.

Would this require a new kernel extensions? What's the idea behind
this?

Thanks.

Re: [PATCH nft] evaluate: don't eval unary arguments.

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Sun, Feb 23, 2020 at 10:14:11PM +0000, Jeremy Sowden wrote:
> > After giving this some thought, it occurred to me that this could be
> > fixed by extending bitwise boolean operations to support a variable
> > righthand operand (IIRC, before Christmas Florian suggested something
> > along these lines to me in another, related context), so I've gone down
> > that route.  Patches to follow shortly.
> 
> Would this require a new kernel extensions? What's the idea behind
> this?

Something like this:
nft ... ct mark set ct mark & 0xffff0000 | meta mark & 0xffff

Re: [PATCH nft] evaluate: don't eval unary arguments.

[Pablo Neira Ayuso]

On Sun, Feb 23, 2020 at 11:34:24PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Sun, Feb 23, 2020 at 10:14:11PM +0000, Jeremy Sowden wrote:
> > > After giving this some thought, it occurred to me that this could be
> > > fixed by extending bitwise boolean operations to support a variable
> > > righthand operand (IIRC, before Christmas Florian suggested something
> > > along these lines to me in another, related context), so I've gone down
> > > that route.  Patches to follow shortly.
> > 
> > Would this require a new kernel extensions? What's the idea behind
> > this?
> 
> Something like this:
> nft ... ct mark set ct mark & 0xffff0000 | meta mark & 0xffff

I see, so this requires two source registers as input for nft_bitwise?

Re: [PATCH nft] evaluate: don't eval unary arguments.

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Sun, Feb 23, 2020 at 11:34:24PM +0100, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > On Sun, Feb 23, 2020 at 10:14:11PM +0000, Jeremy Sowden wrote:
> > > > After giving this some thought, it occurred to me that this could be
> > > > fixed by extending bitwise boolean operations to support a variable
> > > > righthand operand (IIRC, before Christmas Florian suggested something
> > > > along these lines to me in another, related context), so I've gone down
> > > > that route.  Patches to follow shortly.
> > > 
> > > Would this require a new kernel extensions? What's the idea behind
> > > this?
> > 
> > Something like this:
> > nft ... ct mark set ct mark & 0xffff0000 | meta mark & 0xffff
> 
> I see, so this requires two source registers as input for nft_bitwise?

Yes, it requires two source registers as input, probably even two.
I have salvaged this old junk patch from an older branch of mine, it
added both sreg_mask and xor.

(I rebased it just now and it compiles).

I will do some more dumpster diving tomorrow to see if i can locate
the corresponding nftables and kernel branch.

---
 include/libnftnl/expr.h             |  2 ++
 include/linux/netfilter/nf_tables.h |  2 ++
 src/expr/bitwise.c                  | 39 ++++++++++++++++++++++++++---
 3 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/include/libnftnl/expr.h b/include/libnftnl/expr.h
index cfe456dbc7a5..30f4ef73e9d6 100644
--- a/include/libnftnl/expr.h
+++ b/include/libnftnl/expr.h
@@ -118,6 +118,8 @@ enum {
 	NFTNL_EXPR_BITWISE_XOR,
 	NFTNL_EXPR_BITWISE_OP,
 	NFTNL_EXPR_BITWISE_DATA,
+	NFTNL_EXPR_BITWISE_SREG_MASK,
+	NFTNL_EXPR_BITWISE_SREG_XOR,
 };
 
 enum {
diff --git a/include/linux/netfilter/nf_tables.h b/include/linux/netfilter/nf_tables.h
index 065218a20bb7..7c560a50ae19 100644
--- a/include/linux/netfilter/nf_tables.h
+++ b/include/linux/netfilter/nf_tables.h
@@ -549,6 +549,8 @@ enum nft_bitwise_attributes {
 	NFTA_BITWISE_XOR,
 	NFTA_BITWISE_OP,
 	NFTA_BITWISE_DATA,
+	NFTA_BITWISE_SREG_MASK,
+	NFTA_BITWISE_SREG_XOR,
 	__NFTA_BITWISE_MAX
 };
 #define NFTA_BITWISE_MAX	(__NFTA_BITWISE_MAX - 1)
diff --git a/src/expr/bitwise.c b/src/expr/bitwise.c
index 9ea2f662b3e6..7eb8d2a79c80 100644
--- a/src/expr/bitwise.c
+++ b/src/expr/bitwise.c
@@ -23,6 +23,8 @@
 
 struct nftnl_expr_bitwise {
 	enum nft_registers	sreg;
+	enum nft_registers	sreg_xor;
+	enum nft_registers	sreg_mask;
 	enum nft_registers	dreg;
 	enum nft_bitwise_ops	op;
 	unsigned int		len;
@@ -54,6 +56,9 @@ nftnl_expr_bitwise_set(struct nftnl_expr *e, uint16_t type,
 		memcpy(&bitwise->mask.val, data, data_len);
 		bitwise->mask.len = data_len;
 		break;
+	case NFTNL_EXPR_BITWISE_SREG_MASK:
+		memcpy(&bitwise->sreg_mask, data, sizeof(bitwise->sreg_mask));
+		break;
 	case NFTNL_EXPR_BITWISE_XOR:
 		memcpy(&bitwise->xor.val, data, data_len);
 		bitwise->xor.len = data_len;
@@ -62,6 +67,9 @@ nftnl_expr_bitwise_set(struct nftnl_expr *e, uint16_t type,
 		memcpy(&bitwise->data.val, data, data_len);
 		bitwise->data.len = data_len;
 		break;
+	case NFTNL_EXPR_BITWISE_SREG_XOR:
+		memcpy(&bitwise->sreg_xor, data, sizeof(bitwise->sreg_xor));
+		break;
 	default:
 		return -1;
 	}
@@ -90,12 +98,18 @@ nftnl_expr_bitwise_get(const struct nftnl_expr *e, uint16_t type,
 	case NFTNL_EXPR_BITWISE_MASK:
 		*data_len = bitwise->mask.len;
 		return &bitwise->mask.val;
+	case NFTNL_EXPR_BITWISE_SREG_MASK:
+		*data_len = sizeof(bitwise->sreg_mask);
+		return &bitwise->sreg_mask;
 	case NFTNL_EXPR_BITWISE_XOR:
 		*data_len = bitwise->xor.len;
 		return &bitwise->xor.val;
 	case NFTNL_EXPR_BITWISE_DATA:
 		*data_len = bitwise->data.len;
 		return &bitwise->data.val;
+	case NFTNL_EXPR_BITWISE_SREG_XOR:
+		*data_len = sizeof(bitwise->sreg_xor);
+		return &bitwise->sreg_xor;
 	}
 	return NULL;
 }
@@ -110,6 +124,8 @@ static int nftnl_expr_bitwise_cb(const struct nlattr *attr, void *data)
 
 	switch(type) {
 	case NFTA_BITWISE_SREG:
+	case NFTA_BITWISE_SREG_XOR:
+	case NFTA_BITWISE_SREG_MASK:
 	case NFTA_BITWISE_DREG:
 	case NFTA_BITWISE_OP:
 	case NFTA_BITWISE_LEN:
@@ -165,6 +181,8 @@ nftnl_expr_bitwise_build(struct nlmsghdr *nlh, const struct nftnl_expr *e)
 				bitwise->data.val);
 		mnl_attr_nest_end(nlh, nest);
 	}
+	if (e->flags & (1 << NFTNL_EXPR_BITWISE_SREG_XOR))
+		mnl_attr_put_u32(nlh, NFTA_BITWISE_SREG_XOR, htonl(bitwise->sreg_xor));
 }
 
 static int
@@ -197,6 +215,10 @@ nftnl_expr_bitwise_parse(struct nftnl_expr *e, struct nlattr *attr)
 		ret = nftnl_parse_data(&bitwise->mask, tb[NFTA_BITWISE_MASK], NULL);
 		e->flags |= (1 << NFTA_BITWISE_MASK);
 	}
+	if (tb[NFTA_BITWISE_SREG_MASK]) {
+		bitwise->sreg_mask = ntohl(mnl_attr_get_u32(tb[NFTA_BITWISE_SREG_MASK]));
+		e->flags |= (1 << NFTA_BITWISE_SREG_MASK);
+	}
 	if (tb[NFTA_BITWISE_XOR]) {
 		ret = nftnl_parse_data(&bitwise->xor, tb[NFTA_BITWISE_XOR], NULL);
 		e->flags |= (1 << NFTA_BITWISE_XOR);
@@ -205,13 +227,18 @@ nftnl_expr_bitwise_parse(struct nftnl_expr *e, struct nlattr *attr)
 		ret = nftnl_parse_data(&bitwise->data, tb[NFTA_BITWISE_DATA], NULL);
 		e->flags |= (1 << NFTNL_EXPR_BITWISE_DATA);
 	}
+	if (tb[NFTA_BITWISE_SREG_XOR]) {
+		bitwise->sreg_xor = ntohl(mnl_attr_get_u32(tb[NFTA_BITWISE_SREG_XOR]));
+		e->flags |= (1 << NFTA_BITWISE_SREG_XOR);
+	}
 
 	return ret;
 }
 
 static int
 nftnl_expr_bitwise_snprintf_bool(char *buf, size_t size,
-				 const struct nftnl_expr_bitwise *bitwise)
+				 const struct nftnl_expr_bitwise *bitwise,
+				 uint32_t flags)
 {
 	int remain = size, offset = 0, ret;
 
@@ -226,8 +253,12 @@ nftnl_expr_bitwise_snprintf_bool(char *buf, size_t size,
 	ret = snprintf(buf + offset, remain, ") ^ ");
 	SNPRINTF_BUFFER_SIZE(ret, remain, offset);
 
-	ret = nftnl_data_reg_snprintf(buf + offset, remain, &bitwise->xor,
-				      NFTNL_OUTPUT_DEFAULT, 0, DATA_VALUE);
+	if (flags & (1 << NFTNL_EXPR_BITWISE_SREG_XOR))
+		ret = snprintf(buf + offset, remain, "reg %u",
+			       bitwise->sreg_xor);
+	else
+		ret = nftnl_data_reg_snprintf(buf + offset, remain, &bitwise->xor,
+					    NFTNL_OUTPUT_DEFAULT, 0, DATA_VALUE);
 	SNPRINTF_BUFFER_SIZE(ret, remain, offset);
 
 	return offset;
@@ -260,7 +291,7 @@ static int nftnl_expr_bitwise_snprintf_default(char *buf, size_t size,
 
 	switch (bitwise->op) {
 	case NFT_BITWISE_BOOL:
-		err = nftnl_expr_bitwise_snprintf_bool(buf, size, bitwise);
+		err = nftnl_expr_bitwise_snprintf_bool(buf, size, bitwise, e->flags);
 		break;
 	case NFT_BITWISE_LSHIFT:
 		err = nftnl_expr_bitwise_snprintf_shift(buf, size, "<<", bitwise);
-- 
2.24.1

Re: [PATCH nft] evaluate: don't eval unary arguments.

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 5855 bytes --]

On 2020-02-23, at 23:23:21 +0100, Pablo Neira Ayuso wrote:
> On Sun, Feb 23, 2020 at 10:14:11PM +0000, Jeremy Sowden wrote:
> > On 2020-01-28, at 19:49:18 +0100, Pablo Neira Ayuso wrote:
> > > On Mon, Jan 27, 2020 at 11:13:43AM +0000, Jeremy Sowden wrote:
> > > > On 2020-01-27, at 10:33:04 +0100, Pablo Neira Ayuso wrote:
> > > > > On Sun, Jan 19, 2020 at 06:12:03PM +0000, Jeremy Sowden wrote:
> > > > > > When a unary expression is inserted to implement a
> > > > > > byte-order conversion, the expression being converted has
> > > > > > already been evaluated and so expr_evaluate_unary doesn't
> > > > > > need to do so.  For most types of expression, the double
> > > > > > evaluation doesn't matter since evaluation is idempotent.
> > > > > > However, in the case of payload expressions which are munged
> > > > > > during evaluation, it can cause unexpected errors:
> > > > > >
> > > > > >   # nft add table ip t
> > > > > >   # nft add chain ip t c '{ type filter hook input priority filter; }'
> > > > > >   # nft add rule ip t c ip dscp set 'ip dscp | 0x10'
> > > > > >   Error: Value 252 exceeds valid range 0-63
> > > > > >   add rule ip t c ip dscp set ip dscp | 0x10
> > > > > >                               ^^^^^^^
> > > > >
> > > > > I'm still hitting this after applying this patch.
> > > > >
> > > > > nft add rule ip t c ip dscp set ip dscp or 0x10
> > > > > Error: Value 252 exceeds valid range 0-63
> > > > > add rule ip t c ip dscp set ip dscp or 0x10
> > >
> > > [...]
> > >
> > > I think stmt_evaluate_payload() is incomplete, this function was
> > > not made to deal with non-constant expression as values.
> > >
> > > [...]
> > >
> > > Probably you can start making a patchset make this work:
> > >
> > >         add rule ip t c tcp dport set tcp dport lshift 1
> > >
> > > which triggers:
> > >
> > > BUG: invalid binary operation 4
> > > nft: netlink_linearize.c:592: netlink_gen_binop: Assertion `0'
> > > failed.
> > >
> > > since it's missing the bytecode to generate the left-shift. Not
> > > very useful for users, but we can get something already merged
> > > upstream and you'll be half-way done. Merge also a few tests.
> >
> > This assertion failure had already been fixed by the bitwise shift
> > patches you had recently applied.  However, the rule itself doesn't
> > yet quite work because `tcp dport lshift 1` has the wrong
> > endianness.  Thus given an original `tcp dport` of 40, we end up
> > with 20480, instead of 80.
>
> I think the generated bytecode should be like this:
>
>         r1 <- payload to fetch value
>         swap byteorder in r1
>         shift value in r1
>         cmp r1 and immediate value (in host byteorder)

Currently, nft generates this:

  [ meta load l4proto => reg 1 ]
  [ cmp eq reg 1 0x00000006 ]
  [ payload load 2b @ transport header + 0 => reg 1 ]
  [ byteorder reg 1 = ntoh(reg 1, 2, 2) ]
  [ bitwise reg 1 = ( reg 1 << 0x00000001 ) ]
  [ payload write reg 1 => 2b @ transport header + 2 csum_type 1 csum_off 16 csum_flags 0x0 ]

I have a patch to insert the missing hton:

  --- a/src/evaluate.c
  +++ b/src/evaluate.c
  @@ -2218,6 +2218,11 @@ static int stmt_evaluate_payload(struct eval_ctx *ctx, struct stmt *stmt)
                                payload->byteorder, &stmt->payload.val) < 0)
                  return -1;

  +       if (!expr_is_constant(stmt->payload.val) &&
  +           byteorder_conversion(ctx, &stmt->payload.val,
  +                                payload->byteorder) < 0)
  +               return -1;
  +
          need_csum = stmt_evaluate_payload_need_csum(payload);

          if (!payload_needs_adjustment(payload)) {

giving:

  [ meta load l4proto => reg 1 ]
  [ cmp eq reg 1 0x00000006 ]
  [ payload load 2b @ transport header + 0 => reg 1 ]
  [ byteorder reg 1 = ntoh(reg 1, 2, 2) ]
  [ bitwise reg 1 = ( reg 1 << 0x00000001 ) ]
  [ byteorder reg 1 = hton(reg 1, 2, 2) ]
  [ payload write reg 1 => 2b @ transport header + 2 csum_type 1 csum_off 16 csum_flags 0x0 ]

> > > Then, once the more fundamental rshift/lshift bits are merged,
> > > look at this 'harder' path. Just a proposal.
> > >
> > > For reference, the expression tree that stmt_evaluate_payload() to
> > > make the checksum adjustment looks like this:
> > >
> > >            xor
> > >           /   \
> > >         and   value
> > >         / \
> > > payload_   mask
> > >  bytes
> > >
> > > payload_bytes extends the payload expression to get up to 16-bits.
> > > The left hand side is there to fetch bits that need to be left
> > > untouched. The right hand side represent the bits that need to be
> > > set.
> > >
> > > In the new non-constant scenario, the 'value' tree is actually a
> > > binary operation:
> > >
> > >          shift
> > >         /    \
> > >    payload   imm
> > >
> > > The unary should not really be there, it's likely related to some
> > > incorrect byteorder issue that kicks in with non-constant
> > > expression.
> > >
> > > So more work on stmt_evaluate_payload() is required.
> >
> > After giving this some thought, it occurred to me that this could be
> > fixed by extending bitwise boolean operations to support a variable
> > righthand operand (IIRC, before Christmas Florian suggested
> > something along these lines to me in another, related context), so
> > I've gone down that route.  Patches to follow shortly.
>
> Would this require a new kernel extensions? What's the idea behind
> this?

In addition to what Florian has mentioned elsewhere (and the original
reason I started looking at this), is Kevin Darbyshire-Bryant's desire
to be able to do something like:

  nft add rule t c ct mark set ip dscp lshift 16 or 0x10

That specific example wouldn't require a variable RHS (but would require
other changes), but Florian suggested generalizing the solution, and
setting payload fields using non-constant expressions would.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]