* [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 related [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 related [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 related [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 related [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, other threads:[~2020-01-08 22:35 UTC | newest]
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).