[PATCH nftables] evaluate: remove expr_set_context call.

[PATCH nftables] evaluate: remove expr_set_context call.

[Jeremy Sowden]

expr_evaluate_binop calls expr_set_context for shift expressions to set
the context data-type to `integer`.  This doesn't seem to serve a
purpose, and its only effect is to clobber the byte-order of the
context, resulting in unexpected conversions to NBO.  For example:

  $ sudo nft flush ruleset
  $ sudo nft add table t
  $ sudo nft add chain t c '{ type filter hook output priority mangle; }'
  $ sudo nft add rule t c oif lo tcp dport ssh ct mark set '0x10 | 0xe'
  $ sudo nft add rule t c oif lo tcp dport ssh ct mark set '0xf << 1'
  $ sudo nft list table t
  table ip t {
          chain c {
                  type filter hook output priority mangle; policy accept;
                  oif "lo" tcp dport 22 ct mark set 0x0000001e
                  oif "lo" tcp dport 22 ct mark set 0x1e000000
          }
  }

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

diff --git a/src/evaluate.c b/src/evaluate.c
index a865902c0fc7..0feb7d484405 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1144,8 +1144,6 @@ static int expr_evaluate_binop(struct eval_ctx *ctx, struct expr **expr)
 		return -1;
 	left = op->left;
 
-	if (op->op == OP_LSHIFT || op->op == OP_RSHIFT)
-		expr_set_context(&ctx->ectx, &integer_type, ctx->ectx.len);
 	if (expr_evaluate(ctx, &op->right) < 0)
 		return -1;
 	right = op->right;
-- 
2.24.0

[PATCH nftables] evaluate: remove expr_set_context call.

[Jeremy Sowden]

expr_evaluate_binop calls expr_set_context for shift expressions to set
the context data-type to `integer`.  This doesn't seem to serve a
purpose, and its only effect is to clobber the byte-order of the
context, resulting in unexpected conversions to NBO.  For example:

  $ sudo nft flush ruleset
  $ sudo nft add table t
  $ sudo nft add chain t c '{ type filter hook output priority mangle; }'
  $ sudo nft add rule t c oif lo tcp dport ssh ct mark set '0x10 | 0xe'
  $ sudo nft add rule t c oif lo tcp dport ssh ct mark set '0xf << 1'
  $ sudo nft list table t
  table ip t {
          chain c {
                  type filter hook output priority mangle; policy accept;
                  oif "lo" tcp dport 22 ct mark set 0x0000001e
                  oif "lo" tcp dport 22 ct mark set 0x1e000000
          }
  }

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

diff --git a/src/evaluate.c b/src/evaluate.c
index a865902c0fc7..0feb7d484405 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1144,8 +1144,6 @@ static int expr_evaluate_binop(struct eval_ctx *ctx, struct expr **expr)
 		return -1;
 	left = op->left;
 
-	if (op->op == OP_LSHIFT || op->op == OP_RSHIFT)
-		expr_set_context(&ctx->ectx, &integer_type, ctx->ectx.len);
 	if (expr_evaluate(ctx, &op->right) < 0)
 		return -1;
 	right = op->right;
-- 
2.24.0

[PATCH nft v2] evaluate: fix expr_set_context call for shift binops.

[Jeremy Sowden]

expr_evaluate_binop calls expr_set_context for shift expressions to set
the context data-type to `integer`.  This clobbers the byte-order of the
context, resulting in unexpected conversions to NBO.  For example:

  $ sudo nft flush ruleset
  $ sudo nft add table t
  $ sudo nft add chain t c '{ type filter hook output priority mangle; }'
  $ sudo nft add rule t c oif lo tcp dport ssh ct mark set '0x10 | 0xe'
  $ sudo nft add rule t c oif lo tcp dport ssh ct mark set '0xf << 1'
  $ sudo nft list table t
  table ip t {
          chain c {
                  type filter hook output priority mangle; policy accept;
                  oif "lo" tcp dport 22 ct mark set 0x0000001e
                  oif "lo" tcp dport 22 ct mark set 0x1e000000
          }
  }

Replace it with a call to __expr_set_context in order to preserve host
endianness.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 src/evaluate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Change-log:

  In v1, I just dropped the expr_set_context call; however, it is needed
  to ensure that the right operand has integer type.  Instead, I now
  change it to __expr_set_context in order to ensure that the byte-order
  is correct.

