netdev - [RFC net-next v2 1/1] net: sched: protect against loops in TC filter hooks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Thu,  6 Jun 2019 13:49:34 +0100
From:   John Hurley <john.hurley@...ronome.com>
To:     netdev@...r.kernel.org
Cc:     simon.horman@...ronome.com, jakub.kicinski@...ronome.com,
        jhs@...atatu.com, fw@...len.de, oss-drivers@...ronome.com,
        John Hurley <john.hurley@...ronome.com>
Subject: [RFC net-next v2 1/1] net: sched: protect against loops in TC filter hooks

TC hooks allow the application of filters and actions to packets at both
ingress and egress of the network stack. It is possible, with poor
configuration, that this can produce loops whereby an ingress hook calls
a mirred egress action that has an egress hook that redirects back to
the first ingress etc. The TC core classifier protects against loops when
doing reclassifies but there is no protection against a packet looping
between multiple hooks. This can lead to stack overflow panics among other
things.

Previous versions of the kernel (<4.2) had a TTL count in the tc_verd skb
member that protected against loops. This was removed and the tc_verd
variable replaced by bit fields.

Extend the TC fields in the skb with an additional 2 bits to store the TC
hop count. This should use existing allocated memory in the skb.

Add the checking and setting of the new hop count to the act_mirred file
given that it is the source of the loops. This means that the code
additions are not in the main datapath.

v1->v2
- change from per cpu counter to per skb tracking (Jamal)
- move check/update from fast path to act_mirred (Daniel)

Signed-off-by: John Hurley <john.hurley@...ronome.com>
---
 include/linux/skbuff.h | 2 ++
 net/sched/act_mirred.c | 9 +++++++++
 2 files changed, 11 insertions(+)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 2ee5e63..f0dbc5b 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -645,6 +645,7 @@ typedef unsigned char *sk_buff_data_t;
  *	@tc_at_ingress: used within tc_classify to distinguish in/egress
  *	@tc_redirected: packet was redirected by a tc action
  *	@tc_from_ingress: if tc_redirected, tc_at_ingress at time of redirect
+ *	@tc_hop_count: hop counter to prevent packet loops
  *	@peeked: this packet has been seen already, so stats have been
  *		done for it, don't do them again
  *	@nf_trace: netfilter packet trace flag
@@ -827,6 +828,7 @@ struct sk_buff {
 	__u8			tc_at_ingress:1;
 	__u8			tc_redirected:1;
 	__u8			tc_from_ingress:1;
+	__u8			tc_hop_count:2;
 #endif
 #ifdef CONFIG_TLS_DEVICE
 	__u8			decrypted:1;
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index c329390..6cf7f90 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -29,6 +29,8 @@
 #include <linux/tc_act/tc_mirred.h>
 #include <net/tc_act/tc_mirred.h>
 
+#define MAX_HOP_COUNT	3
+
 static LIST_HEAD(mirred_list);
 static DEFINE_SPINLOCK(mirred_list_lock);
 
@@ -222,6 +224,12 @@ static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a,
 	int m_eaction;
 	int mac_len;
 
+	if (unlikely(skb->tc_hop_count == MAX_HOP_COUNT)) {
+		net_warn_ratelimited("Packet at %s exceeded max TC hop count\n",
+				     netdev_name(skb->dev));
+		return TC_ACT_SHOT;
+	}
+
 	tcf_lastuse_update(&m->tcf_tm);
 	bstats_cpu_update(this_cpu_ptr(m->common.cpu_bstats), skb);
 
@@ -271,6 +279,7 @@ static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a,
 
 	skb2->skb_iif = skb->dev->ifindex;
 	skb2->dev = dev;
+	skb2->tc_hop_count++;
 
 	/* mirror is always swallowed */
 	if (is_redirect) {
-- 
2.7.4