netdev - Re: [PATCH net] tcp: properly terminate timers for kernel sockets

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: <CANn89iJ-TJY8Bf_6W2yh1F4V0qBNNUKk0NGNT2XJN9Or0oRgdg@mail.gmail.com>
Date: Sat, 23 Mar 2024 05:45:26 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: "David S . Miller" <davem@...emloft.net>, Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org, 
	eric.dumazet@...il.com, Josef Bacik <josef@...icpanda.com>, 
	Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Subject: Re: [PATCH net] tcp: properly terminate timers for kernel sockets

On Fri, Mar 22, 2024 at 11:47 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Fri, 22 Mar 2024 13:57:32 +0000 Eric Dumazet wrote:
> > +     if (!sk->sk_net_refcnt)
> > +             inet_csk_clear_xmit_timers_sync(sk);
>
> The thought that we should clear or poison sk_net at this point
> (whether sk->sk_net_refcnt or not) keeps coming back to me.
> If we don't guarantee the pointer is valid - to make it easier
> for syzbot to catch invalid accesses?

I do not think we should do this here.

Note that KASAN has quarantine, and can catch invalid UAF accesses anyway.

We could clear the base socket in sk_prot_free() but this will not
make KASAN better.

diff --git a/net/core/sock.c b/net/core/sock.c
index 43bf3818c19e829b47d3989d36e2e1b3bf985438..7a3ed6262a7a3c603e3964e7c1b40c82ad9c8bff
100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2110,6 +2110,7 @@ static void sk_prot_free(struct proto *prot,
struct sock *sk)
        cgroup_sk_free(&sk->sk_cgrp_data);
        mem_cgroup_sk_free(sk);
        security_sk_free(sk);
+       memset(sk, 0, sizeof(*sk));
        if (slab != NULL)
                kmem_cache_free(slab, sk);
        else