netdev - Re: [PATCH v1 net] net: Fix null-ptr-deref by sock_lock_init_class_and

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] [day] [month] [year] [list]

Message-ID: <20250403191138.20479-1-kuniyu@amazon.com>
Date: Thu, 3 Apr 2025 12:11:28 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <jirislaby@...nel.org>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <ematsumiya@...e.de>,
	<horms@...nel.org>, <kuba@...nel.org>, <kuni1840@...il.com>,
	<kuniyu@...zon.com>, <netdev@...r.kernel.org>, <pabeni@...hat.com>,
	<peterz@...radead.org>, <sfrench@...ba.org>, <stable@...r.kernel.org>,
	<wangzhaolong1@...wei.com>, <willemb@...gle.com>
Subject: Re: [PATCH v1 net] net: Fix null-ptr-deref by sock_lock_init_class_and_name() and rmmod.

From: Jiri Slaby <jirislaby@...nel.org>
Date: Thu, 3 Apr 2025 12:46:43 +0200
> On 03. 04. 25, 4:07, Kuniyuki Iwashima wrote:
> ...
> > --- a/net/core/sock.c
> > +++ b/net/core/sock.c
> > @@ -2324,6 +2324,12 @@ static void __sk_destruct(struct rcu_head *head)
> >   		__netns_tracker_free(net, &sk->ns_tracker, false);
> >   		net_passive_dec(net);
> >   	}
> > +
> > +#if IS_ENABLED(CONFIG_PROVE_LOCKING) && IS_ENABLED(CONFIG_MODULES)
> 
> I don't know if this is the right approach at all (it appears not to 
> me), but:
> 
> Having this check in random files looks error prone. Perhaps you want to 
> introduce some macro like SOCK_NEEDS_OWNER? Or you introduce sk_put_owner().

include/net/sock.h and net/core/sock.c are not random files.

Also, __sk_destruct() is the other single user where all sockets
are destroyed, so we have no reason to make sk_put_owner() available
for modules.

But, I'll define sk_put_owner() with sk_set_owner() under the same
config guard because I noticed I need to clear sk_owner in
sk_clone_lock() for MPTCP, which needs sk_clear_owner() or another
ifdef.


> 
> > +	if (sk->sk_owner)
> 
> The if is not needed.

Exactly, will remove it.

Thanks!

---
pw-bot: cr