Messages in this thread | | | Date | Fri, 26 May 2023 18:58:48 +0300 | Subject | Re: [PATCH] udp6: Fix race condition in udp6_sendmsg & connect | From | Ефанов Владислав Алекса ндрович <> |
| |
Paolo,
I don't think that we can just move sk_dst_set() call.
I think we can destroy dst of sendmsg task in this case.
Best regards,
Vlad.
On 26.05.2023 18:33, Paolo Abeni wrote: > On Fri, 2023-05-26 at 18:08 +0300, Vladislav Efanov wrote: >> Syzkaller got the following report: >> BUG: KASAN: use-after-free in sk_setup_caps+0x621/0x690 net/core/sock.c:2018 >> Read of size 8 at addr ffff888027f82780 by task syz-executor276/3255 >> >> The function sk_setup_caps (called by ip6_sk_dst_store_flow-> >> ip6_dst_store) referenced already freed memory as this memory was >> freed by parallel task in udpv6_sendmsg->ip6_sk_dst_lookup_flow-> >> sk_dst_check. >> >> task1 (connect) task2 (udp6_sendmsg) >> sk_setup_caps->sk_dst_set | >> | sk_dst_check-> >> | sk_dst_set >> | dst_release >> sk_setup_caps references | >> to already freed dst_entry| >> >> The reason for this race condition is: udp6_sendmsg() calls >> ip6_sk_dst_lookup() without lock for sock structure and tries to >> allocate/add dst_entry structure to sock structure in parallel with >> "connect" task. >> >> Found by Linux Verification Center (linuxtesting.org) with syzkaller. >> >> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") >> Signed-off-by: Vladislav Efanov <VEfanov@ispras.ru> > Thank you for the detailed report! > >> --- >> net/ipv6/udp.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c >> index e5a337e6b970..a5ecd5d93b0a 100644 >> --- a/net/ipv6/udp.c >> +++ b/net/ipv6/udp.c >> @@ -1563,12 +1563,15 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) >> >> fl6->flowlabel = ip6_make_flowinfo(ipc6.tclass, fl6->flowlabel); >> >> + lock_sock(sk); > Acquiring the socket lock in this fast-path is going to kill the xmit > performances, I think we can't do that. > > What about something like the following instead? Does that addresses > the UaF? (completely untested, not even built ;) If so, feel free to > take it over. > > Thanks. > > Paolo > --- > diff --git a/net/core/sock.c b/net/core/sock.c > index 5440e67bcfe3..24f2761bdb1d 100644 > --- a/net/core/sock.c > +++ b/net/core/sock.c > @@ -2381,7 +2381,6 @@ void sk_setup_caps(struct sock *sk, struct dst_entry *dst) > { > u32 max_segs = 1; > > - sk_dst_set(sk, dst); > sk->sk_route_caps = dst->dev->features; > if (sk_is_tcp(sk)) > sk->sk_route_caps |= NETIF_F_GSO; > @@ -2400,6 +2399,7 @@ void sk_setup_caps(struct sock *sk, struct dst_entry *dst) > } > } > sk->sk_gso_max_segs = max_segs; > + sk_dst_set(sk, dst); > } > EXPORT_SYMBOL_GPL(sk_setup_caps); > >
| |