[PATCH net] af_key: unconditionally clone on broadcast

[PATCH net] af_key: unconditionally clone on broadcast

[Sean Tranchetti]

Attempting to avoid cloning the skb when broadcasting by inflating
the refcount with sock_hold/sock_put while under RCU lock is dangerous
and violates RCU principles. It leads to subtle race conditions when
attempting to free the SKB, as we may reference sockets that have
already been freed by the stack.

Unable to handle kernel paging request at virtual address 6b6b6b6b6b6c4b
[006b6b6b6b6b6c4b] address between user and kernel address ranges
Internal error: Oops: 96000004 [#1] PREEMPT SMP
task: fffffff78f65b380 task.stack: ffffff8049a88000
pc : sock_rfree+0x38/0x6c
lr : skb_release_head_state+0x6c/0xcc
Process repro (pid: 7117, stack limit = 0xffffff8049a88000)
Call trace:
	sock_rfree+0x38/0x6c
	skb_release_head_state+0x6c/0xcc
	skb_release_all+0x1c/0x38
	__kfree_skb+0x1c/0x30
	kfree_skb+0xd0/0xf4
	pfkey_broadcast+0x14c/0x18c
	pfkey_sendmsg+0x1d8/0x408
	sock_sendmsg+0x44/0x60
	___sys_sendmsg+0x1d0/0x2a8
	__sys_sendmsg+0x64/0xb4
	SyS_sendmsg+0x34/0x4c
	el0_svc_naked+0x34/0x38
Kernel panic - not syncing: Fatal exception

Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Sean Tranchetti <stranche@codeaurora.org>
---
Realized I never actually sent this patch out after testing the changes
Eric recommended. Whoops. Better late then never, I suppose...

 net/key/af_key.c | 40 +++++++++++++++-------------------------
 1 file changed, 15 insertions(+), 25 deletions(-)

diff --git a/net/key/af_key.c b/net/key/af_key.c
index 655c787..e800891 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -196,30 +196,22 @@ static int pfkey_release(struct socket *sock)
 	return 0;
 }
 
-static int pfkey_broadcast_one(struct sk_buff *skb, struct sk_buff **skb2,
-			       gfp_t allocation, struct sock *sk)
+static int pfkey_broadcast_one(struct sk_buff *skb, gfp_t allocation,
+			       struct sock *sk)
 {
 	int err = -ENOBUFS;
 
-	sock_hold(sk);
-	if (*skb2 == NULL) {
-		if (refcount_read(&skb->users) != 1) {
-			*skb2 = skb_clone(skb, allocation);
-		} else {
-			*skb2 = skb;
-			refcount_inc(&skb->users);
-		}
-	}
-	if (*skb2 != NULL) {
-		if (atomic_read(&sk->sk_rmem_alloc) <= sk->sk_rcvbuf) {
-			skb_set_owner_r(*skb2, sk);
-			skb_queue_tail(&sk->sk_receive_queue, *skb2);
-			sk->sk_data_ready(sk);
-			*skb2 = NULL;
-			err = 0;
-		}
+	if (atomic_read(&sk->sk_rmem_alloc) > sk->sk_rcvbuf)
+		return err;
+
+	skb = skb_clone(skb, allocation);
+
+	if (skb) {
+		skb_set_owner_r(skb, sk);
+		skb_queue_tail(&sk->sk_receive_queue, skb);
+		sk->sk_data_ready(sk);
+		err = 0;
 	}
-	sock_put(sk);
 	return err;
 }
 
@@ -234,7 +226,6 @@ static int pfkey_broadcast(struct sk_buff *skb, gfp_t allocation,
 {
 	struct netns_pfkey *net_pfkey = net_generic(net, pfkey_net_id);
 	struct sock *sk;
-	struct sk_buff *skb2 = NULL;
 	int err = -ESRCH;
 
 	/* XXX Do we need something like netlink_overrun?  I think
@@ -253,7 +244,7 @@ static int pfkey_broadcast(struct sk_buff *skb, gfp_t allocation,
 		 * socket.
 		 */
 		if (pfk->promisc)
-			pfkey_broadcast_one(skb, &skb2, GFP_ATOMIC, sk);
+			pfkey_broadcast_one(skb, GFP_ATOMIC, sk);
 
 		/* the exact target will be processed later */
 		if (sk == one_sk)
@@ -268,7 +259,7 @@ static int pfkey_broadcast(struct sk_buff *skb, gfp_t allocation,
 				continue;
 		}
 
-		err2 = pfkey_broadcast_one(skb, &skb2, GFP_ATOMIC, sk);
+		err2 = pfkey_broadcast_one(skb, GFP_ATOMIC, sk);
 
 		/* Error is cleared after successful sending to at least one
 		 * registered KM */
@@ -278,9 +269,8 @@ static int pfkey_broadcast(struct sk_buff *skb, gfp_t allocation,
 	rcu_read_unlock();
 
 	if (one_sk != NULL)
-		err = pfkey_broadcast_one(skb, &skb2, allocation, one_sk);
+		err = pfkey_broadcast_one(skb, allocation, one_sk);
 
-	kfree_skb(skb2);
 	kfree_skb(skb);
 	return err;
 }
-- 
1.9.1

Re: [PATCH net] af_key: unconditionally clone on broadcast

[David Miller]

From: Sean Tranchetti <stranche@codeaurora.org>
Date: Thu,  7 Feb 2019 13:33:21 -0700

> Attempting to avoid cloning the skb when broadcasting by inflating
> the refcount with sock_hold/sock_put while under RCU lock is dangerous
> and violates RCU principles. It leads to subtle race conditions when
> attempting to free the SKB, as we may reference sockets that have
> already been freed by the stack.
 ...
> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Sean Tranchetti <stranche@codeaurora.org>
> ---
> Realized I never actually sent this patch out after testing the changes
> Eric recommended. Whoops. Better late then never, I suppose...

Steffen, I assume you will review and pick this up.

Thanks.

Re: [PATCH net] af_key: unconditionally clone on broadcast

[Steffen Klassert]

On Mon, Feb 11, 2019 at 12:45:54PM -0800, David Miller wrote:
> From: Sean Tranchetti <stranche@codeaurora.org>
> Date: Thu,  7 Feb 2019 13:33:21 -0700
> 
> > Attempting to avoid cloning the skb when broadcasting by inflating
> > the refcount with sock_hold/sock_put while under RCU lock is dangerous
> > and violates RCU principles. It leads to subtle race conditions when
> > attempting to free the SKB, as we may reference sockets that have
> > already been freed by the stack.
>  ...
> > Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
> > Signed-off-by: Sean Tranchetti <stranche@codeaurora.org>
> > ---
> > Realized I never actually sent this patch out after testing the changes
> > Eric recommended. Whoops. Better late then never, I suppose...
> 
> Steffen, I assume you will review and pick this up.

I was not on Cc and overlooked it at the list.
Thanks for the pointer!

Now applied to the ipsec tree, thanks Sean!