linux-kernel - Re: [ 31/37] tun: dont zeroize sock->file on detach

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, 19 Aug 2012 18:13:25 +0100
From:	Ben Hutchings <ben@...adent.org.uk>
To:	Stanislav Kinsbursky <skinsbursky@...allels.com>
Cc:	stable@...r.kernel.org, torvalds@...ux-foundation.org,
	akpm@...ux-foundation.org, alan@...rguk.ukuu.org.uk,
	Ruan Zhijie <ruanzhijie@...mail.com>,
	Al Viro <viro@...IV.linux.org.uk>,
	Eric Dumazet <edumazet@...gle.com>,
	Yuchung Cheng <ycheng@...gle.com>,
	"David S. Miller" <davem@...emloft.net>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [ 31/37] tun: dont zeroize sock->file on detach

On Fri, 2012-08-17 at 04:03 +0100, Ben Hutchings wrote:
> 3.2-stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Stanislav Kinsbursky <skinsbursky@...allels.com>
> 
> commit 66d1b9263a371abd15806c53f486f0645ef31a8f upstream.
> 
> This is a fix for bug, introduced in 3.4 kernel by commit
> 1ab5ecb90cb6a3df1476e052f76a6e8f6511cb3d ("tun: don't hold network
> namespace by tun sockets"), which, among other things, replaced simple
> sock_put() by sk_release_kernel(). Below is sequence, which leads to
> oops for non-persistent devices:

I didn't read this message properly when importing cc'd commits.  I'm
going to drop this patch, as it appears that it will result in an inode
leak or other badness if applied to 3.2.y.  Let David Miller know if any
tun fixes should go into 3.2.y.

Ben.

> tun_chr_close()
> tun_detach()				<== tun->socket.file = NULL
> tun_free_netdev()
> sk_release_sock()
> sock_release(sock->file == NULL)
> iput(SOCK_INODE(sock))			<== dereference on NULL pointer
> 
> This patch just removes zeroing of socket's file from __tun_detach().
> sock_release() will do this.
> 
> Reported-by: Ruan Zhijie <ruanzhijie@...mail.com>
> Tested-by: Ruan Zhijie <ruanzhijie@...mail.com>
> Acked-by: Al Viro <viro@...IV.linux.org.uk>
> Acked-by: Eric Dumazet <edumazet@...gle.com>
> Acked-by: Yuchung Cheng <ycheng@...gle.com>
> Signed-off-by: Stanislav Kinsbursky <skinsbursky@...allels.com>
> Signed-off-by: David S. Miller <davem@...emloft.net>
> Signed-off-by: Ben Hutchings <ben@...adent.org.uk>
> ---
>  drivers/net/tun.c |    1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 926d4db..3a16d4f 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -187,7 +187,6 @@ static void __tun_detach(struct tun_struct *tun)
>  	netif_tx_lock_bh(tun->dev);
>  	netif_carrier_off(tun->dev);
>  	tun->tfile = NULL;
> -	tun->socket.file = NULL;
>  	netif_tx_unlock_bh(tun->dev);
>  
>  	/* Drop read queue */
> 

-- 
Ben Hutchings
I say we take off; nuke the site from orbit.  It's the only way to be sure.

Download attachment "signature.asc" of type "application/pgp-signature" (829 bytes)