linux-kernel - Re: remove_proc_entry and read

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: <200701312026.14145.duncan.sands@math.u-psud.fr>
Date:	Wed, 31 Jan 2007 20:26:14 +0100
From:	Duncan Sands <duncan.sands@...h.u-psud.fr>
To:	Alexey Dobriyan <adobriyan@...il.com>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: remove_proc_entry and read_proc

On Wednesday 31 January 2007 19:42:51 Alexey Dobriyan wrote:
> On Wed, Jan 31, 2007 at 11:54:35AM +0100, Duncan Sands wrote:
> > Can read_proc still be executing when remove_proc_entry returns?
> >
> > In my driver [*] I allocate some data and create a proc entry using
> > create_proc_entry.  My read method reads from my allocated data.  When
> > shutting down, I call remove_proc_entry and immediately free the data.
> > If some call to read_proc is still executing at this point then it will
> > be accessing freed memory.  Can this happen?  I've been rummaging around
> > in fs/proc to see what prevents it, but didn't find anything yet.
> 
> This should be fixed by the following patch (in -mm currently):
> http://kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.20-rc6/2.6.20-rc6-mm3/broken-out/fix-rmmod-read-write-races-in-proc-entries.patch
> 
> Tell me if you're unsure it will.

Excellent!  But tell me,

+	atomic_inc(&dp->pde_users);
+	if (!dp->proc_fops)

don't you need a memory barrier between these two?  Also a corresponding
one where proc_fops is set to NULL.


+		/*
+		 * Stop accepting new readers/writers. If you're dynamically
+		 * allocating ->proc_fops, save a pointer somewhere.
+		 */
+		de->proc_fops = NULL;
+		/* Wait until all readers/writers are done. */
+		if (atomic_read(&de->pde_users) > 0) {
+			spin_unlock(&proc_subdir_lock);
+			msleep(1);
+			goto again;
+		}

I don't understand how this is supposed to work.  Consider

CPU1					CPU2

atomic_inc(&dp->pde_users);
if (dp->proc_fops)
					de->proc_fops = NULL;
	use_proc_fops <= BOOM
					if (atomic_read(&de->pde_users) > 0) {

what prevents dereference of a NULL proc_fops value?

Best wishes,

Duncan.
-
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/