netdev - Re: [PATCH bpf 1/2] bpf: sockmap, af_unix sockets need to hold ref for pair sock

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: <87fs289poz.fsf@cloudflare.com>
Date: Wed, 18 Oct 2023 12:40:42 +0200
From: Jakub Sitnicki <jakub@...udflare.com>
To: John Fastabend <john.fastabend@...il.com>
Cc: bpf@...r.kernel.org, netdev@...r.kernel.org, yangyingliang@...wei.com,
 martin.lau@...nel.org
Subject: Re: [PATCH bpf 1/2] bpf: sockmap, af_unix sockets need to hold ref
 for pair sock

On Mon, Oct 16, 2023 at 12:08 PM -07, John Fastabend wrote:
> AF_UNIX sockets are a paired socket. So sending on one of the pairs
> will lookup the paired socket as part of the send operation. It is
> possible however to put just one of the pairs in a BPF map. This
> currently increments the refcnt on the sock in the sockmap to
> ensure it is not free'd by the stack before sockmap cleans up its
> state and stops any skbs being sent/recv'd to that socket.
>
> But we missed a case. If the peer socket is closed it will be
> free'd by the stack. However, the paired socket can still be
> referenced from BPF sockmap side because we hold a reference
> there. Then if we are sending traffic through BPF sockmap to
> that socket it will try to dereference the free'd pair in its
> send logic creating a use after free.  And following splat,
>
>    [59.900375] BUG: KASAN: slab-use-after-free in sk_wake_async+0x31/0x1b0
>    [59.901211] Read of size 8 at addr ffff88811acbf060 by task kworker/1:2/954
>    [...]
>    [59.905468] Call Trace:
>    [59.905787]  <TASK>
>    [59.906066]  dump_stack_lvl+0x130/0x1d0
>    [59.908877]  print_report+0x16f/0x740
>    [59.910629]  kasan_report+0x118/0x160
>    [59.912576]  sk_wake_async+0x31/0x1b0
>    [59.913554]  sock_def_readable+0x156/0x2a0
>    [59.914060]  unix_stream_sendmsg+0x3f9/0x12a0
>    [59.916398]  sock_sendmsg+0x20e/0x250
>    [59.916854]  skb_send_sock+0x236/0xac0
>    [59.920527]  sk_psock_backlog+0x287/0xaa0

Isn't the problem here that unix_stream_sendmsg doesn't grab a ref to
peer sock? Unlike unix_dgram_sendmsg which uses the unix_peer_get
helper.

>
> To fix let BPF sockmap hold a refcnt on both the socket in the
> sockmap and its paired socket.  It wasn't obvious how to contain
> the fix to bpf_unix logic. The primarily problem with keeping this
> logic in bpf_unix was: In the sock close() we could handle the
> deref by having a close handler. But, when we are destroying the
> psock through a map delete operation we wouldn't have gotten any
> signal thorugh the proto struct other than it being replaced.
> If we do the deref from the proto replace its too early because
> we need to deref the skpair after the backlog worker has been
> stopped.
>
> Given all this it seems best to just cache it at the end of the
> psock and eat 8B for the af_unix and vsock users.
>
> Fixes: 94531cfcbe79 ("af_unix: Add unix_stream_proto for sockmap")
> Signed-off-by: John Fastabend <john.fastabend@...il.com>
> ---

[...]