All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sean Tranchetti <stranche@codeaurora.org>
To: netdev@vger.kernel.org, steffen.klassert@secunet.com
Cc: Sean Tranchetti <stranche@codeaurora.org>
Subject: [PATCH net] af_key: free SKBs under RCU protection
Date: Wed, 19 Sep 2018 18:18:32 -0600	[thread overview]
Message-ID: <1537402712-12875-1-git-send-email-stranche@codeaurora.org> (raw)

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 <stranche@codeaurora.org>
---
 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;
 }
 
-- 
1.9.1

             reply	other threads:[~2018-09-20  5:59 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-20  0:18 Sean Tranchetti [this message]
2018-09-20 13:29 ` [PATCH net] af_key: free SKBs under RCU protection Eric Dumazet
2018-09-20 19:25   ` stranche
2018-09-20 22:10     ` Eric Dumazet
2018-09-20 22:29       ` Eric Dumazet
2018-09-21 17:09         ` stranche
2018-09-21 17:40           ` Eric Dumazet
2018-09-21 18:44             ` stranche
2018-09-23 17:15     ` Eric Dumazet
2018-09-24 18:46       ` stranche

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1537402712-12875-1-git-send-email-stranche@codeaurora.org \
    --to=stranche@codeaurora.org \
    --cc=netdev@vger.kernel.org \
    --cc=steffen.klassert@secunet.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.