Re: [3.3-rc7] sys_poll use after free (hibernate)

[ebiederm@xmission.com (Eric W. Biederman)]

Al Viro <viro@ZenIV.linux.org.uk> writes:

> On Sun, Mar 18, 2012 at 12:02:04PM -0700, Linus Torvalds wrote:
>> and that load is from
>> 
>>     poll_wait(filp, &table->poll->wait, wait);
>> 
>> where the testing of %rsi and %rcx are the "if (p && wait_address)"
>> check in poll_wait(), and %rsi is "table->poll" if I read it all
>> correctly.
>> 
>> And the 6b6b6b6b6b6b6b6b pattern is obviously POISON_FREE, so
>> apparently 'table' has already been freed.
>> 
>> I suspect the whole sysctl 'poll' code is seriously broken, since it
>> seems to depend on those ctl_table pointers being stable over the
>> whole open/close sequence, but if somebody unregisters the sysctl,
>> it's all gone. The ctl_table doesn't have any refcounting etc, and I
>> suspect that your hibernate sequence ends up unregistering some sysctl
>> (perhaps as part of a module unload?)
>
> Ewww...  The way it was supposed to work (prio to ->poll() madness) was
> that actual IO gets wrapped into grab_header()/sysctl_head_finish()
> pair.  proc_sys_poll() doesn't do it, so yes, that post-mortem is
> very likely to be correct.

> Looking at that sucker a bit more: what the hell is proc_sys_setattr()
> doing with vmtruncate(), of all things???  Unless something has changed
> very much and very badly, it does *not* use page cache at all...

sysctl continues not to use the page cache.  The vmtruncate was a generic
vfs level push down that has not been removed as unnecessary in
proc_sysctl.c yet.

The question of how to cleanly implement suppoort for byte level
read/writes of sysctl entries remains an open problem.  But even that
looks like a job for seq_file or cousin of seq_file rather than the page
cache.

> Incidentally, I wonder if we want the whole thing in fs/proc; the argument
> against splitoff to a separate fs used to be "that would break userland
> setups - can't ask people to update /etc/fstab or init scripts to mount
> that thing on /proc/sys".  Fair enough, but... what's to stop us from slapping
> ->d_automount() on /proc/sys like that:
> 	struct vfsmount *mnt = vfs_kern_mount(&sysctlfs_type, 0, "sysctl", 0);
> 	if (!IS_ERR(mnt))
> 		mntget(mnt);
> 	return mnt;
> and we are all set.  IOW, now that ->d_automount() stuff is there, we can
> do that easily without any userland breakage.  Comments?

Is that something we could do with /proc/<pid>/net as well?

I am looking at what it will take to move /proc/sys into /proc/<pid>/sys
so that we can remove the namespace inspired dcache weirdness.  With my
recent cleanups that should be a pretty simple change.

I remember attempting that once before for /proc/<pid>/net and the
review got stalled in getting the expiry logic right.

Eric