[PATCH net-next 0/3] fix use-after-free bugs in skb list processing

[PATCH net-next 0/3] fix use-after-free bugs in skb list processing

[Edward Cree]

A couple of bugs in skb list handling were spotted by Dan Carpenter, with
 the help of Smatch; following up on them I found a couple more similar
 cases.  This series fixes them by changing the relevant loops to use the
 dequeue-enqueue model (rather than in-place list modification), and then
 adds a list.h helper macro to refactor code using the dequeue-enqueue
 model.

Edward Cree (3):
  net: core: fix uses-after-free in list processing
  netfilter: fix use-after-free in NF_HOOK_LIST
  net: refactor dequeue-model list processing

 include/linux/list.h      | 15 +++++++++++++++
 include/linux/netfilter.h | 16 +++++++++-------
 net/core/dev.c            | 23 +++++++++++++----------
 net/ipv4/ip_input.c       | 10 ++++------
 net/ipv6/ip6_input.c      | 10 ++++------
 5 files changed, 45 insertions(+), 29 deletions(-)

[PATCH net-next 1/3] net: core: fix uses-after-free in list processing

[Edward Cree]

In netif_receive_skb_list_internal(), all of skb_defer_rx_timestamp(),
 do_xdp_generic() and enqueue_to_backlog() can lead to kfree(skb).  Thus,
 we cannot wait until after they return to remove the skb from the list;
 instead, we remove it first and, in the pass case, add it to a sublist
 afterwards.
In the case of enqueue_to_backlog() we have already decided not to pass
 when we call the function, so we do not need a sublist.

Fixes: 7da517a3bc52 ("net: core: Another step of skb receive list processing")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 net/core/dev.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 89825c1eccdc..ce4583564e00 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4982,25 +4982,30 @@ static void netif_receive_skb_list_internal(struct list_head *head)
 {
 	struct bpf_prog *xdp_prog = NULL;
 	struct sk_buff *skb, *next;
+	struct list_head sublist;
 
+	INIT_LIST_HEAD(&sublist);
 	list_for_each_entry_safe(skb, next, head, list) {
 		net_timestamp_check(netdev_tstamp_prequeue, skb);
-		if (skb_defer_rx_timestamp(skb))
-			/* Handled, remove from list */
-			list_del(&skb->list);
+		list_del(&skb->list);
+		if (!skb_defer_rx_timestamp(skb))
+			list_add_tail(&skb->list, &sublist);
 	}
+	list_splice_init(&sublist, head);
 
 	if (static_branch_unlikely(&generic_xdp_needed_key)) {
 		preempt_disable();
 		rcu_read_lock();
 		list_for_each_entry_safe(skb, next, head, list) {
 			xdp_prog = rcu_dereference(skb->dev->xdp_prog);
-			if (do_xdp_generic(xdp_prog, skb) != XDP_PASS)
-				/* Dropped, remove from list */
-				list_del(&skb->list);
+			list_del(&skb->list);
+			if (do_xdp_generic(xdp_prog, skb) == XDP_PASS)
+				list_add_tail(&skb->list, &sublist);
 		}
 		rcu_read_unlock();
 		preempt_enable();
+		/* Put passed packets back on main list */
+		list_splice_init(&sublist, head);
 	}
 
 	rcu_read_lock();
@@ -5011,9 +5016,9 @@ static void netif_receive_skb_list_internal(struct list_head *head)
 			int cpu = get_rps_cpu(skb->dev, skb, &rflow);
 
 			if (cpu >= 0) {
-				enqueue_to_backlog(skb, cpu, &rflow->last_qtail);
-				/* Handled, remove from list */
+				/* Will be handled, remove from list */
 				list_del(&skb->list);
+				enqueue_to_backlog(skb, cpu, &rflow->last_qtail);
 			}
 		}
 	}

[PATCH net-next 2/3] netfilter: fix use-after-free in NF_HOOK_LIST

[Edward Cree]

nf_hook() can free the skb, so we need to remove it from the list before
 calling, and add passed skbs to a sublist afterwards.

