netdev - Re: [PATCH bpf] bpf: sockmap, fix skb refcnt race after locking changes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8a259e47-f119-9bba-acc1-a95a2f3d4cc3@huawei.com>
Date: Sat, 2 Sep 2023 16:13:28 +0800
From: Xu Kuohai <xukuohai@...wei.com>
To: John Fastabend <john.fastabend@...il.com>, <olsajiri@...il.com>,
	<eddyz87@...il.com>
CC: <edumazet@...gle.com>, <cong.wang@...edance.com>, <bpf@...r.kernel.org>,
	<netdev@...r.kernel.org>
Subject: Re: [PATCH bpf] bpf: sockmap, fix skb refcnt race after locking
 changes

On 9/2/2023 4:21 AM, John Fastabend wrote:
> There is a race where skb's from the sk_psock_backlog can be referenced
> after userspace side has already skb_consumed() the sk_buff and its
> refcnt dropped to zer0 causing use after free.
> 
> The flow is the following,
> 
>    while ((skb = skb_peek(&psock->ingress_skb))
>      sk_psock_handle_Skb(psock, skb, ..., ingress)
>      if (!ingress) ...
>      sk_psock_skb_ingress
>         sk_psock_skb_ingress_enqueue(skb)
>            msg->skb = skb
>            sk_psock_queue_msg(psock, msg)
>      skb_dequeue(&psock->ingress_skb)
> 
> The sk_psock_queue_msg() puts the msg on the ingress_msg queue. This is
> what the application reads when recvmsg() is called. An application can
> read this anytime after the msg is placed on the queue. The recvmsg
> hook will also read msg->skb and then after user space reads the msg
> will call consume_skb(skb) on it effectively free'ing it.
> 
> But, the race is in above where backlog queue still has a reference to
> the skb and calls skb_dequeue(). If the skb_dequeue happens after the
> user reads and free's the skb we have a use after free.
> 
> The !ingress case does not suffer from this problem because it uses
> sendmsg_*(sk, msg) which does not pass the sk_buff further down the
> stack.
> 
> The following splat was observed with 'test_progs -t sockmap_listen':
> 
> [ 1022.710250][ T2556] general protection fault, ...
>   ...
> [ 1022.712830][ T2556] Workqueue: events sk_psock_backlog
> [ 1022.713262][ T2556] RIP: 0010:skb_dequeue+0x4c/0x80
> [ 1022.713653][ T2556] Code: ...
>   ...
> [ 1022.720699][ T2556] Call Trace:
> [ 1022.720984][ T2556]  <TASK>
> [ 1022.721254][ T2556]  ? die_addr+0x32/0x80^M
> [ 1022.721589][ T2556]  ? exc_general_protection+0x25a/0x4b0
> [ 1022.722026][ T2556]  ? asm_exc_general_protection+0x22/0x30
> [ 1022.722489][ T2556]  ? skb_dequeue+0x4c/0x80
> [ 1022.722854][ T2556]  sk_psock_backlog+0x27a/0x300
> [ 1022.723243][ T2556]  process_one_work+0x2a7/0x5b0
> [ 1022.723633][ T2556]  worker_thread+0x4f/0x3a0
> [ 1022.723998][ T2556]  ? __pfx_worker_thread+0x10/0x10
> [ 1022.724386][ T2556]  kthread+0xfd/0x130
> [ 1022.724709][ T2556]  ? __pfx_kthread+0x10/0x10
> [ 1022.725066][ T2556]  ret_from_fork+0x2d/0x50
> [ 1022.725409][ T2556]  ? __pfx_kthread+0x10/0x10
> [ 1022.725799][ T2556]  ret_from_fork_asm+0x1b/0x30
> [ 1022.726201][ T2556]  </TASK>
> 
> To fix we add an skb_get() before passing the skb to be enqueued in
> the engress queue. This bumps the skb->users refcnt so that consume_skb
> and kfree_skb will not immediately free the sk_buff. With this we can
> be sure the skb is still around when we do the dequeue. Then we just
> need to decrement the refcnt or free the skb in the backlog case which
> we do by calling kfree_skb() on the ingress case as well as the sendmsg
> case.
> 
> Before locking change from fixes tag we had the sock locked so we
> couldn't race with user and there was no issue here.
> 
> Fixes: 799aa7f98d53e (skmsg: Avoid lock_sock() in sk_psock_backlog())
> Reported-by: Jiri Olsa  <jolsa@...nel.org>
> Signed-off-by: John Fastabend <john.fastabend@...il.com>
> ---
>   net/core/skmsg.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index a0659fc29bcc..6c31eefbd777 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -612,12 +612,18 @@ static int sk_psock_skb_ingress_self(struct sk_psock *psock, struct sk_buff *skb
>   static int sk_psock_handle_skb(struct sk_psock *psock, struct sk_buff *skb,
>   			       u32 off, u32 len, bool ingress)
>   {
> +	int err = 0;
> +
>   	if (!ingress) {
>   		if (!sock_writeable(psock->sk))
>   			return -EAGAIN;
>   		return skb_send_sock(psock->sk, skb, off, len);
>   	}
> -	return sk_psock_skb_ingress(psock, skb, off, len);
> +	skb_get(skb);
> +	err = sk_psock_skb_ingress(psock, skb, off, len);
> +	if (err < 0)
> +		kfree_skb(skb);
> +	return err;
>   }
>   
>   static void sk_psock_skb_state(struct sk_psock *psock,
> @@ -685,9 +691,7 @@ static void sk_psock_backlog(struct work_struct *work)
>   		} while (len);
>   
>   		skb = skb_dequeue(&psock->ingress_skb);
> -		if (!ingress) {
> -			kfree_skb(skb);
> -		}
> +		kfree_skb(skb);
>   	}
>   end:
>   	mutex_unlock(&psock->work_mutex);

Tested-by: Xu Kuohai <xukuohai@...wei.com>