[PATCH net v2 0/2] Fix tc-ife bugs

This patch-set contains two bugfixes in the tc-ife action, one fixing some
random behaviour in encode side, and one fixing the decode side packet
parsing logic.

Yotam Gigi (2):
  act_ife: Fix external mac header on encode
  act_ife: Fix false parsing on decode side

 net/sched/act_ife.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

-- 
2.4.11

On ife encode side, external mac header is copied from the original packet
and may be overridden if the user requests. Before, the mac header copy
was done from memory region that might not be accessible anymore, as
skb_cow_head might free it and copy the packet. This led to random values
in the external mac header once the values were not set by user.

This fix takes the internal mac header from the packet, after the call to
skb_cow_head.

Fixes: ef6980b6becb ("net sched: introduce IFE action")
Signed-off-by: Yotam Gigi <redacted>
---
 net/sched/act_ife.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index ccf7b4b..b949d97 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c

@@ -766,8 +766,6 @@ static int tcf_ife_encode(struct sk_buff *skb, const struct tc_action *a,
 		return TC_ACT_SHOT;
 	}
 
-	iethh = eth_hdr(skb);
-
 	err = skb_cow_head(skb, hdrm);
 	if (unlikely(err)) {
 		ife->tcf_qstats.drops++;

@@ -778,6 +776,7 @@ static int tcf_ife_encode(struct sk_buff *skb, const struct tc_action *a,
 	if (!(at & AT_EGRESS))
 		skb_push(skb, skb->dev->hard_header_len);
 
+	iethh = (struct ethhdr *)skb->data;
 	__skb_push(skb, hdrm);
 	memcpy(skb->data, iethh, skb->mac_len);
 	skb_reset_mac_header(skb);

-- 
2.4.11

On ife decode side, the action iterates over the tlvs in the ife header
and parses them one by one, where in each iteration the current pointer is
advanced according to the tlv size.

Before, the pointer was advanced in a wrong way, as the tlv type and size
bytes were not taken into account. This led to false parsing of ife
header. In addition, due to the fact that the loop counter was unsigned,
it could lead to infinite parsing loop.

This fix changes the loop counter to be signed and fixes the parsing to
take into account the tlv type and size.

Fixes: ef6980b6becb ("net sched: introduce IFE action")
Signed-off-by: Yotam Gigi <redacted>
---
 net/sched/act_ife.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index b949d97..1a055ea 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c

@@ -653,7 +653,7 @@ static int tcf_ife_decode(struct sk_buff *skb, const struct tc_action *a,
 	struct tcf_ife_info *ife = to_ife(a);
 	int action = ife->tcf_action;
 	struct ifeheadr *ifehdr = (struct ifeheadr *)skb->data;
-	u16 ifehdrln = ifehdr->metalen;
+	int ifehdrln = (int)ifehdr->metalen;
 	struct meta_tlvhdr *tlv = (struct meta_tlvhdr *)(ifehdr->tlv_data);
 
 	spin_lock(&ife->tcf_lock);

@@ -694,8 +694,8 @@ static int tcf_ife_decode(struct sk_buff *skb, const struct tc_action *a,
 			ife->tcf_qstats.overlimits++;
 		}
 
-		tlvdata += alen;
-		ifehdrln -= alen;
+		tlvdata += alen + sizeof(struct meta_tlvhdr);
+		ifehdrln -= alen + sizeof(struct meta_tlvhdr);
 		tlv = (struct meta_tlvhdr *)tlvdata;
 	}
 

-- 
2.4.11

On 16-09-25 08:31 AM, Yotam Gigi wrote:

This patch-set contains two bugfixes in the tc-ife action, one fixing some
random behaviour in encode side, and one fixing the decode side packet
parsing logic.

Yotam Gigi (2):
  act_ife: Fix external mac header on encode
  act_ife: Fix false parsing on decode side

 net/sched/act_ife.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Yotam, did you run the test i proposed? I am worried about the TLV one.
Note:
Encoder includes the length of the TLV header length in the L part.
If you are reaching a conclusion that you need this in the decoding:
+		tlvdata += alen + sizeof(struct meta_tlvhdr);

then very likely whoever is sending that packet is not encoding
correctly.

cheers,
jamal

-----Original Message-----
From: Jamal Hadi Salim [mailto:jhs@mojatatu.com]
Sent: Sunday, September 25, 2016 4:46 PM
To: Yotam Gigi <yotam.gi@gmail.com>; davem@davemloft.net;
netdev@vger.kernel.org; Yotam Gigi [off-list ref]
Subject: Re: [PATCH net v2 0/2] Fix tc-ife bugs

On 16-09-25 08:31 AM, Yotam Gigi wrote:

quoted

This patch-set contains two bugfixes in the tc-ife action, one fixing some
random behaviour in encode side, and one fixing the decode side packet
parsing logic.

Yotam Gigi (2):
  act_ife: Fix external mac header on encode
  act_ife: Fix false parsing on decode side

 net/sched/act_ife.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Yotam, did you run the test i proposed? I am worried about the TLV one.
Note:
Encoder includes the length of the TLV header length in the L part.
If you are reaching a conclusion that you need this in the decoding:
+              tlvdata += alen + sizeof(struct meta_tlvhdr);

then very likely whoever is sending that packet is not encoding
correctly.

The one encoding the packets I get is the ife action. You can look at the code:

  int ife_tlv_meta_encode(void *skbdata, u16 attrtype, u16 dlen, const void *dval)
  {
        u32 *tlv = (u32 *)(skbdata);
        u16 totlen = nla_total_size(dlen);      /*alignment + hdr */
        char *dptr = (char *)tlv + NLA_HDRLEN;
        u32 htlv = attrtype << 16 | dlen;

        *tlv = htonl(htlv);
        memset(dptr, 0, totlen - NLA_HDRLEN);
        memcpy(dptr, dval, dlen);
        
        return totlen;
  }

As you can see, the data that is actually written into the packet is the htlv
var, which has the 'dlen' in it, and not the totlen which corresponds the tlv
header size. You can see that in the following example:

I ran the tc command you proposed:
  $TC filter add dev $ETH parent 1: protocol ip prio 10 \
        u32 match ip protocol 1 0xff flowid 1:2 \
        action skbedit prio 33 \
        action ife encode \
        type 0xDEAD \
        use mark 12 \
        allow prio \
        dst 02:15:15:15:15:15

And this is the packet I got:
  0x0000:  0215 1515 1515 da23 d209 8644 dead 0012
  0x0010:  0001 0004 0000 000c 0003 0004 0000 0033
  0x0020:  fa30 7418 593a da23 d209 8644 0800 4500
  0x0030:  0054 da3c 4000 4001 486a 0c00 0001 0c00
  0x0040:  0002 0800 9fa2 1562 0008 aeec e757 0000
  0x0050:  0000 ecdb 0100 0000 0000 1011 1213 1415
  0x0060:  1617 1819 1a1b 1c1d 1e1f 2021 2223 2425
  0x0070:  2627 2829 2a2b 2c2d 2e2f 3031 3233 3435
  0x0080:  3637

You can see that there are two tlvs there, one for mark (with value 0xc=12) and
one for prio (with value 0x33). In the packets, you can see the on offsets 0x12
and 0x1a, that the size in the tlv is 4 which is the datalen and not 8 which is
the total tlv size.

When I ran the decode without the fix, my kernel went into infinite loop which
was caused by:
 - The loop stopping condition was checking that an unsigned variable is greater
   than zero.
 - The parsing assumes that in the tlv header, the size refers to the whole tlv
   size, but it refers to the size of the data only.

To fix those problems, I fixed the decode side to assume that the tlv->size
refers to the data len and not the whole tlv, and changed the variable to be
signed.

Do you prefer that I will fix the encode side to encode the whole tlv header
size instead of fixing the decode side?

Thanks :)

cheers,
jamal

On 16-09-25 11:55 AM, Yotam Gigi wrote:

quoted

-----Original Message-----
From: Jamal Hadi Salim [mailto:jhs@mojatatu.com]
Sent: Sunday, September 25, 2016 4:46 PM
To: Yotam Gigi <yotam.gi@gmail.com>; davem@davemloft.net;
netdev@vger.kernel.org; Yotam Gigi [off-list ref]
Subject: Re: [PATCH net v2 0/2] Fix tc-ife bugs

On 16-09-25 08:31 AM, Yotam Gigi wrote:

quoted

This patch-set contains two bugfixes in the tc-ife action, one fixing some
random behaviour in encode side, and one fixing the decode side packet
parsing logic.

Yotam Gigi (2):
  act_ife: Fix external mac header on encode
  act_ife: Fix false parsing on decode side

 net/sched/act_ife.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Yotam, did you run the test i proposed? I am worried about the TLV one.
Note:
Encoder includes the length of the TLV header length in the L part.
If you are reaching a conclusion that you need this in the decoding:
+              tlvdata += alen + sizeof(struct meta_tlvhdr);

then very likely whoever is sending that packet is not encoding
correctly.

The one encoding the packets I get is the ife action. You can look at the code:

  int ife_tlv_meta_encode(void *skbdata, u16 attrtype, u16 dlen, const void *dval)
  {
        u32 *tlv = (u32 *)(skbdata);
        u16 totlen = nla_total_size(dlen);      /*alignment + hdr */
        char *dptr = (char *)tlv + NLA_HDRLEN;
        u32 htlv = attrtype << 16 | dlen;

        *tlv = htonl(htlv);
        memset(dptr, 0, totlen - NLA_HDRLEN);
        memcpy(dptr, dval, dlen);

        return totlen;
  }

As you can see, the data that is actually written into the packet is the htlv
var, which has the 'dlen' in it, and not the totlen which corresponds the tlv
header size. You can see that in the following example:

I ran the tc command you proposed:
  $TC filter add dev $ETH parent 1: protocol ip prio 10 \
        u32 match ip protocol 1 0xff flowid 1:2 \
        action skbedit prio 33 \
        action ife encode \
        type 0xDEAD \
        use mark 12 \
        allow prio \
        dst 02:15:15:15:15:15

And this is the packet I got:
  0x0000:  0215 1515 1515 da23 d209 8644 dead 0012
  0x0010:  0001 0004 0000 000c 0003 0004 0000 0033
  0x0020:  fa30 7418 593a da23 d209 8644 0800 4500
  0x0030:  0054 da3c 4000 4001 486a 0c00 0001 0c00
  0x0040:  0002 0800 9fa2 1562 0008 aeec e757 0000
  0x0050:  0000 ecdb 0100 0000 0000 1011 1213 1415
  0x0060:  1617 1819 1a1b 1c1d 1e1f 2021 2223 2425
  0x0070:  2627 2829 2a2b 2c2d 2e2f 3031 3233 3435
  0x0080:  3637

You can see that there are two tlvs there, one for mark (with value 0xc=12) and
one for prio (with value 0x33). In the packets, you can see the on offsets 0x12
and 0x1a, that the size in the tlv is 4 which is the datalen and not 8 which is
the total tlv size.

Indeed.
We cant use totlen because that for non-32-bit aligned data will also
include the padding (I just fixed that in commit:
a823f93750e341bc0d6829a00019435b96830fd4)

When I ran the decode without the fix, my kernel went into infinite loop which
was caused by:
 - The loop stopping condition was checking that an unsigned variable is greater
   than zero.
 - The parsing assumes that in the tlv header, the size refers to the whole tlv
   size, but it refers to the size of the data only.

To fix those problems, I fixed the decode side to assume that the tlv->size
refers to the data len and not the whole tlv, and changed the variable to be
signed.

Do you prefer that I will fix the encode side to encode the whole tlv header
size instead of fixing the decode side?

Yes please - Add NLA_HDRLEN to the dlen on the encode you showed above.

cheers,
jamal

Thanks :)

