* [PATCH bpf 0/2] bpf fix for unconnect af_unix socket
@ 2023-12-01 3:23 John Fastabend
2023-12-01 3:23 ` [PATCH bpf 1/2] bpf: syzkaller found null ptr deref in unix_bpf proto add John Fastabend
2023-12-01 3:23 ` [PATCH bpf 2/2] bpf: sockmap, test for unconnected af_unix sock John Fastabend
0 siblings, 2 replies; 8+ messages in thread
From: John Fastabend @ 2023-12-01 3:23 UTC (permalink / raw)
To: kuniyu, edumazet, jakub; +Cc: john.fastabend, bpf, netdev
Eric reported a syzbot splat from a null ptr deref from recent fix to
resolve a use-after-free with af-unix stream sockets and BPF sockmap
usage.
The issue is I missed is we allow unconnected af_unix STREAM sockets to
be added to the sockmap. Fix this by blocking unconnected sockets.
John Fastabend (2):
bpf: syzkaller found null ptr deref in unix_bpf proto add
bpf: sockmap, test for unconnected af_unix sock
include/net/sock.h | 5 +++
net/core/sock_map.c | 2 ++
.../selftests/bpf/prog_tests/sockmap_basic.c | 34 +++++++++++++++++++
3 files changed, 41 insertions(+)
--
2.33.0
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH bpf 1/2] bpf: syzkaller found null ptr deref in unix_bpf proto add
2023-12-01 3:23 [PATCH bpf 0/2] bpf fix for unconnect af_unix socket John Fastabend
@ 2023-12-01 3:23 ` John Fastabend
2023-12-01 9:24 ` Eric Dumazet
2023-12-01 3:23 ` [PATCH bpf 2/2] bpf: sockmap, test for unconnected af_unix sock John Fastabend
1 sibling, 1 reply; 8+ messages in thread
From: John Fastabend @ 2023-12-01 3:23 UTC (permalink / raw)
To: kuniyu, edumazet, jakub; +Cc: john.fastabend, bpf, netdev
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(+)
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;
}
--
2.33.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH bpf 2/2] bpf: sockmap, test for unconnected af_unix sock
2023-12-01 3:23 [PATCH bpf 0/2] bpf fix for unconnect af_unix socket John Fastabend
2023-12-01 3:23 ` [PATCH bpf 1/2] bpf: syzkaller found null ptr deref in unix_bpf proto add John Fastabend
@ 2023-12-01 3:23 ` John Fastabend
2023-12-01 9:39 ` Jakub Sitnicki
1 sibling, 1 reply; 8+ messages in thread
From: John Fastabend @ 2023-12-01 3:23 UTC (permalink / raw)
To: kuniyu, edumazet, jakub; +Cc: john.fastabend, bpf, netdev
Add test to sockmap_basic to ensure af_unix sockets that are not connected
can not be added to the map. Ensure we keep DGRAM sockets working however
as these will not be connected typically.
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
.../selftests/bpf/prog_tests/sockmap_basic.c | 34 +++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
index f75f84d0b3d7..ad96f4422def 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
@@ -524,6 +524,37 @@ static void test_sockmap_skb_verdict_peek(void)
test_sockmap_pass_prog__destroy(pass);
}
+static void test_sockmap_unconnected_unix(void)
+{
+ int err, map, stream = 0, dgram = 0, zero = 0;
+ struct test_sockmap_pass_prog *skel;
+
+ skel = test_sockmap_pass_prog__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "open_and_load"))
+ return;
+
+ map = bpf_map__fd(skel->maps.sock_map_rx);
+
+ stream = xsocket(AF_UNIX, SOCK_STREAM, 0);
+ if (!ASSERT_GT(stream, -1, "socket(AF_UNIX, SOCK_STREAM)"))
+ return;
+
+ dgram = xsocket(AF_UNIX, SOCK_DGRAM, 0);
+ if (!ASSERT_GT(dgram, -1, "socket(AF_UNIX, SOCK_DGRAM)")) {
+ close(stream);
+ return;
+ }
+
+ err = bpf_map_update_elem(map, &zero, &stream, BPF_ANY);
+ ASSERT_ERR(err, "bpf_map_update_elem(stream)");
+
+ err = bpf_map_update_elem(map, &zero, &dgram, BPF_ANY);
+ ASSERT_OK(err, "bpf_map_update_elem(dgram)");
+
+ close(stream);
+ close(dgram);
+}
+
void test_sockmap_basic(void)
{
if (test__start_subtest("sockmap create_update_free"))
@@ -566,4 +597,7 @@ void test_sockmap_basic(void)
test_sockmap_skb_verdict_fionread(false);
if (test__start_subtest("sockmap skb_verdict msg_f_peek"))
test_sockmap_skb_verdict_peek();
+
+ if (test__start_subtest("sockmap unconnected af_unix"))
+ test_sockmap_unconnected_unix();
}
--
2.33.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH bpf 1/2] bpf: syzkaller found null ptr deref in unix_bpf proto add
2023-12-01 3:23 ` [PATCH bpf 1/2] bpf: syzkaller found null ptr deref in unix_bpf proto add John Fastabend
@ 2023-12-01 9:24 ` Eric Dumazet
2023-12-01 9:35 ` Jakub Sitnicki
0 siblings, 1 reply; 8+ messages in thread
From: Eric Dumazet @ 2023-12-01 9:24 UTC (permalink / raw)
To: John Fastabend; +Cc: kuniyu, jakub, bpf, netdev
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf 1/2] bpf: syzkaller found null ptr deref in unix_bpf proto add
2023-12-01 9:24 ` Eric Dumazet
@ 2023-12-01 9:35 ` Jakub Sitnicki
2023-12-01 15:36 ` John Fastabend
0 siblings, 1 reply; 8+ messages in thread
From: Jakub Sitnicki @ 2023-12-01 9:35 UTC (permalink / raw)
To: John Fastabend, Eric Dumazet; +Cc: kuniyu, bpf, netdev
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.
[...]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf 2/2] bpf: sockmap, test for unconnected af_unix sock
2023-12-01 3:23 ` [PATCH bpf 2/2] bpf: sockmap, test for unconnected af_unix sock John Fastabend
@ 2023-12-01 9:39 ` Jakub Sitnicki
2023-12-01 16:35 ` John Fastabend
0 siblings, 1 reply; 8+ messages in thread
From: Jakub Sitnicki @ 2023-12-01 9:39 UTC (permalink / raw)
To: John Fastabend; +Cc: kuniyu, edumazet, bpf, netdev
On Thu, Nov 30, 2023 at 07:23 PM -08, John Fastabend wrote:
> Add test to sockmap_basic to ensure af_unix sockets that are not connected
> can not be added to the map. Ensure we keep DGRAM sockets working however
> as these will not be connected typically.
>
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> ---
> .../selftests/bpf/prog_tests/sockmap_basic.c | 34 +++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> index f75f84d0b3d7..ad96f4422def 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> @@ -524,6 +524,37 @@ static void test_sockmap_skb_verdict_peek(void)
> test_sockmap_pass_prog__destroy(pass);
> }
>
> +static void test_sockmap_unconnected_unix(void)
> +{
> + int err, map, stream = 0, dgram = 0, zero = 0;
> + struct test_sockmap_pass_prog *skel;
> +
> + skel = test_sockmap_pass_prog__open_and_load();
> + if (!ASSERT_OK_PTR(skel, "open_and_load"))
> + return;
> +
> + map = bpf_map__fd(skel->maps.sock_map_rx);
> +
> + stream = xsocket(AF_UNIX, SOCK_STREAM, 0);
> + if (!ASSERT_GT(stream, -1, "socket(AF_UNIX, SOCK_STREAM)"))
> + return;
Isn't it redudant to use both the xsocket wrapper and ASSERT_* macro?
Or is there some debugging value that comes from that, which I missed?
> +
> + dgram = xsocket(AF_UNIX, SOCK_DGRAM, 0);
> + if (!ASSERT_GT(dgram, -1, "socket(AF_UNIX, SOCK_DGRAM)")) {
> + close(stream);
> + return;
> + }
> +
> + err = bpf_map_update_elem(map, &zero, &stream, BPF_ANY);
> + ASSERT_ERR(err, "bpf_map_update_elem(stream)");
> +
> + err = bpf_map_update_elem(map, &zero, &dgram, BPF_ANY);
> + ASSERT_OK(err, "bpf_map_update_elem(dgram)");
> +
> + close(stream);
> + close(dgram);
> +}
> +
> void test_sockmap_basic(void)
> {
> if (test__start_subtest("sockmap create_update_free"))
> @@ -566,4 +597,7 @@ void test_sockmap_basic(void)
> test_sockmap_skb_verdict_fionread(false);
> if (test__start_subtest("sockmap skb_verdict msg_f_peek"))
> test_sockmap_skb_verdict_peek();
> +
> + if (test__start_subtest("sockmap unconnected af_unix"))
> + test_sockmap_unconnected_unix();
> }
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf 1/2] bpf: syzkaller found null ptr deref in unix_bpf proto add
2023-12-01 9:35 ` Jakub Sitnicki
@ 2023-12-01 15:36 ` John Fastabend
0 siblings, 0 replies; 8+ messages in thread
From: John Fastabend @ 2023-12-01 15:36 UTC (permalink / raw)
To: Jakub Sitnicki, John Fastabend, Eric Dumazet; +Cc: kuniyu, bpf, netdev
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf 2/2] bpf: sockmap, test for unconnected af_unix sock
2023-12-01 9:39 ` Jakub Sitnicki
@ 2023-12-01 16:35 ` John Fastabend
0 siblings, 0 replies; 8+ messages in thread
From: John Fastabend @ 2023-12-01 16:35 UTC (permalink / raw)
To: Jakub Sitnicki, John Fastabend; +Cc: kuniyu, edumazet, bpf, netdev
Jakub Sitnicki wrote:
> On Thu, Nov 30, 2023 at 07:23 PM -08, John Fastabend wrote:
> > Add test to sockmap_basic to ensure af_unix sockets that are not connected
> > can not be added to the map. Ensure we keep DGRAM sockets working however
> > as these will not be connected typically.
> >
> > Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> > ---
> > .../selftests/bpf/prog_tests/sockmap_basic.c | 34 +++++++++++++++++++
> > 1 file changed, 34 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > index f75f84d0b3d7..ad96f4422def 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > @@ -524,6 +524,37 @@ static void test_sockmap_skb_verdict_peek(void)
> > test_sockmap_pass_prog__destroy(pass);
> > }
> >
> > +static void test_sockmap_unconnected_unix(void)
> > +{
> > + int err, map, stream = 0, dgram = 0, zero = 0;
> > + struct test_sockmap_pass_prog *skel;
> > +
> > + skel = test_sockmap_pass_prog__open_and_load();
> > + if (!ASSERT_OK_PTR(skel, "open_and_load"))
> > + return;
> > +
> > + map = bpf_map__fd(skel->maps.sock_map_rx);
> > +
> > + stream = xsocket(AF_UNIX, SOCK_STREAM, 0);
> > + if (!ASSERT_GT(stream, -1, "socket(AF_UNIX, SOCK_STREAM)"))
> > + return;
>
> Isn't it redudant to use both the xsocket wrapper and ASSERT_* macro?
> Or is there some debugging value that comes from that, which I missed?
It looks like xsocket does an error_at_liine() so guess not you
will have the line number if it fails so will know if it was stream
or dgram that failed. Probably can just drop the if altogether and
let the thing fall through and fail.
>
> > +
> > + dgram = xsocket(AF_UNIX, SOCK_DGRAM, 0);
> > + if (!ASSERT_GT(dgram, -1, "socket(AF_UNIX, SOCK_DGRAM)")) {
> > + close(stream);
> > + return;
> > + }
> > +
> > + err = bpf_map_update_elem(map, &zero, &stream, BPF_ANY);
> > + ASSERT_ERR(err, "bpf_map_update_elem(stream)");
> > +
> > + err = bpf_map_update_elem(map, &zero, &dgram, BPF_ANY);
> > + ASSERT_OK(err, "bpf_map_update_elem(dgram)");
> > +
> > + close(stream);
> > + close(dgram);
> > +}
> > +
> > void test_sockmap_basic(void)
> > {
> > if (test__start_subtest("sockmap create_update_free"))
> > @@ -566,4 +597,7 @@ void test_sockmap_basic(void)
> > test_sockmap_skb_verdict_fionread(false);
> > if (test__start_subtest("sockmap skb_verdict msg_f_peek"))
> > test_sockmap_skb_verdict_peek();
> > +
> > + if (test__start_subtest("sockmap unconnected af_unix"))
> > + test_sockmap_unconnected_unix();
> > }
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-12-01 16:35 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-01 3:23 [PATCH bpf 0/2] bpf fix for unconnect af_unix socket John Fastabend
2023-12-01 3:23 ` [PATCH bpf 1/2] bpf: syzkaller found null ptr deref in unix_bpf proto add John Fastabend
2023-12-01 9:24 ` Eric Dumazet
2023-12-01 9:35 ` Jakub Sitnicki
2023-12-01 15:36 ` John Fastabend
2023-12-01 3:23 ` [PATCH bpf 2/2] bpf: sockmap, test for unconnected af_unix sock John Fastabend
2023-12-01 9:39 ` Jakub Sitnicki
2023-12-01 16:35 ` John Fastabend
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).