[PATCH nft 0/3] fix ether address formatting

[PATCH nft 0/3] fix ether address formatting

[Florian Westphal]

ether addresses were listed as HOST_ENDIAN.
This did not show up for plain payload expressions because
the payload expression postprocessing contained a byteorder
conversion for HOST_ENDIAN.

It also did not show up when testing payload statements
because test suite conveniently used ff:ff:ff ...

So,
1. switch the data type to BIG_ENDIAN
2. fix test case to catch bogus formatting
3. remove the payload byte order conversion, I do not think
   it is required (all tests pass without it after patch #1).

[PATCH nft 1/3] datatype: ll: use big endian byte ordering

[Florian Westphal]

ether daddr set 00:03:2d:2b:74:ec  is listed as:
ether daddr set ec:74:2b:2d:03:00

(it was fine without 'set' keyword).  Reason is that
ether address was listed as being HOST endian.

The payload expression (unlike statement) path contains
a few conversion call sites for this, i.e.:

if (tmp->byteorder == BYTEORDER_HOST_ENDIAN)
   mpz_switch_byteorder(tmp->value, tmp->len / BITS_PER_BYTE);

... it might make sense to remove those in a followup patch.

Reported-by: Laura Garcia Liebana <nevola@gmail.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/datatype.c | 7 ++++---
 src/proto.c    | 4 ++--
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/datatype.c b/src/datatype.c
index 2b1619a..1e40287 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -348,7 +348,8 @@ static void lladdr_type_print(const struct expr *expr)
 	uint8_t data[len];
 	unsigned int i;
 
-	mpz_export_data(data, expr->value, BYTEORDER_HOST_ENDIAN, len);
+	mpz_export_data(data, expr->value, BYTEORDER_BIG_ENDIAN, len);
+
 	for (i = 0; i < len; i++) {
 		printf("%s%.2x", delim, data[i]);
 		delim = ":";
@@ -374,7 +375,7 @@ static struct error_record *lladdr_type_parse(const struct expr *sym,
 	}
 
 	*res = constant_expr_alloc(&sym->location, sym->dtype,
-				   BYTEORDER_HOST_ENDIAN, len * BITS_PER_BYTE,
+				   BYTEORDER_BIG_ENDIAN, len * BITS_PER_BYTE,
 				   buf);
 	return NULL;
 }
@@ -383,7 +384,7 @@ const struct datatype lladdr_type = {
 	.type		= TYPE_LLADDR,
 	.name		= "ll_addr",
 	.desc		= "link layer address",
-	.byteorder	= BYTEORDER_HOST_ENDIAN,
+	.byteorder	= BYTEORDER_BIG_ENDIAN,
 	.basetype	= &integer_type,
 	.print		= lladdr_type_print,
 	.parse		= lladdr_type_parse,
diff --git a/src/proto.c b/src/proto.c
index 94995f1..df5439c 100644
--- a/src/proto.c
+++ b/src/proto.c
@@ -854,7 +854,7 @@ const struct datatype etheraddr_type = {
 	.type		= TYPE_ETHERADDR,
 	.name		= "ether_addr",
 	.desc		= "Ethernet address",
-	.byteorder	= BYTEORDER_HOST_ENDIAN,
+	.byteorder	= BYTEORDER_BIG_ENDIAN,
 	.size		= ETH_ALEN * BITS_PER_BYTE,
 	.basetype	= &lladdr_type,
 };
@@ -892,7 +892,7 @@ const struct datatype ethertype_type = {
 	ETHHDR_TEMPLATE(__name, &ethertype_type, __member)
 #define ETHHDR_ADDR(__name, __member) \
 	PROTO_HDR_TEMPLATE(__name, &etheraddr_type,		\
-			   BYTEORDER_HOST_ENDIAN,		\
+			   BYTEORDER_BIG_ENDIAN,		\
 			   offsetof(struct ether_header, __member) * 8,	\
 			   field_sizeof(struct ether_header, __member) * 8)
 
-- 
2.7.3

Re: [PATCH nft 1/3] datatype: ll: use big endian byte ordering

[Pablo Neira Ayuso]

On Fri, Sep 09, 2016 at 02:43:14PM +0200, Florian Westphal wrote:
> ether daddr set 00:03:2d:2b:74:ec  is listed as:
> ether daddr set ec:74:2b:2d:03:00
> 
> (it was fine without 'set' keyword).  Reason is that
> ether address was listed as being HOST endian.
> 
> The payload expression (unlike statement) path contains
> a few conversion call sites for this, i.e.:
> 
> if (tmp->byteorder == BYTEORDER_HOST_ENDIAN)
>    mpz_switch_byteorder(tmp->value, tmp->len / BITS_PER_BYTE);
> 
> ... it might make sense to remove those in a followup patch.
> 
> Reported-by: Laura Garcia Liebana <nevola@gmail.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

[PATCH nft 2/3] tests: catch ordering issue w. ether set

[Florian Westphal]

Before previous commit, ether set (payload statement) was reversed on
output:

ether daddr set 00:03:2d:2b:74:ec

would be shown as 'ec:74:2b:2d:03:00'.

With ff:ff:ff ... such bug doesn't appear so use something
where it will show up.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 tests/py/bridge/ether.t         | 2 +-
 tests/py/bridge/ether.t.payload | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/py/bridge/ether.t b/tests/py/bridge/ether.t
index 4a177ab..5c6766e 100644
--- a/tests/py/bridge/ether.t
+++ b/tests/py/bridge/ether.t
@@ -7,4 +7,4 @@ tcp dport 22 ip daddr 1.2.3.4 ether saddr 00:0f:54:0c:11:04;ok;tcp dport 22 ethe
 tcp dport 22 ether saddr 00:0f:54:0c:11:04 ip daddr 1.2.3.4;ok
 ether saddr 00:0f:54:0c:11:04 ip daddr 1.2.3.4 accept;ok
 
-ether daddr 00:01:02:03:04:05 ether saddr set ff:ff:ff:ff:ff:ff drop;ok
+ether daddr 00:01:02:03:04:05 ether saddr set ff:fe:dc:ba:98:76 drop;ok
diff --git a/tests/py/bridge/ether.t.payload b/tests/py/bridge/ether.t.payload
index c545d65..74956b6 100644
--- a/tests/py/bridge/ether.t.payload
+++ b/tests/py/bridge/ether.t.payload
@@ -42,11 +42,11 @@ bridge test-bridge input
   [ cmp eq reg 1 0x04030201 ]
   [ immediate reg 0 accept ]
 
-# ether daddr 00:01:02:03:04:05 ether saddr set ff:ff:ff:ff:ff:ff drop
+# ether daddr 00:01:02:03:04:05 ether saddr set ff:fe:dc:ba:98:76 drop
 bridge test-bridge input
   [ payload load 6b @ link header + 0 => reg 1 ]
   [ cmp eq reg 1 0x03020100 0x00000504 ]
-  [ immediate reg 1 0xffffffff 0x0000ffff ]
+  [ immediate reg 1 0xbadcfeff 0x00007698 ]
   [ payload write reg 1 => 6b @ link header + 6 csum_type 0 csum_off 0 ]
   [ immediate reg 0 drop ]
 
-- 
2.7.3

Re: [PATCH nft 2/3] tests: catch ordering issue w. ether set

[Pablo Neira Ayuso]

On Fri, Sep 09, 2016 at 02:43:15PM +0200, Florian Westphal wrote:
> Before previous commit, ether set (payload statement) was reversed on
> output:
> 
> ether daddr set 00:03:2d:2b:74:ec
> 
> would be shown as 'ec:74:2b:2d:03:00'.
> 
> With ff:ff:ff ... such bug doesn't appear so use something
> where it will show up.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

[PATCH nft 3/3] payload: remove byteorder conversion

[Florian Westphal]

This is what made ether addresses get formatted correctly with
plain payload expression (ether saddr 00:11 ...) when listing
rules.  Not needed anymore since etheraddr_type is now BIG_ENDIAN.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/netlink_delinearize.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index cddbfa6..05edb01 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -1161,8 +1161,6 @@ static void payload_match_expand(struct rule_pp_ctx *ctx,
 	list_for_each_entry(left, &list, list) {
 		tmp = constant_expr_splice(right, left->len);
 		expr_set_type(tmp, left->dtype, left->byteorder);
-		if (tmp->byteorder == BYTEORDER_HOST_ENDIAN)
-			mpz_switch_byteorder(tmp->value, tmp->len / BITS_PER_BYTE);
 
 		nexpr = relational_expr_alloc(&expr->location, expr->op,
 					      left, tmp);
-- 
2.7.3

Re: [PATCH nft 3/3] payload: remove byteorder conversion

[Pablo Neira Ayuso]

On Fri, Sep 09, 2016 at 02:43:16PM +0200, Florian Westphal wrote:
> This is what made ether addresses get formatted correctly with
> plain payload expression (ether saddr 00:11 ...) when listing
> rules.  Not needed anymore since etheraddr_type is now BIG_ENDIAN.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>