Netfilter-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH nftables] evaluate: remove expr_set_context call.
@ 2019-12-20 19:02 Jeremy Sowden
  2019-12-24 23:12 ` Jeremy Sowden
  2019-12-24 23:14 ` [PATCH nft v2] evaluate: fix expr_set_context call for shift binops Jeremy Sowden
  0 siblings, 2 replies; 8+ messages in thread
From: Jeremy Sowden @ 2019-12-20 19:02 UTC (permalink / raw)
  To: Netfilter Devel

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


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

* [PATCH nftables] evaluate: remove expr_set_context call.
  2019-12-20 19:02 [PATCH nftables] evaluate: remove expr_set_context call Jeremy Sowden
@ 2019-12-24 23:12 ` Jeremy Sowden
  2019-12-24 23:16   ` Jeremy Sowden
  2019-12-24 23:14 ` [PATCH nft v2] evaluate: fix expr_set_context call for shift binops Jeremy Sowden
  1 sibling, 1 reply; 8+ messages in thread
From: Jeremy Sowden @ 2019-12-24 23:12 UTC (permalink / raw)
  To: Netfilter Devel

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


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

* [PATCH nft v2] evaluate: fix expr_set_context call for shift binops.
  2019-12-20 19:02 [PATCH nftables] evaluate: remove expr_set_context call Jeremy Sowden
  2019-12-24 23:12 ` Jeremy Sowden
@ 2019-12-24 23:14 ` Jeremy Sowden
  2020-01-06  9:28   ` Pablo Neira Ayuso
  1 sibling, 1 reply; 8+ messages in thread
From: Jeremy Sowden @ 2019-12-24 23:14 UTC (permalink / raw)
  To: Netfilter Devel

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


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

* Re: [PATCH nftables] evaluate: remove expr_set_context call.
  2019-12-24 23:12 ` Jeremy Sowden
@ 2019-12-24 23:16   ` Jeremy Sowden
  0 siblings, 0 replies; 8+ messages in thread
From: Jeremy Sowden @ 2019-12-24 23:16 UTC (permalink / raw)
  To: Netfilter Devel

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

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

* Re: [PATCH nft v2] evaluate: fix expr_set_context call for shift binops.
  2019-12-24 23:14 ` [PATCH nft v2] evaluate: fix expr_set_context call for shift binops Jeremy Sowden
@ 2020-01-06  9:28   ` Pablo Neira Ayuso
  2020-01-06  9:31     ` Jeremy Sowden
  2020-01-06 22:35     ` [PATCH nft v3] " Jeremy Sowden
  0 siblings, 2 replies; 8+ messages in thread
From: Pablo Neira Ayuso @ 2020-01-06  9:28 UTC (permalink / raw)
  To: Jeremy Sowden; +Cc: Netfilter Devel

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.

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

* Re: [PATCH nft v2] evaluate: fix expr_set_context call for shift binops.
  2020-01-06  9:28   ` Pablo Neira Ayuso
@ 2020-01-06  9:31     ` Jeremy Sowden
  2020-01-06 22:35     ` [PATCH nft v3] " Jeremy Sowden
  1 sibling, 0 replies; 8+ messages in thread
From: Jeremy Sowden @ 2020-01-06  9:31 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: Netfilter Devel

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

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

* [PATCH nft v3] evaluate: fix expr_set_context call for shift binops.
  2020-01-06  9:28   ` Pablo Neira Ayuso
  2020-01-06  9:31     ` Jeremy Sowden
@ 2020-01-06 22:35     ` " Jeremy Sowden
  2020-01-08 22:35       ` Pablo Neira Ayuso
  1 sibling, 1 reply; 8+ messages in thread
From: Jeremy Sowden @ 2020-01-06 22:35 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: Netfilter Devel

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


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

* Re: [PATCH nft v3] evaluate: fix expr_set_context call for shift binops.
  2020-01-06 22:35     ` [PATCH nft v3] " Jeremy Sowden
@ 2020-01-08 22:35       ` Pablo Neira Ayuso
  0 siblings, 0 replies; 8+ messages in thread
From: Pablo Neira Ayuso @ 2020-01-08 22:35 UTC (permalink / raw)
  To: Jeremy Sowden; +Cc: Netfilter Devel

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.

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

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-20 19:02 [PATCH nftables] evaluate: remove expr_set_context call Jeremy Sowden
2019-12-24 23:12 ` Jeremy Sowden
2019-12-24 23:16   ` Jeremy Sowden
2019-12-24 23:14 ` [PATCH nft v2] evaluate: fix expr_set_context call for shift binops Jeremy Sowden
2020-01-06  9:28   ` Pablo Neira Ayuso
2020-01-06  9:31     ` Jeremy Sowden
2020-01-06 22:35     ` [PATCH nft v3] " Jeremy Sowden
2020-01-08 22:35       ` Pablo Neira Ayuso

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