quoted

cheers,
jamal

On 16-09-25 07:17 PM, Jamal Hadi Salim wrote:
[..]

quoted

Do you prefer that I will fix the encode side to encode the whole tlv
header
size instead of fixing the decode side?

Yes please - Add NLA_HDRLEN to the dlen on the encode you showed above.

And the correct commit it fixes is:
a823f93750e341bc0d6829a00019435b96830fd4

I removed the padding but also the headerlen when i did
that. Tested with tc_index which is a u16 was the mistake i made.

cheers,
jamal

-----Original Message-----
From: Jamal Hadi Salim [mailto:jhs@mojatatu.com]
Sent: Monday, September 26, 2016 3:47 AM
To: Yotam Gigi <redacted>; Yotam Gigi <yotam.gi@gmail.com>;
davem@davemloft.net; netdev@vger.kernel.org
Cc: Jiri Pirko <redacted>; Elad Raz <redacted>; mlxsw
[off-list ref]; Roman Mashak [off-list ref]
Subject: Re: [PATCH net v2 0/2] Fix tc-ife bugs

On 16-09-25 07:17 PM, Jamal Hadi Salim wrote:
[..]

quoted

quoted

Do you prefer that I will fix the encode side to encode the whole tlv
header
size instead of fixing the decode side?

