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

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

[Yotam Gigi]

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

[PATCH net 1/2] act_ife: Fix external mac header on encode

[Yotam Gigi]

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 <yotamg@mellanox.com>
---
 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 27b19ca..7f71a3d 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -758,8 +758,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, reserve);
 	if (unlikely(err)) {
 		ife->tcf_qstats.drops++;
@@ -768,6 +766,7 @@ static int tcf_ife_encode(struct sk_buff *skb, const struct tc_action *a,
 	}
 
 	__skb_push(skb, total_push);
+	iethh = (struct ethhdr *)(skb->data + hdrm);
 	memcpy(skb->data, iethh, skb->mac_len);
 	skb_reset_mac_header(skb);
 	skboff += skb->mac_len;
-- 
2.4.11

[PATCH net 2/2] act_ife: Fix false parsing on decode side

[Yotam Gigi]

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 <yotamg@mellanox.com>
---
 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 7f71a3d..4786b28 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -627,7 +627,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);
@@ -668,8 +668,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

Re: [PATCH net 1/2] act_ife: Fix external mac header on encode

[Jamal Hadi Salim]

On 16-09-22 08:55 AM, Yotam Gigi wrote:
> 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.


Since this depends on the previous patch, can you double check for me? I 
will test later, but here's a very simple test case:

sudo $TC qdisc del dev $ETH root handle 1: prio
sudo $TC qdisc add dev $ETH root handle 1: prio

#set mark of decimal 17 and allow sending out
sudo $TC filter add dev $ETH parent 1: protocol ip prio 10 \
u32 match ip protocol 1 0xff flowid 1:2 \
action skbedit mark 17 \
action ife encode \
type 0xDEAD \
allow mark \
dst 02:15:15:15:15:15

I am not going to comment on your other patch, but i suggest you
test with with this (encoding at least two TLVs):

sudo $TC qdisc del dev $ETH root handle 1: prio
sudo $TC qdisc add dev $ETH root handle 1: prio
#Override mark and send prio of 0x33 (unfortunately
#skbedit is not very consistent 33 means 0x33)
sudo $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


cheers,
jamal

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

[Yotam Gigi]

>-----Original Message-----
>From: Yotam Gigi
>Sent: Thursday, September 22, 2016 3:55 PM
>To: jhs@mojatatu.com; davem@davemloft.net; netdev@vger.kernel.org
>Cc: Yotam Gigi <yotamg@mellanox.com>
>Subject: [PATCH net 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.

I have to rebase the patches. I will send v2 soon.

Thanks!

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