linux-kernel - Re: [PATCH 116/150] vfs,proc: guarantee unique inodes in /proc

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: <1364362839.3520.52.camel@deadeye.wl.decadent.org.uk>
Date:	Wed, 27 Mar 2013 05:40:39 +0000
From:	Ben Hutchings <ben@...adent.org.uk>
To:	Luis Henriques <luis.henriques@...onical.com>
Cc:	linux-kernel@...r.kernel.org, stable@...r.kernel.org,
	kernel-team@...ts.ubuntu.com,
	Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH 116/150] vfs,proc: guarantee unique inodes in /proc

On Tue, 2013-03-26 at 15:20 +0000, Luis Henriques wrote:
> 3.5.7.9 -stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Linus Torvalds <torvalds@...ux-foundation.org>
> 
> commit 51f0885e5415b4cc6535e9cdcc5145bfbc134353 upstream.
> 
> Dave Jones found another /proc issue with his Trinity tool: thanks to
> the namespace model, we can have multiple /proc dentries that point to
> the same inode, aliasing directories in /proc/<pid>/net/ for example.
> 
> This ends up being a total disaster, because it acts like hardlinked
> directories, and causes locking problems.  We rely on the topological
> sort of the inodes pointed to by dentries, and if we have aliased
> directories, that odering becomes unreliable.
> 
> In short: don't do this.  Multiple dentries with the same (directory)
> inode is just a bad idea, and the namespace code should never have
> exposed things this way.  But we're kind of stuck with it.
> 
> This solves things by just always allocating a new inode during /proc
> dentry lookup, instead of using "iget_locked()" to look up existing
> inodes by superblock and number.  That actually simplies the code a bit,
> at the cost of potentially doing more inode [de]allocations.
> 
> That said, the inode lookup wasn't free either (and did a lot of locking
> of inodes), so it is probably not that noticeable.  We could easily keep
> the old lookup model for non-directory entries, but rather than try to
> be excessively clever this just implements the minimal and simplest
> workaround for the problem.
> 
> Reported-and-tested-by: Dave Jones <davej@...hat.com>
> Analyzed-by: Al Viro <viro@...iv.linux.org.uk>
> Signed-off-by: Linus Torvalds <torvalds@...ux-foundation.org>
> [ luis: backported to 3.5; adjust context ]

Prior to commit d3d009cb965eae7e002ea5badf603ea8f4c34915, callers of
proc_get_inode() don't expect it to call pde_put() before returning NULL
- only when returning an existing inode, which it will never do after
this.  So I think you must either cherry-pick that first, or delete
'else pde_put(de);' as part of this fix.

Ben.

> Signed-off-by: Luis Henriques <luis.henriques@...onical.com>
> ---
>  fs/proc/inode.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/proc/inode.c b/fs/proc/inode.c
> index 7ac817b..b02ddd0 100644
> --- a/fs/proc/inode.c
> +++ b/fs/proc/inode.c
> @@ -443,12 +443,10 @@ static const struct file_operations proc_reg_file_ops_no_compat = {
>  
>  struct inode *proc_get_inode(struct super_block *sb, struct proc_dir_entry *de)
>  {
> -	struct inode * inode;
> +	struct inode *inode = new_inode_pseudo(sb);
>  
> -	inode = iget_locked(sb, de->low_ino);
> -	if (!inode)
> -		return NULL;
> -	if (inode->i_state & I_NEW) {
> +	if (inode) {
> +		inode->i_ino = de->low_ino;
>  		inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
>  		PROC_I(inode)->fd = 0;
>  		PROC_I(inode)->pde = de;
> @@ -477,7 +475,6 @@ struct inode *proc_get_inode(struct super_block *sb, struct proc_dir_entry *de)
>  				inode->i_fop = de->proc_fops;
>  			}
>  		}
> -		unlock_new_inode(inode);
>  	} else
>  	       pde_put(de);
>  	return inode;

-- 
Ben Hutchings
I'm not a reverse psychological virus.  Please don't copy me into your sig.

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