Yes please - Add NLA_HDRLEN to the dlen on the encode you showed above.

OK. Did it and tested it. Everything seems functional.

quoted

And the correct commit it fixes is:
a823f93750e341bc0d6829a00019435b96830fd4

I can't find this commit. Where is it? 

Don't you mean that commit: 28a10c426e81afc88514bca8e73affccf850fdf6
with title: "net sched: fix encoding to use real length"?

I removed the padding but also the headerlen when i did
that. Tested with tc_index which is a u16 was the mistake i made.

cheers,
jamal

On 16-09-26 03:01 AM, Yotam Gigi wrote:

quoted

-----Original Message-----
From: Jamal Hadi Salim [mailto:jhs@mojatatu.com]
Sent: Monday, September 26, 2016 3:47 AM
To: Yotam Gigi <redacted>; Yotam Gigi <yotam.gi@gmail.com>;

[..]

quoted

quoted

Yes please - Add NLA_HDRLEN to the dlen on the encode you showed above.

OK. Did it and tested it. Everything seems functional.

Thank you. Please resend with this change and add my ACK to both
(I tested the first one)

quoted

quoted

And the correct commit it fixes is:
a823f93750e341bc0d6829a00019435b96830fd4

I can't find this commit. Where is it?

Don't you mean that commit: 28a10c426e81afc88514bca8e73affccf850fdf6
with title: "net sched: fix encoding to use real length"?

Sorry, yes that patch.

cheers,
jamal