netdev - Re: [PATCH] net: explicitly clear the sk pointer, when pf->create fails

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: <20241003215038.11611-1-kuniyu@amazon.com>
Date: Thu, 3 Oct 2024 14:50:38 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <ignat@...udflare.com>
CC: <alibuda@...ux.alibaba.com>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<kernel-team@...udflare.com>, <kuba@...nel.org>, <kuniyu@...zon.com>,
	<linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>,
	<pabeni@...hat.com>, <stable@...r.kernel.org>
Subject: Re: [PATCH] net: explicitly clear the sk pointer, when pf->create fails

From: Ignat Korchagin <ignat@...udflare.com>
Date: Thu,  3 Oct 2024 18:01:51 +0100
> We have recently noticed the exact same KASAN splat as in commit
> 6cd4a78d962b ("net: do not leave a dangling sk pointer, when socket
> creation fails"). The problem is that commit did not fully address the
> problem, as some pf->create implementations do not use sk_common_release
> in their error paths.
> 
> For example, we can use the same reproducer as in the above commit, but
> changing ping to arping. arping uses AF_PACKET socket and if packet_create
> fails, it will just sk_free the allocated sk object.
> 
> While we could chase all the pf->create implementations and make sure they
> NULL the freed sk object on error from the socket, we can't guarantee
> future protocols will not make the same mistake.
> 
> So it is easier to just explicitly NULL the sk pointer upon return from
> pf->create in __sock_create. We do know that pf->create always releases the
> allocated sk object on error, so if the pointer is not NULL, it is
> definitely dangling.

Sounds good to me.

Let's remove the change by 6cd4a78d962b that should be unnecessary
with this patch.


> 
> Fixes: 6cd4a78d962b ("net: do not leave a dangling sk pointer, when socket creation fails")
> Signed-off-by: Ignat Korchagin <ignat@...udflare.com>
> Cc: stable@...r.kernel.org
> ---
>  net/socket.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/net/socket.c b/net/socket.c
> index 7b046dd3e9a7..19afac3c2de9 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -1575,8 +1575,13 @@ int __sock_create(struct net *net, int family, int type, int protocol,
>  	rcu_read_unlock();
>  
>  	err = pf->create(net, sock, protocol, kern);
> -	if (err < 0)
> +	if (err < 0) {
> +		/* ->create should release the allocated sock->sk object on error
> +		 * but it may leave the dangling pointer
> +		 */
> +		sock->sk = NULL;
>  		goto out_module_put;
> +	}
>  
>  	/*
>  	 * Now to bump the refcnt of the [loadable] module that owns this
> -- 
> 2.39.5
>