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 <[email protected]>
---
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
On Fri, May 26, 2023 at 5:08 PM Vladislav Efanov <[email protected]> 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 <[email protected]>
> ---
> 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.
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 <[email protected]>
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);
Eric,
Here is the full report:
==================================================================
BUG: KASAN: use-after-free in sk_setup_caps+0x621/0x690 net/core/sock.c:2018
Read of size 8 at addr ffff88814c2cc8c0 by task syz-executor.5/22717
CPU: 1 PID: 22717 Comm: syz-executor.5 Not tainted 5.10.179-syzkaller #0
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x107/0x167 lib/dump_stack.c:118
print_address_description.constprop.0+0x1e/0x250 mm/kasan/report.c:384
__kasan_report mm/kasan/report.c:584 [inline]
kasan_report.cold+0x1f/0x37 mm/kasan/report.c:601
sk_setup_caps+0x621/0x690 net/core/sock.c:2018
ip6_dst_store include/net/ip6_route.h:234 [inline]
ip6_sk_dst_store_flow+0x2c9/0x7b0 net/ipv6/route.c:2852
ip6_datagram_dst_update+0x801/0xe30 net/ipv6/datagram.c:107
__ip6_datagram_connect+0x5f2/0x1360 net/ipv6/datagram.c:248
ip6_datagram_connect+0x2b/0x50 net/ipv6/datagram.c:272
inet_dgram_connect+0x150/0x2e0 net/ipv4/af_inet.c:577
__sys_connect_file+0x15c/0x1a0 net/socket.c:1846
__sys_connect+0x165/0x1a0 net/socket.c:1863
__do_sys_connect net/socket.c:1873 [inline]
__se_sys_connect net/socket.c:1870 [inline]
__x64_sys_connect+0x6e/0xb0 net/socket.c:1870
do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46
entry_SYSCALL_64_after_hwframe+0x61/0xc6
RIP: 0033:0x469fe9
Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007f6b6e7e0c08 EFLAGS: 00000246 ORIG_RAX: 000000000000002a
RAX: ffffffffffffffda RBX: 000000000056c030 RCX: 0000000000469fe9
RDX: 000000000000001c RSI: 0000000020000080 RDI: 0000000000000004
RBP: 000000000056c030 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 00007ffdd98ee44f R14: 00007f6b6e7e1700 R15: 0000000000000001
Allocated by task 22717:
kasan_save_stack+0x1b/0x40 mm/kasan/common.c:48
kasan_set_track mm/kasan/common.c:56 [inline]
__kasan_kmalloc.constprop.0+0xc9/0xd0 mm/kasan/common.c:461
slab_post_alloc_hook mm/slab.h:532 [inline]
slab_alloc_node mm/slub.c:2896 [inline]
slab_alloc mm/slub.c:2904 [inline]
kmem_cache_alloc+0x146/0x2e0 mm/slub.c:2909
dst_alloc+0xa0/0x660 net/core/dst.c:93
ip6_blackhole_route+0x61/0x550 net/ipv6/route.c:2535
make_blackhole net/xfrm/xfrm_policy.c:3019 [inline]
xfrm_lookup_route net/xfrm/xfrm_policy.c:3212 [inline]
xfrm_lookup_route+0x109/0x200 net/xfrm/xfrm_policy.c:3203
ip6_dst_lookup_flow+0x159/0x1d0 net/ipv6/ip6_output.c:1235
ip6_datagram_dst_update+0x5d5/0xe30 net/ipv6/datagram.c:89
__ip6_datagram_connect+0x5f2/0x1360 net/ipv6/datagram.c:248
ip6_datagram_connect+0x2b/0x50 net/ipv6/datagram.c:272
inet_dgram_connect+0x150/0x2e0 net/ipv4/af_inet.c:577
__sys_connect_file+0x15c/0x1a0 net/socket.c:1846
__sys_connect+0x165/0x1a0 net/socket.c:1863
__do_sys_connect net/socket.c:1873 [inline]
__se_sys_connect net/socket.c:1870 [inline]
__x64_sys_connect+0x6e/0xb0 net/socket.c:1870
do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46
entry_SYSCALL_64_after_hwframe+0x61/0xc6
Freed by task 5512:
kasan_save_stack+0x1b/0x40 mm/kasan/common.c:48
kasan_set_track+0x1c/0x30 mm/kasan/common.c:56
kasan_set_free_info+0x1b/0x30 mm/kasan/generic.c:355
__kasan_slab_free+0x112/0x170 mm/kasan/common.c:422
slab_free_hook mm/slub.c:1542 [inline]
slab_free_freelist_hook+0xb8/0x1b0 mm/slub.c:1576
slab_free mm/slub.c:3149 [inline]
kmem_cache_free+0xaa/0x2e0 mm/slub.c:3165
dst_destroy+0x2c1/0x3c0 net/core/dst.c:129
rcu_do_batch kernel/rcu/tree.c:2492 [inline]
rcu_core+0x649/0x1310 kernel/rcu/tree.c:2733
__do_softirq+0x1d4/0x8d3 kernel/softirq.c:298
Last call_rcu():
kasan_save_stack+0x1b/0x40 mm/kasan/common.c:48
kasan_record_aux_stack+0xad/0xc0 mm/kasan/generic.c:346
__call_rcu kernel/rcu/tree.c:2974 [inline]
call_rcu+0xb6/0x950 kernel/rcu/tree.c:3048
dst_release net/core/dst.c:179 [inline]
dst_release+0x7e/0xe0 net/core/dst.c:169
sk_dst_set include/net/sock.h:2024 [inline]
sk_setup_caps+0x95/0x690 net/core/sock.c:2017
ip6_dst_store include/net/ip6_route.h:234 [inline]
ip6_sk_dst_store_flow+0x2c9/0x7b0 net/ipv6/route.c:2852
ip6_sk_dst_lookup_flow+0x641/0x9a0 net/ipv6/ip6_output.c:1269
udpv6_sendmsg+0x183f/0x2d10 net/ipv6/udp.c:1522
inet6_sendmsg+0x105/0x140 net/ipv6/af_inet6.c:651
sock_sendmsg_nosec net/socket.c:651 [inline]
sock_sendmsg+0xf2/0x190 net/socket.c:671
____sys_sendmsg+0x32e/0x870 net/socket.c:2356
___sys_sendmsg+0x100/0x170 net/socket.c:2410
__sys_sendmmsg+0x192/0x460 net/socket.c:2496
__do_sys_sendmmsg net/socket.c:2525 [inline]
__se_sys_sendmmsg net/socket.c:2522 [inline]
__x64_sys_sendmmsg+0x98/0x100 net/socket.c:2522
do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46
entry_SYSCALL_64_after_hwframe+0x61/0xc6
Second to last call_rcu():
kasan_save_stack+0x1b/0x40 mm/kasan/common.c:48
kasan_record_aux_stack+0xad/0xc0 mm/kasan/generic.c:346
__call_rcu kernel/rcu/tree.c:2974 [inline]
call_rcu+0xb6/0x950 kernel/rcu/tree.c:3048
dst_release net/core/dst.c:179 [inline]
dst_release+0x7e/0xe0 net/core/dst.c:169
rawv6_sendmsg+0xf73/0x3cf0 net/ipv6/raw.c:964
inet_sendmsg+0x11d/0x140 net/ipv4/af_inet.c:817
sock_sendmsg_nosec net/socket.c:651 [inline]
sock_sendmsg+0x13c/0x190 net/socket.c:671
____sys_sendmsg+0x32e/0x870 net/socket.c:2356
___sys_sendmsg+0x100/0x170 net/socket.c:2410
__sys_sendmmsg+0x192/0x460 net/socket.c:2496
__do_sys_sendmmsg net/socket.c:2525 [inline]
__se_sys_sendmmsg net/socket.c:2522 [inline]
__x64_sys_sendmmsg+0x98/0x100 net/socket.c:2522
do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46
entry_SYSCALL_64_after_hwframe+0x61/0xc6
The buggy address belongs to the object at ffff88814c2cc8c0
which belongs to the cache ip6_dst_cache of size 232
The buggy address is located 0 bytes inside of
232-byte region [ffff88814c2cc8c0, ffff88814c2cc9a8)
The buggy address belongs to the page:
page:000000009e9a5247 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x14c2cc
head:000000009e9a5247 order:1 compound_mapcount:0
flags: 0x57ffe0000010200(slab|head)
raw: 057ffe0000010200 dead000000000100 dead000000000122 ffff888019cc8dc0
raw: 0000000000000000 0000000080190019 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected
Memory state around the buggy address:
ffff88814c2cc780: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ffff88814c2cc800: 00 00 00 00 00 00 00 00 00 00 00 00 00 fc fc fc
>ffff88814c2cc880: fc fc fc fc fc fc fc fc fa fb fb fb fb fb fb fb
^
ffff88814c2cc900: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff88814c2cc980: fb fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc
==================================================================
Best regards,
Vlad.
On 26.05.2023 18:29, Eric Dumazet wrote:
> On Fri, May 26, 2023 at 5:08 PM Vladislav Efanov <[email protected]> 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 <[email protected]>
>> ---
>> 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.
Eric,
udp6_sendmsg() currently still locks the socket (on line 1595).
Best regards,
Vlad.
On 26.05.2023 18:29, Eric Dumazet wrote:
> On Fri, May 26, 2023 at 5:08 PM Vladislav Efanov <[email protected]> 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 <[email protected]>
>> ---
>> 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.
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 <[email protected]>
> 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);
>
>
On Fri, May 26, 2023 at 5:58 PM Ефанов Владислав Александрович
<[email protected]> wrote:
>
> 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.
>
dst are RCU protected, it should be easy to make sure we respect all the rules.
sk_dst_set() is called by sk_setup_caps().
sk_dst_set() replaces dst in socket using xchg() call and we still have
two tasks use one socket but expect different dst in sk_dst_cache.
__sk_dst_set() is rcu protected, but it checks for socket lock.
static inline void
__sk_dst_set(struct sock *sk, struct dst_entry *dst)
{
struct dst_entry *old_dst;
sk_tx_queue_clear(sk);
sk->sk_dst_pending_confirm = 0;
old_dst = rcu_dereference_protected(sk->sk_dst_cache,
lockdep_sock_is_held(sk));
rcu_assign_pointer(sk->sk_dst_cache, dst);
dst_release(old_dst);
}
Best regards.
Vlad.
On 26.05.2023 19:00, Eric Dumazet wrote:
> On Fri, May 26, 2023 at 5:58 PM Ефанов Владислав Александрович
> <[email protected]> wrote:
>> 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.
>>
> dst are RCU protected, it should be easy to make sure we respect all the rules.
On Fri, May 26, 2023 at 6:41 PM Vlad Efanov <[email protected]> wrote:
>
> sk_dst_set() is called by sk_setup_caps().
>
> sk_dst_set() replaces dst in socket using xchg() call and we still have
> two tasks use one socket but expect different dst in sk_dst_cache.
>
>
> __sk_dst_set() is rcu protected, but it checks for socket lock.
>
>
> static inline void
> __sk_dst_set(struct sock *sk, struct dst_entry *dst)
> {
> struct dst_entry *old_dst;
>
> sk_tx_queue_clear(sk);
> sk->sk_dst_pending_confirm = 0;
> old_dst = rcu_dereference_protected(sk->sk_dst_cache,
> lockdep_sock_is_held(sk));
> rcu_assign_pointer(sk->sk_dst_cache, dst);
> dst_release(old_dst);
> }
I am quite familiar with this code.
What are you trying to say exactly ?
Please come with a V2 without grabbing the socket lock.
On Fri, May 26, 2023 at 6:09 PM Vlad Efanov <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >> ---
> >> 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.
I will rework the patch.
Best regards,
Vlad.
On 26.05.2023 19:47, Eric Dumazet wrote:
> On Fri, May 26, 2023 at 6:41 PM Vlad Efanov <[email protected]> wrote:
>> sk_dst_set() is called by sk_setup_caps().
>>
>> sk_dst_set() replaces dst in socket using xchg() call and we still have
>> two tasks use one socket but expect different dst in sk_dst_cache.
>>
>>
>> __sk_dst_set() is rcu protected, but it checks for socket lock.
>>
>>
>> static inline void
>> __sk_dst_set(struct sock *sk, struct dst_entry *dst)
>> {
>> struct dst_entry *old_dst;
>>
>> sk_tx_queue_clear(sk);
>> sk->sk_dst_pending_confirm = 0;
>> old_dst = rcu_dereference_protected(sk->sk_dst_cache,
>> lockdep_sock_is_held(sk));
>> rcu_assign_pointer(sk->sk_dst_cache, dst);
>> dst_release(old_dst);
>> }
> I am quite familiar with this code.
>
> What are you trying to say exactly ?
>
> Please come with a V2 without grabbing the socket lock.
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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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.
On Fri, 2023-05-26 at 18:58 +0300, Ефанов Владислав Александрович
wrote:
> 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.
AFAICS ip6_sk_dst_lookup_flow tries to acquire a reference to the
cached dst. If the connect() clears the cache, decreasing the refcnt,
the counter of the dst in use by sendmsg() must still be non zero.
IMHO the problem you see is that sk_setup_caps() keeps using the dst
after transferring the ownership to the dst cache, which is illegal.
The suggested patch addressed that.
If I'm wrong your syzkaller repro will keep splatting. Please have just
have a spin, thanks.
Paolo
Thank you for detail information.
The patch is reworked and being tested now.
Vlad.
On 26.05.2023 21:13, Paolo Abeni wrote:
> On Fri, 2023-05-26 at 18:58 +0300, Ефанов Владислав Александрович
> wrote:
>> 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.
> AFAICS ip6_sk_dst_lookup_flow tries to acquire a reference to the
> cached dst. If the connect() clears the cache, decreasing the refcnt,
> the counter of the dst in use by sendmsg() must still be non zero.
>
> IMHO the problem you see is that sk_setup_caps() keeps using the dst
> after transferring the ownership to the dst cache, which is illegal.
> The suggested patch addressed that.
>
> If I'm wrong your syzkaller repro will keep splatting. Please have just
> have a spin, thanks.
>
> Paolo
>
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: sk_setup_caps() keeps using
the dst after transferring the ownership to the dst cache.
Found by Linux Verification Center (linuxtesting.org) with syzkaller.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Vladislav Efanov <[email protected]>
Signed-off-by: Paolo Abeni <[email protected]>
---
v2: Move sk_dst_set() call in sk_setup_caps() as
Paolo Abeni <[email protected]> suggested.
net/core/sock.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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);
--
2.34.1
Hello:
This patch was applied to netdev/net.git (main)
by David S. Miller <[email protected]>:
On Tue, 30 May 2023 14:39:41 +0300 you 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.
>
> [...]
Here is the summary with links:
- [v2] udp6: Fix race condition in udp6_sendmsg & connect
https://git.kernel.org/netdev/net/c/448a5ce1120c
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html