Messages in this thread | | | From | (Eric W. Biederman) | Date | Thu, 22 Mar 2012 15:24:04 -0700 | Subject | Re: [3.3-rc7] sys_poll use after free (hibernate) |
| |
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
| |