From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Dumazet Subject: Re: [PATCH net] af_key: free SKBs under RCU protection Date: Thu, 20 Sep 2018 06:29:57 -0700 Message-ID: <6d194ac2-15e8-76d7-31d0-b4c4eed68d5a@gmail.com> References: <1537402712-12875-1-git-send-email-stranche@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit To: Sean Tranchetti , netdev@vger.kernel.org, steffen.klassert@secunet.com Return-path: Received: from mail-pg1-f196.google.com ([209.85.215.196]:38769 "EHLO mail-pg1-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726177AbeITTNb (ORCPT ); Thu, 20 Sep 2018 15:13:31 -0400 Received: by mail-pg1-f196.google.com with SMTP id t84-v6so4445686pgb.5 for ; Thu, 20 Sep 2018 06:30:00 -0700 (PDT) In-Reply-To: <1537402712-12875-1-git-send-email-stranche@codeaurora.org> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 09/19/2018 05:18 PM, Sean Tranchetti wrote: > pfkey_broadcast() can make calls to pfkey_broadcast_one() which > will clone or copy the passed in SKB. This new SKB will be assigned > the sock_rfree() function as its destructor, which requires that > the socket reference the SKB contains is valid when the SKB is freed. > > If this SKB is ever passed to pfkey_broadcast() again by some other > function (such as pkfey_dump() or pfkey_promisc) it will then be > freed there. However, since this free occurs outside of RCU protection, > it is possible that userspace could close the socket and trigger > pfkey_release() to free the socket before sock_rfree() can run, creating > the following race condition: > > 1: An SKB belonging to the pfkey socket is passed to pfkey_broadcast(). > It performs the broadcast to any other sockets, and calls > rcu_read_unlock(), but does not yet reach kfree_skb(). > 2: Userspace closes the socket, triggering pfkey_realse(). Since no one > holds the RCU lock, synchronize_rcu() returns and it is allowed to > continue. It calls sock_put() to free the socket. > 3: pfkey_broadcast() now calls kfree_skb() on the original SKB it was > passed, triggering a call to sock_rfree(). This function now accesses > the freed struct sock * via skb->sk, and attempts to update invalid > memory. > > By ensuring that the pfkey_broadcast() also frees the SKBs while it holds > the RCU lock, we can ensure that the socket will remain valid when the SKB > is freed, avoiding crashes like the following: > > 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 > > Signed-off-by: Sean Tranchetti > --- > net/key/af_key.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/key/af_key.c b/net/key/af_key.c > index 9d61266..dd257c7 100644 > --- a/net/key/af_key.c > +++ b/net/key/af_key.c > @@ -275,13 +275,13 @@ static int pfkey_broadcast(struct sk_buff *skb, gfp_t allocation, > if ((broadcast_flags & BROADCAST_REGISTERED) && err) > err = err2; > } > - rcu_read_unlock(); > > if (one_sk != NULL) > err = pfkey_broadcast_one(skb, &skb2, allocation, one_sk); > > kfree_skb(skb2); > kfree_skb(skb); > + rcu_read_unlock(); > return err; > } > > I do not believe the changelog or the patch makes sense. Having skb still referencing a socket prevents this socket being released. If you think about it, what would prevent the freeing happening _before_ the rcu_read_lock() in pfkey_broadcast() ? Maybe the correct fix is that pfkey_broadcast_one() should ensure the socket is still valid. I would suggest something like : diff --git a/net/key/af_key.c b/net/key/af_key.c index 9d61266526e767770d9a1ce184ac8cdd59de309a..5ce309d020dda5e46e4426c4a639bfb551e2260d 100644 --- a/net/key/af_key.c +++ b/net/key/af_key.c @@ -201,7 +201,9 @@ static int pfkey_broadcast_one(struct sk_buff *skb, struct sk_buff **skb2, { int err = -ENOBUFS; - sock_hold(sk); + if (!refcount_inc_not_zero(&sk->sk_refcnt)) + return -ENOENT; + if (*skb2 == NULL) { if (refcount_read(&skb->users) != 1) { *skb2 = skb_clone(skb, allocation);