On 12/8/23 5:19 AM, Cong Wang wrote:
On Mon, Dec 04, 2023 at 01:40:40PM -0800, John Fastabend wrote:
Kuniyuki Iwashima wrote:
From: John Fastabend <john.fastabend@xxxxxxxxx>
Date: Fri, 1 Dec 2023 10:01:38 -0800
I added logic to track the sock pair for stream_unix sockets so that we
ensure lifetime of the sock matches the time a sockmap could reference
the sock (see fixes tag). I forgot though that we allow af_unix unconnected
sockets into a sock{map|hash} map.
This is problematic because previous fixed expected sk_pair() to exist
and did not NULL check it. Because unconnected sockets have a NULL
sk_pair this resulted in the NULL ptr dereference found by syzkaller.
BUG: KASAN: null-ptr-deref in unix_stream_bpf_update_proto+0x72/0x430 net/unix/unix_bpf.c:171
Write of size 4 at addr 0000000000000080 by task syz-executor360/5073
Call Trace:
<TASK>
...
sock_hold include/net/sock.h:777 [inline]
unix_stream_bpf_update_proto+0x72/0x430 net/unix/unix_bpf.c:171
sock_map_init_proto net/core/sock_map.c:190 [inline]
sock_map_link+0xb87/0x1100 net/core/sock_map.c:294
sock_map_update_common+0xf6/0x870 net/core/sock_map.c:483
sock_map_update_elem_sys+0x5b6/0x640 net/core/sock_map.c:577
bpf_map_update_value+0x3af/0x820 kernel/bpf/syscall.c:167
We considered just checking for the null ptr and skipping taking a ref
on the NULL peer sock. But, if the socket is then connected() after
being added to the sockmap we can cause the original issue again. So
instead this patch blocks adding af_unix sockets that are not in the
ESTABLISHED state.
I'm not sure if someone has the unconnected stream socket use case
though, can't we call additional sock_hold() in connect() by checking
sk_prot under sk_callback_lock ?
Could be done I guess yes. I'm not sure the utility of it though. I
thought above patch was the simplest solution and didn't require touching
main af_unix code. I don't actually use the sockmap with af_unix
sockets anywhere so maybe someone who is using this can comment if
unconnected is needed?
Our use case is also connected unix stream socket, as demonstrated in
the selftest unix_redir_to_connected().
Great, is everyone good to move this fix forward then? Would be great if
this receives at least one ack if the latter is indeed the case.
Thanks,
Daniel
