linux-kernel - Re: [PATCH v4] Fix rmmod/read/write races in /proc entries

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: <20070213161627.GC6036@localhost.sw.ru>
Date:	Tue, 13 Feb 2007 19:16:27 +0300
From:	Alexey Dobriyan <adobriyan@...nvz.org>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Alexey Dobriyan <adobriyan@...il.com>, viro@....linux.org.uk,
	linux-kernel@...r.kernel.org, duncan.sands@...h.u-psud.fr
Subject: Re: [PATCH v4] Fix rmmod/read/write races in /proc entries

On Mon, Feb 12, 2007 at 10:35:52PM -0800, Andrew Morton wrote:
> On Sun, 11 Feb 2007 23:23:30 +0300 Alexey Dobriyan <adobriyan@...il.com> wrote:
> 
> > [PATCH v4] Fix rmmod/read/write races in /proc entries
> 
> This:
> 
> static ssize_t
> proc_file_write(struct file *file, const char __user *buffer,
> 		size_t count, loff_t *ppos)
> {
> 	struct inode *inode = file->f_path.dentry->d_inode;
> 	struct proc_dir_entry * dp;
> 	ssize_t rv = -EIO;
> 	
> 	dp = PDE(inode);
> 
> 	if (!dp->write_proc)
> 		goto out;
> 
> 	spin_lock(&dp->pde_unload_lock);
> 	if (!dp->proc_fops)
> 		/*
> 		 * remove_proc_entry() marked PDE as "going away".
> 		 * No new writers allowed.
> 		 */
> 		goto out_unlock;
> 
> versus
> 
> 		spin_lock(&de->pde_unload_lock);
> 		/*
> 		 * Stop accepting new readers/writers. If you're dynamically
> 		 * allocating ->proc_fops, save a pointer somewhere.
> 		 */
> 		de->proc_fops = NULL;
> 		/* Wait until all existing readers/writers are done. */
> 		if (de->pde_users > 0) {
> 			struct completion c;
>
> 			init_completion(&c);
> 			if (!de->pde_unload_completion)
> 				de->pde_unload_completion = &c;
>
> 			spin_unlock(&de->pde_unload_lock);
> 			spin_unlock(&proc_subdir_lock);
>
> 			wait_for_completion(de->pde_unload_completion);
>
> 			spin_lock(&proc_subdir_lock);
> 			goto continue_removing;
> 		}
> 		spin_unlock(&de->pde_unload_lock);
>   <here>
> 	...
> 	<free de>
>
> What prevents proc_file_write() from looking up and playing with this de in
> <here>?

If I understood your two-column diagram correctly, scenario below can't
happen because of PDE's own refcount (->count) and existence of
->deleted (0/1)

remove_proc_entry() sees positive ->count and doesn't immediately free
PDE. remove_proc_entry() will at most a) lock b) access to check
->proc_fops which is NULL now, and c) unlock which is fine because
memory is in place.

->count is bumped in proc_get_inode after checking PDEs lists, but our
PDE was already removed from it.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/