[net-next 0/2] net: sched: cls-flower: add support for port-based fragment filtering

[net-next 0/2] net: sched: cls-flower: add support for port-based fragment filtering

[Jason Baron]

Port based allow rules must currently allow all fragments since the
port number is not included in the 1rst fragment. We want to restrict
allowing all fragments by inclucding the port number in the 1rst
fragments.

For example, we can now allow fragments for only port 80 via:

# tc filter add dev $DEVICE parent ffff: priority 1 protocol ipv4 flower
  ip_proto tcp dst_port 80 action pass
# tc filter add dev $DEVICE parent ffff: priority 2 protocol ipv4 flower
  ip_flags frag/nofirstfrag action pass

The first patch includes ports for 1rst fragments.
The second patch adds test cases, demonstrating the new behavior.

Jason Baron (2):
  net: sched: cls-flower: include ports in 1rst fragment
  selftests: tc_flower: add destination port tests

 net/core/flow_dissector.c                          |  4 +-
 net/sched/cls_flower.c                             |  3 +-
 .../testing/selftests/net/forwarding/tc_flower.sh  | 73 +++++++++++++++++++++-
 3 files changed, 77 insertions(+), 3 deletions(-)

-- 
2.7.4

[net-next 1/2] net: sched: cls-flower: include ports in 1rst fragment

[Jason Baron]

cls-flower does not include the ports in the 1rst ipv4/6
fragments. Thus, if fragments are to be allowed, then port based
rules have no choice but to allow all fragments destined to all
ports. Let's include ports in the 1rst fragment. Thus, fragmented
packets can be allowed/blocked by adding a rule which
allows/blocks specific ports and allows all non-first ip fragments
(via setting ip_flags to frag/nofirstfrag).


Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Jason Baron <jbaron@akamai.com>
---
 net/core/flow_dissector.c | 4 +++-
 net/sched/cls_flower.c    | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 0aeb335..bfd554e 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -1446,7 +1446,9 @@ bool __skb_flow_dissect(const struct net *net,
 		break;
 	}
 
-	if (!(key_control->flags & FLOW_DIS_IS_FRAGMENT))
+	if (!(key_control->flags & FLOW_DIS_IS_FRAGMENT) ||
+	    ((key_control->flags & FLOW_DIS_FIRST_FRAG) &&
+	     (FLOW_DISSECTOR_F_PARSE_1ST_FRAG & flags)))
 		__skb_flow_dissect_ports(skb, flow_dissector, target_container,
 					 data, nhoff, ip_proto, hlen);
 
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 96f5999..e16227a 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -317,7 +317,8 @@ static int fl_classify(struct sk_buff *skb, const struct tcf_proto *tp,
 		skb_flow_dissect_ct(skb, &mask->dissector, &skb_key,
 				    fl_ct_info_to_flower_map,
 				    ARRAY_SIZE(fl_ct_info_to_flower_map));
-		skb_flow_dissect(skb, &mask->dissector, &skb_key, 0);
+		skb_flow_dissect(skb, &mask->dissector, &skb_key,
+				 FLOW_DISSECTOR_F_PARSE_1ST_FRAG);
 
 		fl_set_masked_key(&skb_mkey, &skb_key, mask);
 
-- 
2.7.4

[net-next 2/2] selftests: tc_flower: add destination port tests

[Jason Baron]

Verify that tc flower can match on destination port for
udp/tcp for both non-fragment and first fragment cases.

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Jason Baron <jbaron@akamai.com>
---
 .../testing/selftests/net/forwarding/tc_flower.sh  | 73 +++++++++++++++++++++-
 1 file changed, 72 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/forwarding/tc_flower.sh b/tools/testing/selftests/net/forwarding/tc_flower.sh
index 058c746..6424084 100755
--- a/tools/testing/selftests/net/forwarding/tc_flower.sh
+++ b/tools/testing/selftests/net/forwarding/tc_flower.sh
@@ -3,7 +3,8 @@
 
 ALL_TESTS="match_dst_mac_test match_src_mac_test match_dst_ip_test \
 	match_src_ip_test match_ip_flags_test match_pcp_test match_vlan_test \
-	match_ip_tos_test match_indev_test"
+	match_ip_tos_test match_indev_test match_dst_port_tcp_test \
+	match_dst_port_udp_test"
 NUM_NETIFS=2
 source tc_common.sh
 source lib.sh
@@ -334,6 +335,76 @@ match_indev_test()
 	log_test "indev match ($tcflags)"
 }
 