diff --git a/src/evaluate.c b/src/evaluate.c
index a865902c0fc7..43637e1cf6c8 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1145,7 +1145,8 @@ static int expr_evaluate_binop(struct eval_ctx *ctx, struct expr **expr)
 	left = op->left;
 
 	if (op->op == OP_LSHIFT || op->op == OP_RSHIFT)
-		expr_set_context(&ctx->ectx, &integer_type, ctx->ectx.len);
+		__expr_set_context(&ctx->ectx, &integer_type,
+				   BYTEORDER_HOST_ENDIAN, ctx->ectx.len, 0);
 	if (expr_evaluate(ctx, &op->right) < 0)
 		return -1;
 	right = op->right;
-- 
2.24.0

Re: [PATCH nftables] evaluate: remove expr_set_context call.

[Jeremy Sowden]

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

On 2019-12-24, at 23:12:11 +0000, Jeremy Sowden wrote:
> expr_evaluate_binop calls expr_set_context for shift expressions to
> set the context data-type to `integer`.  This doesn't seem to serve a
> purpose, and its only effect is to clobber the byte-order of the
> context, resulting in unexpected conversions to NBO.  For example:

Whoops.  Sent v1 again, instead of v2.  Let's try that again.

J.

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

Re: [PATCH nft v2] evaluate: fix expr_set_context call for shift binops.

[Pablo Neira Ayuso]

Hi Jeremy,

On Tue, Dec 24, 2019 at 11:14:28PM +0000, Jeremy Sowden wrote:
> expr_evaluate_binop calls expr_set_context for shift expressions to set
> the context data-type to `integer`.  This clobbers the byte-order of the
> context, resulting in unexpected conversions to NBO.  For example:
> 
>   $ sudo nft flush ruleset
>   $ sudo nft add table t
>   $ sudo nft add chain t c '{ type filter hook output priority mangle; }'
>   $ sudo nft add rule t c oif lo tcp dport ssh ct mark set '0x10 | 0xe'
>   $ sudo nft add rule t c oif lo tcp dport ssh ct mark set '0xf << 1'
>   $ sudo nft list table t
>   table ip t {
>           chain c {
>                   type filter hook output priority mangle; policy accept;
>                   oif "lo" tcp dport 22 ct mark set 0x0000001e
>                   oif "lo" tcp dport 22 ct mark set 0x1e000000
>           }
>   }
> 
> Replace it with a call to __expr_set_context in order to preserve host
> endianness.
> 
> Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> ---
>  src/evaluate.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Change-log:
> 
>   In v1, I just dropped the expr_set_context call; however, it is needed
>   to ensure that the right operand has integer type.  Instead, I now
>   change it to __expr_set_context in order to ensure that the byte-order
>   is correct.
> 
> diff --git a/src/evaluate.c b/src/evaluate.c
> index a865902c0fc7..43637e1cf6c8 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -1145,7 +1145,8 @@ static int expr_evaluate_binop(struct eval_ctx *ctx, struct expr **expr)
>  	left = op->left;
>  
>  	if (op->op == OP_LSHIFT || op->op == OP_RSHIFT)
> -		expr_set_context(&ctx->ectx, &integer_type, ctx->ectx.len);
> +		__expr_set_context(&ctx->ectx, &integer_type,
> +				   BYTEORDER_HOST_ENDIAN, ctx->ectx.len, 0);

tests/py spews a few warnings after this patch.

Re: [PATCH nft v2] evaluate: fix expr_set_context call for shift binops.

[Jeremy Sowden]

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

