Commit fc2d5cfd authored by Sean Tranchetti's avatar Sean Tranchetti Committed by Steffen Klassert
Browse files

af_key: unconditionally clone on broadcast 



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: default avatarEric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: default avatarSean Tranchetti <stranche@codeaurora.org>
Signed-off-by: default avatarSteffen Klassert <steffen.klassert@secunet.com>
parent f75a2804

net/key/af_key.c

+15 −25
Original line number Original line Diff line number Diff line
@@ -196,30 +196,22 @@ static int pfkey_release(struct socket *sock) 
	return 0;

	return 0;

}

}





static int pfkey_broadcast_one(struct sk_buff *skb, struct sk_buff **skb2,

static int pfkey_broadcast_one(struct sk_buff *skb, gfp_t allocation,

			       gfp_t allocation, struct sock *sk)

			       struct sock *sk)

{

{

	int err = -ENOBUFS;

	int err = -ENOBUFS;





	sock_hold(sk);

	if (atomic_read(&sk->sk_rmem_alloc) > sk->sk_rcvbuf)

	if (*skb2 == NULL) {

		return err;

		if (refcount_read(&skb->users) != 1) {



			*skb2 = skb_clone(skb, allocation);

	skb = skb_clone(skb, allocation);

		} else {



			*skb2 = skb;

	if (skb) {

			refcount_inc(&skb->users);

		skb_set_owner_r(skb, sk);

		}

		skb_queue_tail(&sk->sk_receive_queue, skb);

	}

	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);

		sk->sk_data_ready(sk);

			*skb2 = NULL;

		err = 0;

		err = 0;

	}

	}

	}

	sock_put(sk);

	return err;

	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 netns_pfkey *net_pfkey = net_generic(net, pfkey_net_id);

	struct sock *sk;

	struct sock *sk;

	struct sk_buff *skb2 = NULL;

	int err = -ESRCH;

	int err = -ESRCH;





	/* XXX Do we need something like netlink_overrun?  I think

	/* 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.

		 * socket.

		 */

		 */

		if (pfk->promisc)

		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 */

		/* the exact target will be processed later */

		if (sk == one_sk)

		if (sk == one_sk)
@@ -268,7 +259,7 @@ static int pfkey_broadcast(struct sk_buff *skb, gfp_t allocation, 
				continue;

				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

		/* Error is cleared after successful sending to at least one

		 * registered KM */

		 * registered KM */
@@ -278,9 +269,8 @@ static int pfkey_broadcast(struct sk_buff *skb, gfp_t allocation, 
	rcu_read_unlock();

	rcu_read_unlock();





	if (one_sk != NULL)

	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);

	kfree_skb(skb);

	return err;

	return err;

}

}

CRA Git | Maintained and supported by SUSTech CRA and CCSE