+match_dst_port_test_helper()
+{
+	RET=0
+	proto="$1"
+
+	tc filter add dev $h2 ingress protocol ip pref 1 handle 101 flower \
+		$tcflags ip_proto $proto dst_port 80 action drop
+	tc filter add dev $h2 ingress protocol ip pref 2 handle 102 flower \
+		$tcflags action drop
+
+	# port match, no fragment
+	$MZ $h1 -c 1 -p 64 -a $h1mac -b $h2mac -A 192.0.2.1 -B 192.0.2.2 \
+		-t $proto dp=80 -q
+
+	tc_check_packets "dev $h2 ingress" 101 1
+	check_err $? "Did not match on correct filter"
+
+	tc_check_packets "dev $h2 ingress" 102 1
+	check_fail $? "Matched on a wrong filter"
+
+	# port mis-match, no fragment
+	$MZ $h1 -c 1 -p 64 -a $h1mac -b $h2mac -A 192.0.2.1 -B 192.0.2.2 \
+		-t $proto dp=81 -q
+
+	tc_check_packets "dev $h2 ingress" 101 2
+	check_fail $? "Matched on a wrong filter"
+
+	tc_check_packets "dev $h2 ingress" 102 1
+	check_err $? "Did not match on correct filter"
+
+	# port match, first fragment
+	$MZ $h1 -c 1 -p 64 -a $h1mac -b $h2mac -A 192.0.2.1 -B 192.0.2.2 \
+		-t $proto dp=80,frag=0,mf -q
+
+	tc_check_packets "dev $h2 ingress" 101 2
+	check_err $? "Did not match on correct filter"
+
+	tc_check_packets "dev $h2 ingress" 102 2
+	check_fail $? "Matched on a wrong filter"
+
+	# port match, non-first fragment
+	$MZ $h1 -c 1 -p 64 -a $h1mac -b $h2mac -A 192.0.2.1 -B 192.0.2.2 \
+		-t $proto dp=80,frag=2,mf -q
+
+	tc_check_packets "dev $h2 ingress" 101 3
+	check_fail $? "Matched on a wrong filter"
+
+	tc_check_packets "dev $h2 ingress" 102 2
+	check_err $? "Did not match on correct filter"
+
+	tc filter del dev $h2 ingress protocol ip pref 2 handle 102 flower
+	tc filter del dev $h2 ingress protocol ip pref 1 handle 101 flower
+}
+
+match_dst_port_tcp_test()
+{
+	RET=0
+
+	match_dst_port_test_helper "tcp"
+	log_test "dst_port_tcp match ($tcflags)"
+}
+
+match_dst_port_udp_test()
+{
+	RET=0
+
+	match_dst_port_test_helper "udp"
+	log_test "dst_port_udp match ($tcflags)"
+}
+
 setup_prepare()
 {
 	h1=${NETIFS[p1]}
-- 
2.7.4

Re: [net-next 0/2] net: sched: cls-flower: add support for port-based fragment filtering

[David Miller]

From: Jason Baron <jbaron@akamai.com>
Date: Wed, 27 May 2020 16:25:28 -0400

> Port based allow rules must currently allow all fragments since the
> port number is not included in the 1rst fragment. We want to restrict
> allowing all fragments by inclucding the port number in the 1rst
> fragments.
> 
> For example, we can now allow fragments for only port 80 via:
> 
> # tc filter add dev $DEVICE parent ffff: priority 1 protocol ipv4 flower
>   ip_proto tcp dst_port 80 action pass
> # tc filter add dev $DEVICE parent ffff: priority 2 protocol ipv4 flower
>   ip_flags frag/nofirstfrag action pass
> 
> The first patch includes ports for 1rst fragments.
> The second patch adds test cases, demonstrating the new behavior.

But this is only going to drop the first frag right?

Unless there is logic to toss the rest of the frags this seems
extremely hackish as best.

I don't want to apply this as-is, it's a short sighted design
as far as I am concerned.

Re: [net-next 0/2] net: sched: cls-flower: add support for port-based fragment filtering

[Jason Baron]

On 5/29/20 7:52 PM, David Miller wrote:
> From: Jason Baron <jbaron@akamai.com>
> Date: Wed, 27 May 2020 16:25:28 -0400
> 
>> Port based allow rules must currently allow all fragments since the
>> port number is not included in the 1rst fragment. We want to restrict
>> allowing all fragments by inclucding the port number in the 1rst
>> fragments.
>>
>> For example, we can now allow fragments for only port 80 via:
>>
>> # tc filter add dev $DEVICE parent ffff: priority 1 protocol ipv4 flower
>>   ip_proto tcp dst_port 80 action pass
>> # tc filter add dev $DEVICE parent ffff: priority 2 protocol ipv4 flower
>>   ip_flags frag/nofirstfrag action pass
>>
>> The first patch includes ports for 1rst fragments.
>> The second patch adds test cases, demonstrating the new behavior.
> 
> But this is only going to drop the first frag right?
>

Yes, only the first frag is dropped, as subsequent packets wouldn't
have the port #. This will prevent re-assembly since the first fragment
is missing. Currently, drop rules on ports will just allow all fragments
through, so this seems to me to be easy to subvert.

> Unless there is logic to toss the rest of the frags this seems
> extremely hackish as best.
> 
> I don't want to apply this as-is, it's a short sighted design
> as far as I am concerned.
> 

The use-case we are using this patch for is to allow specific ports
through the firewall without having to allow all fragments. So the
rules look like:

1) Allow port 80 including first frags

# tc filter add dev $DEVICE parent ffff: priority 1 protocol ipv4 flower
  ip_proto tcp dst_port 80 action pass

2) Allow all non-first frags

# tc filter add dev $DEVICE parent ffff: priority 2 protocol ipv4 flower
  ip_flags frag/nofirstfrag action pass

3) Drop everything left

# tc filter add dev $DEVICE parent ffff: priority 3 flower action drop


This allows one to process fragments without allowing all fragments to
all ports.

I'm certainly open to other ideas on how to improve this.

Thanks,

-Jason