On 2020-01-06, at 10:28:42 +0100, Pablo Neira Ayuso wrote:
> On Tue, Dec 24, 2019 at 11:14:28PM +0000, Jeremy Sowden wrote:
> > expr_evaluate_binop calls expr_set_context for shift expressions to
> > set the context data-type to `integer`.  This clobbers the
> > byte-order of the context, resulting in unexpected conversions to
> > NBO.  For example:
> >
> >   $ sudo nft flush ruleset
> >   $ sudo nft add table t
> >   $ sudo nft add chain t c '{ type filter hook output priority mangle; }'
> >   $ sudo nft add rule t c oif lo tcp dport ssh ct mark set '0x10 | 0xe'
> >   $ sudo nft add rule t c oif lo tcp dport ssh ct mark set '0xf << 1'
> >   $ sudo nft list table t
> >   table ip t {
> >           chain c {
> >                   type filter hook output priority mangle; policy accept;
> >                   oif "lo" tcp dport 22 ct mark set 0x0000001e
> >                   oif "lo" tcp dport 22 ct mark set 0x1e000000
> >           }
> >   }
> >
> > Replace it with a call to __expr_set_context in order to preserve host
> > endianness.
> >
> > Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> > ---
> >  src/evaluate.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > Change-log:
> >
> >   In v1, I just dropped the expr_set_context call; however, it is needed
> >   to ensure that the right operand has integer type.  Instead, I now
> >   change it to __expr_set_context in order to ensure that the byte-order
> >   is correct.
> >
> > diff --git a/src/evaluate.c b/src/evaluate.c
> > index a865902c0fc7..43637e1cf6c8 100644
> > --- a/src/evaluate.c
> > +++ b/src/evaluate.c
> > @@ -1145,7 +1145,8 @@ static int expr_evaluate_binop(struct eval_ctx *ctx, struct expr **expr)
> >  	left = op->left;
> >
> >  	if (op->op == OP_LSHIFT || op->op == OP_RSHIFT)
> > -		expr_set_context(&ctx->ectx, &integer_type, ctx->ectx.len);
> > +		__expr_set_context(&ctx->ectx, &integer_type,
> > +				   BYTEORDER_HOST_ENDIAN, ctx->ectx.len, 0);
>
> tests/py spews a few warnings after this patch.

I'll take a look.

Cheers.

J.

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

[PATCH nft v3] evaluate: fix expr_set_context call for shift binops.

[Jeremy Sowden]

expr_evaluate_binop calls expr_set_context for shift expressions to set
the context data-type to `integer`.  This clobbers the byte-order of the
context, resulting in unexpected conversions to NBO.  For example:

  $ sudo nft flush ruleset
  $ sudo nft add table t
  $ sudo nft add chain t c '{ type filter hook output priority mangle; }'
  $ sudo nft add rule t c oif lo tcp dport ssh ct mark set '0x10 | 0xe'
  $ sudo nft add rule t c oif lo tcp dport ssh ct mark set '0xf << 1'
  $ sudo nft list table t
  table ip t {
          chain c {
                  type filter hook output priority mangle; policy accept;
                  oif "lo" tcp dport 22 ct mark set 0x0000001e
                  oif "lo" tcp dport 22 ct mark set 0x1e000000
          }
  }

Replace it with a call to __expr_set_context and set the byteorder to
that of the left operand since this is the value being shifted.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 src/evaluate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Since v2:

  * set the byte-order to that of the left operand, rather than hard-
    coding it as host-endian.

Since v1:

  * replace expr_set_context with __expr_set_context (and explicity set
    the byte-order) instead of removing it altogether in order to ensure
    that the right operand has integer type.

diff --git a/src/evaluate.c b/src/evaluate.c
index 817b23220bb9..34e4473e4c9a 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1145,7 +1145,8 @@ static int expr_evaluate_binop(struct eval_ctx *ctx, struct expr **expr)
 	left = op->left;
 
 	if (op->op == OP_LSHIFT || op->op == OP_RSHIFT)
-		expr_set_context(&ctx->ectx, &integer_type, ctx->ectx.len);
+		__expr_set_context(&ctx->ectx, &integer_type,
+				   left->byteorder, ctx->ectx.len, 0);
 	if (expr_evaluate(ctx, &op->right) < 0)
 		return -1;
 	right = op->right;
-- 
2.24.1

Re: [PATCH nft v3] evaluate: fix expr_set_context call for shift binops.

[Pablo Neira Ayuso]

On Mon, Jan 06, 2020 at 10:35:10PM +0000, Jeremy Sowden wrote:
> expr_evaluate_binop calls expr_set_context for shift expressions to set
> the context data-type to `integer`.  This clobbers the byte-order of the
> context, resulting in unexpected conversions to NBO.  For example:
> 
>   $ sudo nft flush ruleset
>   $ sudo nft add table t
>   $ sudo nft add chain t c '{ type filter hook output priority mangle; }'
>   $ sudo nft add rule t c oif lo tcp dport ssh ct mark set '0x10 | 0xe'
>   $ sudo nft add rule t c oif lo tcp dport ssh ct mark set '0xf << 1'
>   $ sudo nft list table t
>   table ip t {
>           chain c {
>                   type filter hook output priority mangle; policy accept;
>                   oif "lo" tcp dport 22 ct mark set 0x0000001e
>                   oif "lo" tcp dport 22 ct mark set 0x1e000000
>           }
>   }
> 
> Replace it with a call to __expr_set_context and set the byteorder to
> that of the left operand since this is the value being shifted.

Looks good, applied, thanks Jeremy.