Messages in this thread | | | Date | Fri, 26 May 2023 19:57:47 +0300 | Subject | Re: [PATCH] udp6: Fix race condition in udp6_sendmsg & connect | From | Vlad Efanov <> |
| |
Yes.
There is no lock for this lines and my patch does not broken this logic.
I sugessted to set lock only for lines 1566-1571 (ip6_sk_dst_lookup_flow() call).
Best regards,
Vlad.
On 26.05.2023 19:46, Eric Dumazet wrote: > On Fri, May 26, 2023 at 6:09 PM Vlad Efanov <vefanov@ispras.ru> wrote: >> Eric, >> >> >> udp6_sendmsg() currently still locks the socket (on line 1595). >> > Not really, look more closely at lines 1580 -> 1594 > > >> Best regards, >> >> Vlad. >> >> >> On 26.05.2023 18:29, Eric Dumazet wrote: >>> On Fri, May 26, 2023 at 5:08 PM Vladislav Efanov <VEfanov@ispras.ru> 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 >>> Please include a full report. >>> >>>> 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") >>> This is a bogus Fixes: tag >>> >>> In old times, UDP sendmsg() was using the socket lock. >>> >>> Then, in linux-4.0 Vlad Yasevich made UDP v6 sendmsg() lockless (and >>> racy in many points) >>> >>> >>>> Signed-off-by: Vladislav Efanov <VEfanov@ispras.ru> >>>> --- >>>> 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); >>>> dst = ip6_sk_dst_lookup_flow(sk, fl6, final_p, connected); >>>> if (IS_ERR(dst)) { >>>> err = PTR_ERR(dst); >>>> dst = NULL; >>>> + release_sock(sk); >>>> goto out; >>>> } >>>> + release_sock(sk); >>>> >>>> if (ipc6.hlimit < 0) >>>> ipc6.hlimit = ip6_sk_dst_hoplimit(np, fl6, dst); >>>> -- >>>> 2.34.1 >>>> >>> There must be another way really. >>> You just killed UDP performance.
| |