Re: [syzbot] KASAN: use-after-free Read in tcp_retransmit_timer (5)
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: 2022-04-10 11:36:10
Also in:
bpf
On 2022/04/10 2:47, Eric Dumazet wrote:
So please add to your tree the NFS fix:
commit f00432063db1a0db484e85193eccc6845435b80e
Author: Trond Myklebust [off-list ref]
Date: Sun Apr 3 15:58:11 2022 -0400
SUNRPC: Ensure we flush any closed sockets before xs_xprt_free()OK. Since the socket is sk->sk_net_refcnt=0, adding
quoted hunk ↗ jump to hunk
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index e31cf137c6140f76f838b4a0dcddf9f104ad653b..3dacd202bf2af43c55ffe820c08316150d2018ea100644--- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c@@ -2928,6 +2928,8 @@ void tcp_close(struct sock *sk, long timeout) lock_sock(sk); __tcp_close(sk, timeout); release_sock(sk); + if (!sk->sk_net_refcnt) + inet_csk_clear_xmit_timers_sync(sk); sock_put(sk); } EXPORT_SYMBOL(tcp_close);
part indeed helped avoiding use-after-free increment on sock_net(sk). But it seems to me that __sk_destruct() is forever not called. ---------------------------------------- [ 93.024086][ C1] sock: sk_clone_lock(): sk=ffff888110328000 net=ffff88810efb8000 sk->sk_family=10 sk->sk_net_refcnt=0 refcount_read(&net->ns.count)=2 [ 93.030257][ C1] sock: sk_clone_lock(): newsk=ffff888110350000 net=ffff88810efb8000 newsk->sk_family=10 newsk->sk_net_refcnt=0 refcount_read(&net->ns.count)=2 (...snipped...) [ 93.170750][ T740] TCP: Calling inet_csk_clear_xmit_timers_sync() on sock=ffff888110350000 (...snipped...) [ 214.272450][ T8] TCP: Calling inet_csk_clear_xmit_timers_sync() on sock=ffff888110328000 (...snipped...) [ 214.358528][ C3] sock: __sk_destruct(): sk=ffff888110328000 family=10 net=ffff88810efb8000 sk->sk_net_refcnt=0 ---------------------------------------- If I do - inet_csk_clear_xmit_timers_sync(sk); + write_pnet(&sk->sk_net, &init_net); in this patch (i.e. just avoid use-after-free access), __sk_destruct() is called when timer fires. ---------------------------------------- [ 81.969884][ C0] sock: sk_clone_lock(): sk=ffff8880156f8000 net=ffff8881030d8000 sk->sk_family=10 sk->sk_net_refcnt=0 refcount_read(&net->ns.count)=2 [ 81.975329][ C0] sock: sk_clone_lock(): newsk=ffff8880156f8c40 net=ffff8881030d8000 newsk->sk_family=10 newsk->sk_net_refcnt=0 refcount_read(&net->ns.count)=2 (...snipped...) [ 82.078152][ T735] TCP: Resetting sk->sk_net on sock=ffff8880156f8c40 (...snipped...) [ 203.937701][ T735] TCP: Resetting sk->sk_net on sock=ffff8880156f8000 (...snipped...) [ 204.042570][ C1] sock: __sk_destruct(): sk=ffff8880156f8000 family=10 net=ffffffff84588cc0 sk->sk_net_refcnt=0 (...snipped...) [ 214.124851][ C1] sock: __sk_destruct(): sk=ffff8880156f8c40 family=10 net=ffffffff84588cc0 sk->sk_net_refcnt=0 ---------------------------------------- Therefore, I guess that this patch is missing something here.