| Message ID | 20231201180139.328529-2-john.fastabend@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8d6650646ce49e9a5b8c5c23eb94f74b1749f70f |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf fix for unconnect af_unix socket | expand |
From: John Fastabend <john.fastabend@gmail.com> 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 ?
Kuniyuki Iwashima wrote: > From: John Fastabend <john.fastabend@gmail.com> > 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? From rcu and locking side looks like holding sk_callback_lock would be sufficient. I was thinking it would require a rcu grace period or something but seems not. I guess I could improve original patch if folks want. .John
+cc Cong and Jiang, as potential users of AF_UNIX sockmap w/ unconnected SOCK_STREAM sockets https://lore.kernel.org/netdev/20231201180139.328529-1-john.fastabend@gmail.com/ From: John Fastabend <john.fastabend@gmail.com> Date: Mon, 04 Dec 2023 13:40:40 -0800 > Kuniyuki Iwashima wrote: > > From: John Fastabend <john.fastabend@gmail.com> > > 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? > > From rcu and locking side looks like holding sk_callback_lock would > be sufficient. I was thinking it would require a rcu grace period > or something but seems not. > > I guess I could improve original patch if folks want. > > .John
On Mon, Dec 04, 2023 at 01:40 PM -08, John Fastabend wrote: > Kuniyuki Iwashima wrote: >> From: John Fastabend <john.fastabend@gmail.com> >> 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? > > From rcu and locking side looks like holding sk_callback_lock would > be sufficient. I was thinking it would require a rcu grace period > or something but seems not. I'd revist the option of grabbing an skpair ref in unix_stream_sendmsg.
On Mon, Dec 04, 2023 at 01:40:40PM -0800, John Fastabend wrote: > Kuniyuki Iwashima wrote: > > From: John Fastabend <john.fastabend@gmail.com> > > 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(). Thanks.
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@gmail.com> >>> 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
On Mon, Dec 11, 2023 at 6:56 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > > 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@gmail.com> > >>> 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 I just want to ack that we are not inserting unconnected UDS to sockmap.
diff --git a/include/net/sock.h b/include/net/sock.h index 1d6931caf0c3..0201136b0b9c 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -2799,6 +2799,11 @@ static inline bool sk_is_tcp(const struct sock *sk) return sk->sk_type == SOCK_STREAM && sk->sk_protocol == IPPROTO_TCP; } +static inline bool sk_is_stream_unix(const struct sock *sk) +{ + return sk->sk_family == AF_UNIX && sk->sk_type == SOCK_STREAM; +} + /** * sk_eat_skb - Release a skb if it is no longer needed * @sk: socket to eat this skb from diff --git a/net/core/sock_map.c b/net/core/sock_map.c index 4292c2ed1828..27d733c0f65e 100644 --- a/net/core/sock_map.c +++ b/net/core/sock_map.c @@ -536,6 +536,8 @@ static bool sock_map_sk_state_allowed(const struct sock *sk) { if (sk_is_tcp(sk)) return (1 << sk->sk_state) & (TCPF_ESTABLISHED | TCPF_LISTEN); + if (sk_is_stream_unix(sk)) + return (1 << sk->sk_state) & TCPF_ESTABLISHED; return true; }
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. Reported-by: Eric Dumazet <edumazet@google.com> Reported-by: syzbot+e8030702aefd3444fb9e@syzkaller.appspotmail.com Fixes: 8866730aed51 ("bpf, sockmap: af_unix stream sockets need to hold ref for pair sock") Signed-off-by: John Fastabend <john.fastabend@gmail.com> --- include/net/sock.h | 5 +++++ net/core/sock_map.c | 2 ++ 2 files changed, 7 insertions(+)