Fixes: 17266ee93984 ("net: ipv4: listified version of ip_rcv")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 include/linux/netfilter.h | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
index 5a5e0a2ab2a3..23b48de8c2e2 100644
--- a/include/linux/netfilter.h
+++ b/include/linux/netfilter.h
@@ -294,12 +294,16 @@ NF_HOOK_LIST(uint8_t pf, unsigned int hook, struct net *net, struct sock *sk,
 	     int (*okfn)(struct net *, struct sock *, struct sk_buff *))
 {
 	struct sk_buff *skb, *next;
+	struct list_head sublist;
 
+	INIT_LIST_HEAD(&sublist);
 	list_for_each_entry_safe(skb, next, head, list) {
-		int ret = nf_hook(pf, hook, net, sk, skb, in, out, okfn);
-		if (ret != 1)
-			list_del(&skb->list);
+		list_del(&skb->list);
+		if (nf_hook(pf, hook, net, sk, skb, in, out, okfn) == 1)
+			list_add_tail(&skb->list, &sublist);
 	}
+	/* Put passed packets back on main list */
+	list_splice(&sublist, head);
 }
 
 /* Call setsockopt() */

[PATCH net-next 3/3] net: refactor dequeue-model list processing

[Edward Cree]

New macro list_for_each_entry_dequeue loops over a list by popping entries
 from the head, allowing a more concise expression of the dequeue-enqueue
 model of list processing and avoiding the need for a 'next' pointer (as
 used in list_for_each_entry_safe).

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 include/linux/list.h      | 15 +++++++++++++++
 include/linux/netfilter.h |  6 ++----
 net/core/dev.c            |  6 ++----
 net/ipv4/ip_input.c       | 10 ++++------
 net/ipv6/ip6_input.c      | 10 ++++------
 5 files changed, 27 insertions(+), 20 deletions(-)

diff --git a/include/linux/list.h b/include/linux/list.h
index de04cc5ed536..150751d03441 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -645,6 +645,21 @@ static inline void list_splice_tail_init(struct list_head *list,
 #define list_safe_reset_next(pos, n, member)				\
 	n = list_next_entry(pos, member)
 
+/**
+ * list_for_each_entry_dequeue - iterate over list by removing entries
+ * @pos:	the type * to use as a loop cursor.
+ * @head:	the head for your list.
+ * @member:	the name of the list_head within the struct.
+ *
+ * Iterate over list of given type, removing each entry from the list before
+ * running the loop body.  The loop will run until the list is empty, so
+ * adding an entry back onto the list in the loop body will requeue it.
+ */
+#define list_for_each_entry_dequeue(pos, head, member)			\
+	while (!list_empty(head) &&					\
+	       (pos = list_first_entry(head, typeof(*pos), member)) &&	\
+	       (list_del(&pos->member), true))
+
 /*
  * Double linked lists with a single pointer list head.
  * Mostly useful for hash tables where the two pointer list head is
diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
index 23b48de8c2e2..f9a037eb053d 100644
--- a/include/linux/netfilter.h
+++ b/include/linux/netfilter.h
@@ -293,15 +293,13 @@ NF_HOOK_LIST(uint8_t pf, unsigned int hook, struct net *net, struct sock *sk,
 	     struct list_head *head, struct net_device *in, struct net_device *out,
 	     int (*okfn)(struct net *, struct sock *, struct sk_buff *))
 {
-	struct sk_buff *skb, *next;
 	struct list_head sublist;
+	struct sk_buff *skb;
 
 	INIT_LIST_HEAD(&sublist);
-	list_for_each_entry_safe(skb, next, head, list) {
-		list_del(&skb->list);
+	list_for_each_entry_dequeue(skb, head, list)
 		if (nf_hook(pf, hook, net, sk, skb, in, out, okfn) == 1)
 			list_add_tail(&skb->list, &sublist);
-	}
 	/* Put passed packets back on main list */
 	list_splice(&sublist, head);
 }
diff --git a/net/core/dev.c b/net/core/dev.c
index ce4583564e00..68055f012d13 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4985,9 +4985,8 @@ static void netif_receive_skb_list_internal(struct list_head *head)
 	struct list_head sublist;
 
 	INIT_LIST_HEAD(&sublist);
