diff mbox series

[bpf,1/2] bpf: syzkaller found null ptr deref in unix_bpf proto add

Message ID 20231201032316.183845-2-john.fastabend@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series bpf fix for unconnect af_unix socket | expand

Checks

Context Check Description
bpf/vmtest-bpf-PR success PR summary
bpf/vmtest-bpf-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-VM_Test-2 success Logs for Validate matrix.py
bpf/vmtest-bpf-VM_Test-3 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-VM_Test-8 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-VM_Test-4 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-9 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-VM_Test-7 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-14 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-VM_Test-17 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-19 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-16 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-VM_Test-18 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-15 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-22 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-21 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-20 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-27 success Logs for x86_64-llvm-16 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-23 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-28 success Logs for x86_64-llvm-16 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-24 success Logs for x86_64-llvm-16 / build / build for x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-29 success Logs for x86_64-llvm-16 / veristat
bpf/vmtest-bpf-VM_Test-25 success Logs for x86_64-llvm-16 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-26 success Logs for x86_64-llvm-16 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-5 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-6 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-13 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-VM_Test-12 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-VM_Test-11 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-VM_Test-10 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf, async
netdev/ynl success SINGLE THREAD; Generated files up to date; no warnings/errors;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 3641 this patch: 3641
netdev/cc_maintainers fail 1 blamed authors not CCed: daniel@iogearbox.net; 3 maintainers not CCed: kuba@kernel.org pabeni@redhat.com daniel@iogearbox.net
netdev/build_clang success Errors and warnings before: 1294 this patch: 1294
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 3881 this patch: 3881
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 19 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

John Fastabend Dec. 1, 2023, 3:23 a.m. UTC 
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(+)

Comments

Eric Dumazet Dec. 1, 2023, 9:24 a.m. UTC | #1 
On Fri, Dec 1, 2023 at 4:23 AM John Fastabend <john.fastabend@gmail.com> wrote:
>
> 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.


This (and the name chosen for sk_is_unix() helper) is a bit confusing ?

When you say "af_unix sockets" you seem to imply STREAM sockets.


>
> 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(+)
>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 1d6931caf0c3..ea1155d68f0b 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_unix(const struct sock *sk)

Maybe sk_is_stream_unix() ?

> +{
> +       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..448aea066942 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_unix(sk))
> +               return (1 << sk->sk_state) & TCPF_ESTABLISHED;
>         return true;
>  }
>
> --
> 2.33.0
>
Jakub Sitnicki Dec. 1, 2023, 9:35 a.m. UTC | #2 
On Fri, Dec 01, 2023 at 10:24 AM +01, Eric Dumazet wrote:
> On Fri, Dec 1, 2023 at 4:23 AM John Fastabend <john.fastabend@gmail.com> wrote:

[...]

>> diff --git a/include/net/sock.h b/include/net/sock.h
>> index 1d6931caf0c3..ea1155d68f0b 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_unix(const struct sock *sk)
>
> Maybe sk_is_stream_unix() ?
>

+1. I found it confusing as well.

[...]
John Fastabend Dec. 1, 2023, 3:36 p.m. UTC | #3 
Jakub Sitnicki wrote:
> On Fri, Dec 01, 2023 at 10:24 AM +01, Eric Dumazet wrote:
> > On Fri, Dec 1, 2023 at 4:23 AM John Fastabend <john.fastabend@gmail.com> wrote:
> 
> [...]
> 
> >> diff --git a/include/net/sock.h b/include/net/sock.h
> >> index 1d6931caf0c3..ea1155d68f0b 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_unix(const struct sock *sk)
> >
> > Maybe sk_is_stream_unix() ?
> >
> 
> +1. I found it confusing as well.
> 
> [...]

OK will do v2 with sk_is_stream_unix() so it reads better. Thanks.
diff mbox series

Patch

diff --git a/include/net/sock.h b/include/net/sock.h
index 1d6931caf0c3..ea1155d68f0b 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_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..448aea066942 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_unix(sk))
+		return (1 << sk->sk_state) & TCPF_ESTABLISHED;
 	return true;
 }