From: Sean Tranchetti <stran...@codeaurora.org>

[ Upstream commit fc2d5cfdcfe2ab76b263d91429caa22451123085 ]

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.duma...@gmail.com>
Signed-off-by: Sean Tranchetti <stran...@codeaurora.org>
Signed-off-by: Steffen Klassert <steffen.klass...@secunet.com>
Signed-off-by: Sasha Levin <sas...@kernel.org>
---
 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 9d61266526e7..7da629d59717 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;
 }
-- 
2.19.1

Reply via email to