Re: [PATCH bpf-next] net: Fix slab-out-of-bounds in inet[6]_steal_sock

[Lorenz Bauer]

On Wed, Aug 9, 2023 at 3:39 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> 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/7188429a-c380-14c8-57bb-9d05d3ba4e5e@xxxxxxxxx/)
> 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 <memxor@xxxxxxxxx>
> > Signed-off-by: Lorenz Bauer <lmb@xxxxxxxxxxxxx>
> > ---
> >   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,
>