-	list_for_each_entry_safe(skb, next, head, list) {
+	list_for_each_entry_dequeue(skb, head, list) {
 		net_timestamp_check(netdev_tstamp_prequeue, skb);
-		list_del(&skb->list);
 		if (!skb_defer_rx_timestamp(skb))
 			list_add_tail(&skb->list, &sublist);
 	}
@@ -4996,9 +4995,8 @@ static void netif_receive_skb_list_internal(struct list_head *head)
 	if (static_branch_unlikely(&generic_xdp_needed_key)) {
 		preempt_disable();
 		rcu_read_lock();
-		list_for_each_entry_safe(skb, next, head, list) {
+		list_for_each_entry_dequeue(skb, head, list) {
 			xdp_prog = rcu_dereference(skb->dev->xdp_prog);
-			list_del(&skb->list);
 			if (do_xdp_generic(xdp_prog, skb) == XDP_PASS)
 				list_add_tail(&skb->list, &sublist);
 		}
diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
index 1a3b6f32b1c9..26285a24c067 100644
--- a/net/ipv4/ip_input.c
+++ b/net/ipv4/ip_input.c
@@ -538,14 +538,13 @@ static void ip_list_rcv_finish(struct net *net, struct sock *sk,
 			       struct list_head *head)
 {
 	struct dst_entry *curr_dst = NULL;
-	struct sk_buff *skb, *next;
 	struct list_head sublist;
+	struct sk_buff *skb;
 
 	INIT_LIST_HEAD(&sublist);
-	list_for_each_entry_safe(skb, next, head, list) {
+	list_for_each_entry_dequeue(skb, head, list) {
 		struct dst_entry *dst;
 
-		list_del(&skb->list);
 		/* if ingress device is enslaved to an L3 master device pass the
 		 * skb to its handler for processing
 		 */
@@ -584,15 +583,14 @@ void ip_list_rcv(struct list_head *head, struct packet_type *pt,
 {
 	struct net_device *curr_dev = NULL;
 	struct net *curr_net = NULL;
-	struct sk_buff *skb, *next;
 	struct list_head sublist;
+	struct sk_buff *skb;
 
 	INIT_LIST_HEAD(&sublist);
-	list_for_each_entry_safe(skb, next, head, list) {
+	list_for_each_entry_dequeue(skb, head, list) {
 		struct net_device *dev = skb->dev;
 		struct net *net = dev_net(dev);
 
-		list_del(&skb->list);
 		skb = ip_rcv_core(skb, net);
 		if (skb == NULL)
 			continue;
diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
index 6242682be876..65344cef0edd 100644
--- a/net/ipv6/ip6_input.c
+++ b/net/ipv6/ip6_input.c
@@ -88,14 +88,13 @@ static void ip6_list_rcv_finish(struct net *net, struct sock *sk,
 				struct list_head *head)
 {
 	struct dst_entry *curr_dst = NULL;
-	struct sk_buff *skb, *next;
 	struct list_head sublist;
+	struct sk_buff *skb;
 
 	INIT_LIST_HEAD(&sublist);
-	list_for_each_entry_safe(skb, next, head, list) {
+	list_for_each_entry_dequeue(skb, head, list) {
 		struct dst_entry *dst;
 
-		list_del(&skb->list);
 		/* if ingress device is enslaved to an L3 master device pass the
 		 * skb to its handler for processing
 		 */
@@ -287,15 +286,14 @@ void ipv6_list_rcv(struct list_head *head, struct packet_type *pt,
 {
 	struct net_device *curr_dev = NULL;
 	struct net *curr_net = NULL;
-	struct sk_buff *skb, *next;
 	struct list_head sublist;
+	struct sk_buff *skb;
 
 	INIT_LIST_HEAD(&sublist);
-	list_for_each_entry_safe(skb, next, head, list) {
+	list_for_each_entry_dequeue(skb, head, list) {
 		struct net_device *dev = skb->dev;
 		struct net *net = dev_net(dev);
 
-		list_del(&skb->list);
 		skb = ip6_rcv_core(skb, dev, net);
 		if (skb == NULL)
 			continue;

Re: [PATCH net-next 0/3] fix use-after-free bugs in skb list processing

[David Miller]

From: Edward Cree <ecree@solarflare.com>
Date: Fri, 6 Jul 2018 18:01:04 +0100

> A couple of bugs in skb list handling were spotted by Dan Carpenter, with
>  the help of Smatch; following up on them I found a couple more similar
>  cases.  This series fixes them by changing the relevant loops to use the
>  dequeue-enqueue model (rather than in-place list modification), and then
>  adds a list.h helper macro to refactor code using the dequeue-enqueue
>  model.

Edward, I think the helper added in patch #3 is not all that great.

It can be even more confusing to understand what the code is doing in
my opinion.  I am pretty sure more hackers would need to go and
read the macro definition in list.h before understanding what the
read side effects of it are.

Would you please resubmit this series including only patches #1
and #2?

Thank you.