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

From: Eric W. Biederman
Date: Thu Mar 22 2012 - 18:20:30 EST


Al Viro <viro@xxxxxxxxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/