linux-kernel - Re: 2.6.27-rc7-sha1: EIP at proc_sys

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: <20080928141856.GG28946@ZenIV.linux.org.uk>
Date:	Sun, 28 Sep 2008 15:18:56 +0100
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Alexey Dobriyan <adobriyan@...il.com>, ebiederm@...ssion.com,
	akpm@...ux-foundation.org, linux-kernel@...r.kernel.org
Subject: Re: 2.6.27-rc7-sha1: EIP at proc_sys_compare+0x36/0x50

On Fri, Sep 26, 2008 at 08:47:51AM -0700, Linus Torvalds wrote:

> and as far as I can tell, there is nothing to say that a /proc inode 
> cannot be a negative dentry. Sure, we try to get rid of them, but during a 
> parallel lookup, we will have added the dentry with a NULL inode in the 
> other lookup.
> 
> So assuming that you have an inode at that point seems to be utter crap.
> 
> Now, the whole _function_ is utter crap and should probably be dropped, 
> but whatever. That's just another sysctl insanity. In the meantime, 
> something like this does look appropriate, no?
> 
> Al, did I miss something?

The real underlying bug, whatever it is.  If this sucker ever becomes
negative, we have a big problem.  Where _could_ that happen?  Remember,
we do not allow ->rmdir() and ->unlink() to succeed there.  So d_delete()
callers in namei.c are out of question.  We also never do d_add() with
NULL inode in there.  We _might_ be doing a bogus d_rehash() on a negative
/prooc/sys/<something> dentry that had never been hashed to start with
somewhere in generic code, but... I don't see where that could happen.
vfs_rename_dir() with negative new_dentry would have to get it from
something and that would have to be ->lookup().  And that sucker returns
ERR_PTR() or a positive dentry in all cases here.  d_splice_alias() is not
used there at all; d_move_locked() would scream bloody murder if dentry
it's rehashing is negative.  d_materialize_unique() and d_add_unique()
are not used.  So just WTF is creating this sucker?

IOW, your patch will probably be enough to stop the visible problem, but
I would dearly like to understand what's really causing it.  It appears to
be a refcounting breakage somewhere and we have *another* bug report that
smells like that - it seems like we sometimes end up with negative dentry
on alias list of an inode (outside of /proc/sys, AFAICT).  Something really
fishy is going on...
--
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/