Netfilter-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH nft] evaluate: don't eval unary arguments.
@ 2020-01-19 18:12 Jeremy Sowden
  2020-01-27  9:33 ` Pablo Neira Ayuso
  0 siblings, 1 reply; 5+ messages in thread
From: Jeremy Sowden @ 2020-01-19 18:12 UTC (permalink / raw)
  To: Netfilter Devel

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH nft] evaluate: don't eval unary arguments.
  2020-01-19 18:12 [PATCH nft] evaluate: don't eval unary arguments Jeremy Sowden
@ 2020-01-27  9:33 ` Pablo Neira Ayuso
  2020-01-27 11:13   ` Jeremy Sowden
  0 siblings, 1 reply; 5+ messages in thread
From: Pablo Neira Ayuso @ 2020-01-27  9:33 UTC (permalink / raw)
  To: Jeremy Sowden; +Cc: Netfilter Devel

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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH nft] evaluate: don't eval unary arguments.
  2020-01-27  9:33 ` Pablo Neira Ayuso
@ 2020-01-27 11:13   ` Jeremy Sowden
  2020-01-28 18:49     ` Pablo Neira Ayuso
  0 siblings, 1 reply; 5+ messages in thread
From: Jeremy Sowden @ 2020-01-27 11:13 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: Netfilter Devel

[-- 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 --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH nft] evaluate: don't eval unary arguments.
  2020-01-27 11:13   ` Jeremy Sowden
@ 2020-01-28 18:49     ` Pablo Neira Ayuso
  2020-02-04 11:02       ` Jeremy Sowden
  0 siblings, 1 reply; 5+ messages in thread
From: Pablo Neira Ayuso @ 2020-01-28 18:49 UTC (permalink / raw)
  To: Jeremy Sowden; +Cc: Netfilter Devel

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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH nft] evaluate: don't eval unary arguments.
  2020-01-28 18:49     ` Pablo Neira Ayuso
@ 2020-02-04 11:02       ` Jeremy Sowden
  0 siblings, 0 replies; 5+ messages in thread
From: Jeremy Sowden @ 2020-02-04 11:02 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: Netfilter Devel

[-- 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 --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-19 18:12 [PATCH nft] evaluate: don't eval unary arguments Jeremy Sowden
2020-01-27  9:33 ` Pablo Neira Ayuso
2020-01-27 11:13   ` Jeremy Sowden
2020-01-28 18:49     ` Pablo Neira Ayuso
2020-02-04 11:02       ` Jeremy Sowden

Netfilter-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netfilter-devel/0 netfilter-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netfilter-devel netfilter-devel/ https://lore.kernel.org/netfilter-devel \
		netfilter-devel@vger.kernel.org
	public-inbox-index netfilter-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netfilter-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git