[PATCH net] net/sched: act_pedit: fix 'ex munge' on network header in case of QinQ packet

[PATCH net] net/sched: act_pedit: fix 'ex munge' on network header in case of QinQ packet

[Davide Caratti]

Like it has been done in commit 2ecba2d1e45b ("net: sched: act_csum: Fix
csum calc for tagged packets"), also 'pedit' needs to adjust the network
offset when multiple tags are present in the packets: otherwise wrong IP
headers (but good checksums) can be observed with the following command:

 # tc filter add dev test0 parent ffff: protocol 802.1Q flower \
   vlan_ethtype ipv4 action \
   pedit ex munge ip ttl set 10 pipe \
   csum ip and icmp pipe \
   mirred egress redirect dev test1

Reported-by: Li Shuang <shuali@redhat.com>
Fixes: d8b9605d2697 ("net: sched: fix skb->protocol use in case of accelerated vlan path")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
 net/sched/act_pedit.c | 38 ++++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index d790c02b9c6c..25dcf9ee092e 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -277,9 +277,12 @@ static bool offset_valid(struct sk_buff *skb, int offset)
 }
 
 static int pedit_skb_hdr_offset(struct sk_buff *skb,
-				enum pedit_header_type htype, int *hoffset)
+				enum pedit_header_type htype, int *hoffset,
+				unsigned int *vlan_hdr_count)
 {
+	bool orig_vlan_tag_present = false;
 	int ret = -EINVAL;
+	__be16 protocol;
 
 	switch (htype) {
 	case TCA_PEDIT_KEY_EX_HDR_TYPE_ETH:
@@ -291,8 +294,29 @@ static int pedit_skb_hdr_offset(struct sk_buff *skb,
 	case TCA_PEDIT_KEY_EX_HDR_TYPE_NETWORK:
 	case TCA_PEDIT_KEY_EX_HDR_TYPE_IP4:
 	case TCA_PEDIT_KEY_EX_HDR_TYPE_IP6:
-		*hoffset = skb_network_offset(skb);
-		ret = 0;
+		protocol = tc_skb_protocol(skb);
+again:
+		switch (protocol) {
+		case cpu_to_be16(ETH_P_8021AD): /* fall through */
+		case cpu_to_be16(ETH_P_8021Q):
+			if (skb_vlan_tag_present(skb) &&
+			    !orig_vlan_tag_present) {
+				protocol = skb->protocol;
+				orig_vlan_tag_present = true;
+			} else {
+				struct vlan_hdr *vlan;
+
+				vlan = (struct vlan_hdr *)skb->data;
+				protocol = vlan->h_vlan_encapsulated_proto;
+				skb_pull(skb, VLAN_HLEN);
+				skb_reset_network_header(skb);
+				(*vlan_hdr_count)++;
+			}
+			goto again;
+		default:
+			*hoffset = skb_network_offset(skb);
+			ret = 0;
+		}
 		break;
 	case TCA_PEDIT_KEY_EX_HDR_TYPE_TCP:
 	case TCA_PEDIT_KEY_EX_HDR_TYPE_UDP:
@@ -313,6 +337,7 @@ static int tcf_pedit_act(struct sk_buff *skb, const struct tc_action *a,
 			 struct tcf_result *res)
 {
 	struct tcf_pedit *p = to_pedit(a);
+	unsigned int vlan_hdr_count = 0;
 	int i;
 
 	if (skb_unclone(skb, GFP_ATOMIC))
@@ -343,7 +368,8 @@ static int tcf_pedit_act(struct sk_buff *skb, const struct tc_action *a,
 				tkey_ex++;
 			}
 
-			rc = pedit_skb_hdr_offset(skb, htype, &hoffset);
+			rc = pedit_skb_hdr_offset(skb, htype, &hoffset,
+						  &vlan_hdr_count);
 			if (rc) {
 				pr_info("tc action pedit bad header type specified (0x%x)\n",
 					htype);
@@ -407,6 +433,10 @@ static int tcf_pedit_act(struct sk_buff *skb, const struct tc_action *a,
 bad:
 	p->tcf_qstats.overlimits++;
 done:
+	while (vlan_hdr_count--) {
+		skb_push(skb, VLAN_HLEN);
+		skb_reset_network_header(skb);
+	}
 	bstats_update(&p->tcf_bstats, skb);
 	spin_unlock(&p->tcf_lock);
 	return p->tcf_action;
-- 
2.20.1

Re: [PATCH net] net/sched: act_pedit: fix 'ex munge' on network header in case of QinQ packet

[Eric Dumazet]

On 5/28/19 1:50 PM, Davide Caratti wrote:
> Like it has been done in commit 2ecba2d1e45b ("net: sched: act_csum: Fix
> csum calc for tagged packets"), also 'pedit' needs to adjust the network
> offset when multiple tags are present in the packets: otherwise wrong IP
> headers (but good checksums) can be observed with the following command:

...

> +again:
> +		switch (protocol) {
> +		case cpu_to_be16(ETH_P_8021AD): /* fall through */
> +		case cpu_to_be16(ETH_P_8021Q):
> +			if (skb_vlan_tag_present(skb) &&
> +			    !orig_vlan_tag_present) {
> +				protocol = skb->protocol;
> +				orig_vlan_tag_present = true;
> +			} else {
> +				struct vlan_hdr *vlan;
> +
> +				vlan = (struct vlan_hdr *)skb->data;
> +				protocol = vlan->h_vlan_encapsulated_proto;
> +				skb_pull(skb, VLAN_HLEN);
> +				skb_reset_network_header(skb);
> +				(*vlan_hdr_count)++;
> +			}
> +			goto again;

What prevents this loop to access data not yet in skb->head ?

skb_header_pointer() (or pskb_may_pull()) seems needed.

Re: [PATCH net] net/sched: act_pedit: fix 'ex munge' on network header in case of QinQ packet

[Davide Caratti]

hi Eric, thanks for looking at this!

On Tue, 2019-05-28 at 14:02 -0700, Eric Dumazet wrote:
> 
> On 5/28/19 1:50 PM, Davide Caratti wrote:
> > +				vlan = (struct vlan_hdr *)skb->data;
> > +				protocol = vlan->h_vlan_encapsulated_proto;
> > +				skb_pull(skb, VLAN_HLEN);
> > +				skb_reset_network_header(skb);
> > +				(*vlan_hdr_count)++;
> > +			}
> > +			goto again;
> 
> What prevents this loop to access data not yet in skb->head ?

just luck.

'pedit' does skb_header_pointer() later on, when it writes in the packet.
But indeed, there is no guarantee that all the nested vlan headers are in
the linear area of the packet. 

Looking at 2ecba2d1e45b and current act_csum.c, probably also tcf_csum_act()
needs the same check: I will try a patch for that tomorrow.

> skb_header_pointer() (or pskb_may_pull()) seems needed.

pskb_may_pull(), with proper error handling, seems better to me. 
regards,
-- 
davide