Kumar reported a KASAN splat in tcp_v6_rcv:
bash-5.2# ./test_progs -t btf_skc_cls_ingress
...
[ 51.810085] BUG: KASAN: slab-out-of-bounds in tcp_v6_rcv+0x2d7d/0x3440
[ 51.810458] Read of size 2 at addr ffff8881053f038c by task test_progs/226
The problem is that inet[6]_steal_sock accesses sk->sk_protocol without
accounting for request sockets. I added the check to ensure that we only
every try to perform a reuseport lookup on a supported socket.
It turns out that this isn't necessary at all. struct sock_common contains
a skc_reuseport flag which indicates whether a socket is part of a
reuseport group. inet[6]_lookup_reuseport already check this flag,
so we can't execute an erroneous reuseport lookup by definition.
Remove the unnecessary assertions to fix the out of bounds access.
Fixes: 9c02bec95954 ("bpf, net: Support SO_REUSEPORT sockets with bpf_sk_assign")
Reported-by: Kumar Kartikeya Dwivedi <[email protected]>
Signed-off-by: Lorenz Bauer <[email protected]>
---
include/net/inet6_hashtables.h | 10 ----------
include/net/inet_hashtables.h | 10 ----------
2 files changed, 20 deletions(-)
diff --git a/include/net/inet6_hashtables.h b/include/net/inet6_hashtables.h
index 284b5ce7205d..f9907ed36d54 100644
--- a/include/net/inet6_hashtables.h
+++ b/include/net/inet6_hashtables.h
@@ -119,16 +119,6 @@ struct sock *inet6_steal_sock(struct net *net, struct sk_buff *skb, int doff,
if (!prefetched)
return sk;
- if (sk->sk_protocol == IPPROTO_TCP) {
- if (sk->sk_state != TCP_LISTEN)
- return sk;
- } else if (sk->sk_protocol == IPPROTO_UDP) {
- if (sk->sk_state != TCP_CLOSE)
- return sk;
- } else {
- return sk;
- }
-
reuse_sk = inet6_lookup_reuseport(net, sk, skb, doff,
saddr, sport, daddr, ntohs(dport),
ehashfn);
diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
index 1177effabed3..57a46993383a 100644
--- a/include/net/inet_hashtables.h
+++ b/include/net/inet_hashtables.h
@@ -465,16 +465,6 @@ struct sock *inet_steal_sock(struct net *net, struct sk_buff *skb, int doff,
if (!prefetched)
return sk;
- if (sk->sk_protocol == IPPROTO_TCP) {
- if (sk->sk_state != TCP_LISTEN)
- return sk;
- } else if (sk->sk_protocol == IPPROTO_UDP) {
- if (sk->sk_state != TCP_CLOSE)
- return sk;
- } else {
- return sk;
- }
-
reuse_sk = inet_lookup_reuseport(net, sk, skb, doff,
saddr, sport, daddr, ntohs(dport),
ehashfn);
---
base-commit: eb62e6aef940fcb1879100130068369d4638088f
change-id: 20230808-bpf-next-a442a095562b
Best regards,
--
Lorenz Bauer <[email protected]>
On Wed, 9 Aug 2023 at 14:04, Lorenz Bauer <[email protected]> wrote:
>
> Kumar reported a KASAN splat in tcp_v6_rcv:
>
> bash-5.2# ./test_progs -t btf_skc_cls_ingress
> ...
> [ 51.810085] BUG: KASAN: slab-out-of-bounds in tcp_v6_rcv+0x2d7d/0x3440
> [ 51.810458] Read of size 2 at addr ffff8881053f038c by task test_progs/226
>
> The problem is that inet[6]_steal_sock accesses sk->sk_protocol without
> accounting for request sockets. I added the check to ensure that we only
> every try to perform a reuseport lookup on a supported socket.
>
> It turns out that this isn't necessary at all. struct sock_common contains
> a skc_reuseport flag which indicates whether a socket is part of a
> reuseport group. inet[6]_lookup_reuseport already check this flag,
> so we can't execute an erroneous reuseport lookup by definition.
>
> Remove the unnecessary assertions to fix the out of bounds access.
>
> Fixes: 9c02bec95954 ("bpf, net: Support SO_REUSEPORT sockets with bpf_sk_assign")
> Reported-by: Kumar Kartikeya Dwivedi <[email protected]>
> Signed-off-by: Lorenz Bauer <[email protected]>
> ---
Thanks for the fix!
Tested-by: Kumar Kartikeya Dwivedi <[email protected]>
On 8/9/23 1:33 AM, Lorenz Bauer wrote:
> Kumar reported a KASAN splat in tcp_v6_rcv:
>
> bash-5.2# ./test_progs -t btf_skc_cls_ingress
> ...
> [ 51.810085] BUG: KASAN: slab-out-of-bounds in tcp_v6_rcv+0x2d7d/0x3440
> [ 51.810458] Read of size 2 at addr ffff8881053f038c by task test_progs/226
>
> The problem is that inet[6]_steal_sock accesses sk->sk_protocol without
> accounting for request sockets. I added the check to ensure that we only
> every try to perform a reuseport lookup on a supported socket.
>
> It turns out that this isn't necessary at all. struct sock_common contains
> a skc_reuseport flag which indicates whether a socket is part of a
Does it go back to the earlier discussion
(https://lore.kernel.org/bpf/[email protected]/)
that the sk->sk_reuseport is 1 from sk_clone for TCP_ESTABLISHED? It works
because there is sk->sk_reuseport"_cb" check going deeper into
reuseport_select_sock() but there is an extra inet6_ehashfn for all TCP_ESTABLISHED.
> reuseport group. inet[6]_lookup_reuseport already check this flag,
> so we can't execute an erroneous reuseport lookup by definition.
>
> Remove the unnecessary assertions to fix the out of bounds access.
>
> Fixes: 9c02bec95954 ("bpf, net: Support SO_REUSEPORT sockets with bpf_sk_assign")
> Reported-by: Kumar Kartikeya Dwivedi <[email protected]>
> Signed-off-by: Lorenz Bauer <[email protected]>
> ---
> include/net/inet6_hashtables.h | 10 ----------
> include/net/inet_hashtables.h | 10 ----------
> 2 files changed, 20 deletions(-)
>
> diff --git a/include/net/inet6_hashtables.h b/include/net/inet6_hashtables.h
> index 284b5ce7205d..f9907ed36d54 100644
> --- a/include/net/inet6_hashtables.h
> +++ b/include/net/inet6_hashtables.h
> @@ -119,16 +119,6 @@ struct sock *inet6_steal_sock(struct net *net, struct sk_buff *skb, int doff,
> if (!prefetched)
> return sk;
>
> - if (sk->sk_protocol == IPPROTO_TCP) {
> - if (sk->sk_state != TCP_LISTEN)
> - return sk;
> - } else if (sk->sk_protocol == IPPROTO_UDP) {
> - if (sk->sk_state != TCP_CLOSE)
> - return sk;
> - } else {
> - return sk;
> - }
> -
> reuse_sk = inet6_lookup_reuseport(net, sk, skb, doff,
> saddr, sport, daddr, ntohs(dport),
> ehashfn);
> diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
> index 1177effabed3..57a46993383a 100644
> --- a/include/net/inet_hashtables.h
> +++ b/include/net/inet_hashtables.h
> @@ -465,16 +465,6 @@ struct sock *inet_steal_sock(struct net *net, struct sk_buff *skb, int doff,
> if (!prefetched)
> return sk;
>
> - if (sk->sk_protocol == IPPROTO_TCP) {
> - if (sk->sk_state != TCP_LISTEN)
> - return sk;
> - } else if (sk->sk_protocol == IPPROTO_UDP) {
> - if (sk->sk_state != TCP_CLOSE)
> - return sk;
> - } else {
> - return sk;
> - }
> -
> reuse_sk = inet_lookup_reuseport(net, sk, skb, doff,
> saddr, sport, daddr, ntohs(dport),
> ehashfn);
>
> ---
> base-commit: eb62e6aef940fcb1879100130068369d4638088f
> change-id: 20230808-bpf-next-a442a095562b
>
> Best regards,
From: Lorenz Bauer <[email protected]>
Date: Wed, 9 Aug 2023 16:08:31 +0100
> On Wed, Aug 9, 2023 at 3:39 PM Martin KaFai Lau <[email protected]> wrote:
> >
> > On 8/9/23 1:33 AM, Lorenz Bauer wrote:
> > > Kumar reported a KASAN splat in tcp_v6_rcv:
> > >
> > > bash-5.2# ./test_progs -t btf_skc_cls_ingress
> > > ...
> > > [ 51.810085] BUG: KASAN: slab-out-of-bounds in tcp_v6_rcv+0x2d7d/0x3440
> > > [ 51.810458] Read of size 2 at addr ffff8881053f038c by task test_progs/226
> > >
> > > The problem is that inet[6]_steal_sock accesses sk->sk_protocol without
> > > accounting for request sockets. I added the check to ensure that we only
> > > every try to perform a reuseport lookup on a supported socket.
> > >
> > > It turns out that this isn't necessary at all. struct sock_common contains
> > > a skc_reuseport flag which indicates whether a socket is part of a
> >
> > Does it go back to the earlier discussion
> > (https://lore.kernel.org/bpf/[email protected]/)
> > that the sk->sk_reuseport is 1 from sk_clone for TCP_ESTABLISHED? It works
> > because there is sk->sk_reuseport"_cb" check going deeper into
> > reuseport_select_sock() but there is an extra inet6_ehashfn for all TCP_ESTABLISHED.
>
> Sigh, I'd forgotten about this...
>
> For the TPROXY TCP replacement use case we sk_assign the SYN to the
> listener, which creates the reqsk. We can let follow up packets pass
> without sk_assign since they will match the reqsk and convert to a
> fullsock via the usual route. At least that is what the test does. I'm
> not even sure what it means to redirect a random packet into an
> established TCP socket TBH. It'd probably be dropped?
>
> For UDP, I'm not sure whether we even get into this situation? Doesn't
> seem like UDP sockets are cloned from each other, so we also shouldn't
> end up with a reuseport flag set erroneously.
>
> Things we could do if necessary:
> 1. Reset the flag in inet_csk_clone_lock like we do for SOCK_RCU_FREE
I think we can't do this as sk_reuseport is inherited to twsk and used
in inet_bind_conflict().
> 2. Duplicate the cb check into inet[6]_steal_sock
or 3. Add sk_fullsock() test ?
On Wed, Aug 9, 2023 at 3:39 PM Martin KaFai Lau <[email protected]> wrote:
>
> On 8/9/23 1:33 AM, Lorenz Bauer wrote:
> > Kumar reported a KASAN splat in tcp_v6_rcv:
> >
> > bash-5.2# ./test_progs -t btf_skc_cls_ingress
> > ...
> > [ 51.810085] BUG: KASAN: slab-out-of-bounds in tcp_v6_rcv+0x2d7d/0x3440
> > [ 51.810458] Read of size 2 at addr ffff8881053f038c by task test_progs/226
> >
> > The problem is that inet[6]_steal_sock accesses sk->sk_protocol without
> > accounting for request sockets. I added the check to ensure that we only
> > every try to perform a reuseport lookup on a supported socket.
> >
> > It turns out that this isn't necessary at all. struct sock_common contains
> > a skc_reuseport flag which indicates whether a socket is part of a
>
> Does it go back to the earlier discussion
> (https://lore.kernel.org/bpf/[email protected]/)
> that the sk->sk_reuseport is 1 from sk_clone for TCP_ESTABLISHED? It works
> because there is sk->sk_reuseport"_cb" check going deeper into
> reuseport_select_sock() but there is an extra inet6_ehashfn for all TCP_ESTABLISHED.
Sigh, I'd forgotten about this...
For the TPROXY TCP replacement use case we sk_assign the SYN to the
listener, which creates the reqsk. We can let follow up packets pass
without sk_assign since they will match the reqsk and convert to a
fullsock via the usual route. At least that is what the test does. I'm
not even sure what it means to redirect a random packet into an
established TCP socket TBH. It'd probably be dropped?
For UDP, I'm not sure whether we even get into this situation? Doesn't
seem like UDP sockets are cloned from each other, so we also shouldn't
end up with a reuseport flag set erroneously.
Things we could do if necessary:
1. Reset the flag in inet_csk_clone_lock like we do for SOCK_RCU_FREE
2. Duplicate the cb check into inet[6]_steal_sock
Best
Lorenz
>
> > reuseport group. inet[6]_lookup_reuseport already check this flag,
> > so we can't execute an erroneous reuseport lookup by definition.
> >
> > Remove the unnecessary assertions to fix the out of bounds access.
> >
> > Fixes: 9c02bec95954 ("bpf, net: Support SO_REUSEPORT sockets with bpf_sk_assign")
> > Reported-by: Kumar Kartikeya Dwivedi <[email protected]>
> > Signed-off-by: Lorenz Bauer <[email protected]>
> > ---
> > include/net/inet6_hashtables.h | 10 ----------
> > include/net/inet_hashtables.h | 10 ----------
> > 2 files changed, 20 deletions(-)
> >
> > diff --git a/include/net/inet6_hashtables.h b/include/net/inet6_hashtables.h
> > index 284b5ce7205d..f9907ed36d54 100644
> > --- a/include/net/inet6_hashtables.h
> > +++ b/include/net/inet6_hashtables.h
> > @@ -119,16 +119,6 @@ struct sock *inet6_steal_sock(struct net *net, struct sk_buff *skb, int doff,
> > if (!prefetched)
> > return sk;
> >
> > - if (sk->sk_protocol == IPPROTO_TCP) {
> > - if (sk->sk_state != TCP_LISTEN)
> > - return sk;
> > - } else if (sk->sk_protocol == IPPROTO_UDP) {
> > - if (sk->sk_state != TCP_CLOSE)
> > - return sk;
> > - } else {
> > - return sk;
> > - }
> > -
> > reuse_sk = inet6_lookup_reuseport(net, sk, skb, doff,
> > saddr, sport, daddr, ntohs(dport),
> > ehashfn);
> > diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
> > index 1177effabed3..57a46993383a 100644
> > --- a/include/net/inet_hashtables.h
> > +++ b/include/net/inet_hashtables.h
> > @@ -465,16 +465,6 @@ struct sock *inet_steal_sock(struct net *net, struct sk_buff *skb, int doff,
> > if (!prefetched)
> > return sk;
> >
> > - if (sk->sk_protocol == IPPROTO_TCP) {
> > - if (sk->sk_state != TCP_LISTEN)
> > - return sk;
> > - } else if (sk->sk_protocol == IPPROTO_UDP) {
> > - if (sk->sk_state != TCP_CLOSE)
> > - return sk;
> > - } else {
> > - return sk;
> > - }
> > -
> > reuse_sk = inet_lookup_reuseport(net, sk, skb, doff,
> > saddr, sport, daddr, ntohs(dport),
> > ehashfn);
> >
> > ---
> > base-commit: eb62e6aef940fcb1879100130068369d4638088f
> > change-id: 20230808-bpf-next-a442a095562b
> >
> > Best regards,
>
On Wed, Aug 9, 2023 at 4:56 PM Kuniyuki Iwashima <[email protected]> wrote:
>
> > Things we could do if necessary:
> > 1. Reset the flag in inet_csk_clone_lock like we do for SOCK_RCU_FREE
>
> I think we can't do this as sk_reuseport is inherited to twsk and used
> in inet_bind_conflict().
Ok, so what kind of state does reuseport carry in the various states then?
TCP_LISTEN: sk_reuseport && sk_reuseport_cb
TCP_ESTABLISHED: sk_reuseport && !sk_reuseport_cb
TCP_TIME_WAIT: sk_reuseport && !sk_reuseport_cb
Where is sk_reuseport_cb cleared? On clone? Or not at all?
> > 2. Duplicate the cb check into inet[6]_steal_sock
>
> or 3. Add sk_fullsock() test ?
I guess this would be in addition to the convoluted series of checks
I've removed in this patch?
From: Lorenz Bauer <[email protected]>
Date: Wed, 9 Aug 2023 17:55:02 +0100
> On Wed, Aug 9, 2023 at 4:56 PM Kuniyuki Iwashima <[email protected]> wrote:
> >
> > > Things we could do if necessary:
> > > 1. Reset the flag in inet_csk_clone_lock like we do for SOCK_RCU_FREE
> >
> > I think we can't do this as sk_reuseport is inherited to twsk and used
> > in inet_bind_conflict().
>
> Ok, so what kind of state does reuseport carry in the various states then?
>
> TCP_LISTEN: sk_reuseport && sk_reuseport_cb
> TCP_ESTABLISHED: sk_reuseport && !sk_reuseport_cb
> TCP_TIME_WAIT: sk_reuseport && !sk_reuseport_cb
>
> Where is sk_reuseport_cb cleared? On clone? Or not at all?
sk_clone_lock() does when cloning sk from listener, and we
cannot check sk_reuseport_cb for twsk as it doesn't have the
member.
>
> > > 2. Duplicate the cb check into inet[6]_steal_sock
> >
> > or 3. Add sk_fullsock() test ?
>
> I guess this would be in addition to the convoluted series of checks
> I've removed in this patch?
Yes.
On 8/9/23 8:55 AM, Kuniyuki Iwashima wrote:
> From: Lorenz Bauer <[email protected]>
> Date: Wed, 9 Aug 2023 16:08:31 +0100
>> On Wed, Aug 9, 2023 at 3:39 PM Martin KaFai Lau <[email protected]> wrote:
>>>
>>> On 8/9/23 1:33 AM, Lorenz Bauer wrote:
>>>> Kumar reported a KASAN splat in tcp_v6_rcv:
>>>>
>>>> bash-5.2# ./test_progs -t btf_skc_cls_ingress
>>>> ...
>>>> [ 51.810085] BUG: KASAN: slab-out-of-bounds in tcp_v6_rcv+0x2d7d/0x3440
>>>> [ 51.810458] Read of size 2 at addr ffff8881053f038c by task test_progs/226
>>>>
>>>> The problem is that inet[6]_steal_sock accesses sk->sk_protocol without
>>>> accounting for request sockets. I added the check to ensure that we only
>>>> every try to perform a reuseport lookup on a supported socket.
>>>>
>>>> It turns out that this isn't necessary at all. struct sock_common contains
>>>> a skc_reuseport flag which indicates whether a socket is part of a
>>>
>>> Does it go back to the earlier discussion
>>> (https://lore.kernel.org/bpf/[email protected]/)
>>> that the sk->sk_reuseport is 1 from sk_clone for TCP_ESTABLISHED? It works
>>> because there is sk->sk_reuseport"_cb" check going deeper into
>>> reuseport_select_sock() but there is an extra inet6_ehashfn for all TCP_ESTABLISHED.
>>
>> Sigh, I'd forgotten about this...
>>
>> For the TPROXY TCP replacement use case we sk_assign the SYN to the
>> listener, which creates the reqsk. We can let follow up packets pass
>> without sk_assign since they will match the reqsk and convert to a
>> fullsock via the usual route. At least that is what the test does. I'm
>> not even sure what it means to redirect a random packet into an
>> established TCP socket TBH. It'd probably be dropped?
It could act like an earlier early-demux for established sk? If the bpf prog has
already looked up an established sk for other needs (eg. reading the sk local
storage), it may as well bpf_sk_assign it to the skb. I don't have a use case
for that but I also don't see why it won't work also.
>>
>> For UDP, I'm not sure whether we even get into this situation? Doesn't
>> seem like UDP sockets are cloned from each other, so we also shouldn't
>> end up with a reuseport flag set erroneously.
>>
>> Things we could do if necessary:
>> 1. Reset the flag in inet_csk_clone_lock like we do for SOCK_RCU_FREE
>
> I think we can't do this as sk_reuseport is inherited to twsk and used
> in inet_bind_conflict().
>
>
>> 2. Duplicate the cb check into inet[6]_steal_sock
>
> or 3. Add sk_fullsock() test ?
yeah, probably adding sk_fullsock() is needed, may be something like(?):
if (!prefetched || !sk_fullsock(sk))
return sk;
On 8/9/23 10:12 AM, Martin KaFai Lau wrote:
> On 8/9/23 8:55 AM, Kuniyuki Iwashima wrote:
>> From: Lorenz Bauer <[email protected]>
>> Date: Wed, 9 Aug 2023 16:08:31 +0100
>>> On Wed, Aug 9, 2023 at 3:39 PM Martin KaFai Lau <[email protected]> wrote:
>>>>
>>>> On 8/9/23 1:33 AM, Lorenz Bauer wrote:
>>>>> Kumar reported a KASAN splat in tcp_v6_rcv:
>>>>>
>>>>> bash-5.2# ./test_progs -t btf_skc_cls_ingress
>>>>> ...
>>>>> [ 51.810085] BUG: KASAN: slab-out-of-bounds in tcp_v6_rcv+0x2d7d/0x3440
>>>>> [ 51.810458] Read of size 2 at addr ffff8881053f038c by task
>>>>> test_progs/226
>>>>>
>>>>> The problem is that inet[6]_steal_sock accesses sk->sk_protocol without
>>>>> accounting for request sockets. I added the check to ensure that we only
>>>>> every try to perform a reuseport lookup on a supported socket.
>>>>>
>>>>> It turns out that this isn't necessary at all. struct sock_common contains
>>>>> a skc_reuseport flag which indicates whether a socket is part of a
>>>>
>>>> Does it go back to the earlier discussion
>>>> (https://lore.kernel.org/bpf/[email protected]/)
>>>> that the sk->sk_reuseport is 1 from sk_clone for TCP_ESTABLISHED? It works
>>>> because there is sk->sk_reuseport"_cb" check going deeper into
>>>> reuseport_select_sock() but there is an extra inet6_ehashfn for all
>>>> TCP_ESTABLISHED.
>>>
>>> Sigh, I'd forgotten about this...
>>>
>>> For the TPROXY TCP replacement use case we sk_assign the SYN to the
>>> listener, which creates the reqsk. We can let follow up packets pass
>>> without sk_assign since they will match the reqsk and convert to a
>>> fullsock via the usual route. At least that is what the test does. I'm
>>> not even sure what it means to redirect a random packet into an
>>> established TCP socket TBH. It'd probably be dropped?
>
> It could act like an earlier early-demux for established sk? If the bpf prog has
> already looked up an established sk for other needs (eg. reading the sk local
> storage), it may as well bpf_sk_assign it to the skb. I don't have a use case
> for that but I also don't see why it won't work also.
>
>>>
>>> For UDP, I'm not sure whether we even get into this situation? Doesn't
>>> seem like UDP sockets are cloned from each other, so we also shouldn't
>>> end up with a reuseport flag set erroneously.
>>>
>>> Things we could do if necessary:
>>> 1. Reset the flag in inet_csk_clone_lock like we do for SOCK_RCU_FREE
>>
>> I think we can't do this as sk_reuseport is inherited to twsk and used
>> in inet_bind_conflict().
>>
>>
>>> 2. Duplicate the cb check into inet[6]_steal_sock
>>
>> or 3. Add sk_fullsock() test ?
>
> yeah, probably adding sk_fullsock() is needed, may be something like(?):
>
> if (!prefetched || !sk_fullsock(sk))
> return sk;
Friendly ping. Thanks.