netdev - Re: [syzbot] KASAN: use-after-free Read in tcp_retransmit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 10 Apr 2022 20:36:02 +0900
From:   Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     bpf <bpf@...r.kernel.org>,
        syzbot <syzbot+694120e1002c117747ed@...kaller.appspotmail.com>,
        Andrii Nakryiko <andrii@...nel.org>,
        Andrii Nakryiko <andriin@...com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        David Miller <davem@...emloft.net>,
        David Ahern <dsahern@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        Martin KaFai Lau <kafai@...com>,
        KP Singh <kpsingh@...nel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Alexey Kuznetsov <kuznet@....inr.ac.ru>,
        netdev <netdev@...r.kernel.org>,
        Song Liu <songliubraving@...com>,
        syzkaller-bugs <syzkaller-bugs@...glegroups.com>,
        tpa@...hospital.com, Yonghong Song <yhs@...com>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Trond Myklebust <trondmy@...merspace.com>
Subject: Re: [syzbot] KASAN: use-after-free Read in tcp_retransmit_timer (5)

On 2022/04/10 2:47, Eric Dumazet wrote:
> So please add to your tree the NFS fix:
> 
> commit f00432063db1a0db484e85193eccc6845435b80e
> Author: Trond Myklebust <trond.myklebust@...merspace.com>
> 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

> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index e31cf137c6140f76f838b4a0dcddf9f104ad653b..3dacd202bf2af43c55ffe820c08316150d2018ea
> 100644
> --- 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.