netdev - Re: [PATCH] bpf: sockmap, fix use after free from sleep in psock backlog workqueue

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] [day] [month] [year] [list]

Date:   Fri, 24 May 2019 08:52:54 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>,
        John Fastabend <john.fastabend@...il.com>, bpf@...r.kernel.org,
        jakub@...udflare.com, ast@...nel.org
Cc:     netdev@...r.kernel.org, marek@...udflare
Subject: Re: [PATCH] bpf: sockmap, fix use after free from sleep in psock
 backlog workqueue

Daniel Borkmann wrote:
> On 05/23/2019 05:48 PM, John Fastabend wrote:
> > Backlog work for psock (sk_psock_backlog) might sleep while waiting
> > for memory to free up when sending packets. However, while sleeping
> > the socket may be closed and removed from the map by the user space
> > side.
> > 
> > This breaks an assumption in sk_stream_wait_memory, which expects the
> > wait queue to be still there when it wakes up resulting in a
> > use-after-free shown below. To fix his mark sendmsg as MSG_DONTWAIT
> > to avoid the sleep altogether. We already set the flag for the
> > sendpage case but we missed the case were sendmsg is used.
> > Sockmap is currently the only user of skb_send_sock_locked() so only
> > the sockmap paths should be impacted.
> > 
> > ==================================================================
> > BUG: KASAN: use-after-free in remove_wait_queue+0x31/0x70
> > Write of size 8 at addr ffff888069a0c4e8 by task kworker/0:2/110
> > 
> > CPU: 0 PID: 110 Comm: kworker/0:2 Not tainted 5.0.0-rc2-00335-g28f9d1a3d4fe-dirty #14
> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-2.fc27 04/01/2014
> > Workqueue: events sk_psock_backlog
> > Call Trace:
> >  print_address_description+0x6e/0x2b0
> >  ? remove_wait_queue+0x31/0x70
> >  kasan_report+0xfd/0x177
> >  ? remove_wait_queue+0x31/0x70
> >  ? remove_wait_queue+0x31/0x70
> >  remove_wait_queue+0x31/0x70
> >  sk_stream_wait_memory+0x4dd/0x5f0
> >  ? sk_stream_wait_close+0x1b0/0x1b0
> >  ? wait_woken+0xc0/0xc0
> >  ? tcp_current_mss+0xc5/0x110
> >  tcp_sendmsg_locked+0x634/0x15d0
> >  ? tcp_set_state+0x2e0/0x2e0
> >  ? __kasan_slab_free+0x1d1/0x230
> >  ? kmem_cache_free+0x70/0x140
> >  ? sk_psock_backlog+0x40c/0x4b0
> >  ? process_one_work+0x40b/0x660
> >  ? worker_thread+0x82/0x680
> >  ? kthread+0x1b9/0x1e0
> >  ? ret_from_fork+0x1f/0x30
> >  ? check_preempt_curr+0xaf/0x130
> >  ? iov_iter_kvec+0x5f/0x70
> >  ? kernel_sendmsg_locked+0xa0/0xe0
> >  skb_send_sock_locked+0x273/0x3c0
> >  ? skb_splice_bits+0x180/0x180
> >  ? start_thread+0xe0/0xe0
> >  ? update_min_vruntime.constprop.27+0x88/0xc0
> >  sk_psock_backlog+0xb3/0x4b0
> >  ? strscpy+0xbf/0x1e0
> >  process_one_work+0x40b/0x660
> >  worker_thread+0x82/0x680
> >  ? process_one_work+0x660/0x660
> >  kthread+0x1b9/0x1e0
> >  ? __kthread_create_on_node+0x250/0x250
> >  ret_from_fork+0x1f/0x30
> > 
> > Fixes: 20bf50de3028c ("skbuff: Function to send an skbuf on a socket")
> > Reported-by: Jakub Sitnicki <jakub@...udflare.com>
> > Tested-by: Jakub Sitnicki <jakub@...udflare.com>
> > Signed-off-by: John Fastabend <john.fastabend@...il.com>
> > ---
> >  net/core/skbuff.c |    1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index e89be62..c3b03c5 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -2337,6 +2337,7 @@ int skb_send_sock_locked(struct sock *sk, struct sk_buff *skb, int offset,
> >  		kv.iov_base = skb->data + offset;
> >  		kv.iov_len = slen;
> >  		memset(&msg, 0, sizeof(msg));
> > +		msg.flags = MSG_DONTWAIT;
> >  
> >  		ret = kernel_sendmsg_locked(sk, &msg, &kv, 1, slen);
> >  		if (ret <= 0)
> 
> This doesn't even compile. :( It should have been msg_flags instead ...

Sorry sent the patch without commiting an update, sent a v2 with correction.