netdev - Re: [PATCH v1 net] smc: Fix use-after-free in tcp_write_timer

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]

Message-ID: <ZDOJYGq1wLYipY6X@TONYMAC-ALIBABA.local>
Date:   Mon, 10 Apr 2023 11:58:24 +0800
From:   Tony Lu <tonylu@...ux.alibaba.com>
To:     Kuniyuki Iwashima <kuniyu@...zon.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Karsten Graul <kgraul@...ux.ibm.com>,
        Wenjia Zhang <wenjia@...ux.ibm.com>,
        Jan Karcher <jaka@...ux.ibm.com>,
        Kuniyuki Iwashima <kuni1840@...il.com>, netdev@...r.kernel.org,
        linux-s390@...r.kernel.org,
        syzbot+7e1e1bdb852961150198@...kaller.appspotmail.com
Subject: Re: [PATCH v1 net] smc: Fix use-after-free in
 tcp_write_timer_handler().

Thanks for the fix.

Using net/smc as prefix in subject is preferred.

On Sat, Apr 08, 2023 at 11:49:43AM -0700, Kuniyuki Iwashima wrote:
> With Eric's ref tracker, syzbot finally found a repro for
> use-after-free in tcp_write_timer_handler() by kernel TCP
> sockets. [0]
> 
> If SMC creates a kernel socket in __smc_create(), the kernel
> socket is supposed to be freed in smc_clcsock_release() by
> calling sock_release() when we close() the parent SMC socket.
> 
> However, at the end of smc_clcsock_release(), the kernel
> socket's sk_state might not be TCP_CLOSE.  This means that
> we have not called inet_csk_destroy_sock() in __tcp_close()
> and have not stopped the TCP timers.
> 
> The kernel socket's TCP timers can be fired later, so we
> need to hold a refcnt for net as we do for MPTCP subflows
> in mptcp_subflow_create_socket().

Agreed.

> 
> [0]:
> leaked reference.
>  sk_alloc (./include/net/net_namespace.h:335 net/core/sock.c:2108)
>  inet_create (net/ipv4/af_inet.c:319 net/ipv4/af_inet.c:244)
>  __sock_create (net/socket.c:1546)
>  smc_create (net/smc/af_smc.c:3269 net/smc/af_smc.c:3284)
>  __sock_create (net/socket.c:1546)
>  __sys_socket (net/socket.c:1634 net/socket.c:1618 net/socket.c:1661)
>  __x64_sys_socket (net/socket.c:1672)
>  do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80)
>  entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120)
> ==================================================================
> BUG: KASAN: slab-use-after-free in tcp_write_timer_handler (net/ipv4/tcp_timer.c:378 net/ipv4/tcp_timer.c:624 net/ipv4/tcp_timer.c:594)
> Read of size 1 at addr ffff888052b65e0d by task syzrepro/18091
> 
> CPU: 0 PID: 18091 Comm: syzrepro Tainted: G        W          6.3.0-rc4-01174-gb5d54eb5899a #7
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.0-1.amzn2022.0.1 04/01/2014
> Call Trace:
>  <IRQ>
>  dump_stack_lvl (lib/dump_stack.c:107)
>  print_report (mm/kasan/report.c:320 mm/kasan/report.c:430)
>  kasan_report (mm/kasan/report.c:538)
>  tcp_write_timer_handler (net/ipv4/tcp_timer.c:378 net/ipv4/tcp_timer.c:624 net/ipv4/tcp_timer.c:594)
>  tcp_write_timer (./include/linux/spinlock.h:390 net/ipv4/tcp_timer.c:643)
>  call_timer_fn (./arch/x86/include/asm/jump_label.h:27 ./include/linux/jump_label.h:207 ./include/trace/events/timer.h:127 kernel/time/timer.c:1701)
>  __run_timers.part.0 (kernel/time/timer.c:1752 kernel/time/timer.c:2022)
>  run_timer_softirq (kernel/time/timer.c:2037)
>  __do_softirq (./arch/x86/include/asm/jump_label.h:27 ./include/linux/jump_label.h:207 ./include/trace/events/irq.h:142 kernel/softirq.c:572)
>  __irq_exit_rcu (kernel/softirq.c:445 kernel/softirq.c:650)
>  irq_exit_rcu (kernel/softirq.c:664)
>  sysvec_apic_timer_interrupt (arch/x86/kernel/apic/apic.c:1107 (discriminator 14))
>  </IRQ>
> 
> Fixes: ac7138746e14 ("smc: establish new socket family")
> Reported-by: syzbot+7e1e1bdb852961150198@...kaller.appspotmail.com
> Link: https://lore.kernel.org/netdev/000000000000a3f51805f8bcc43a@google.com/
> Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>

LGTM.

Reviewed-by: Tony Lu <tonylu@...ux.alibaba.com>

> ---
>  net/smc/af_smc.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index c6b4a62276f6..50c38b624f77 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -3270,6 +3270,17 @@ static int __smc_create(struct net *net, struct socket *sock, int protocol,
>  			sk_common_release(sk);
>  			goto out;
>  		}
> +
> +		/* smc_clcsock_release() does not wait smc->clcsock->sk's
> +		 * destruction;  its sk_state might not be TCP_CLOSE after
> +		 * smc->sk is close()d, and TCP timers can be fired later,
> +		 * which need net ref.
> +		 */
> +		sk = smc->clcsock->sk;
> +		__netns_tracker_free(net, &sk->ns_tracker, false);
> +		sk->sk_net_refcnt = 1;
> +		get_net_track(net, &sk->ns_tracker, GFP_KERNEL);
> +		sock_inuse_add(net, 1);
>  	} else {
>  		smc->clcsock = clcsock;
>  	}
> -- 
> 2.30.2