linux-kernel - Re: [RFC 04/10] Temporary struct vfs_lookup in file

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]

Date:	Thu, 9 Aug 2007 00:24:10 +0100
From:	Christoph Hellwig <hch@...radead.org>
To:	Andreas Gruenbacher <agruen@...e.de>
Cc:	Christoph Hellwig <hch@...radead.org>,
	linux-kernel@...r.kernel.org, John Johansen <jjohansen@...e.de>,
	Jan Blunck <jblunck@...e.de>, Erez Zadok <ezk@...sunysb.edu>,
	Josef 'Jeff' Sipek <jsipek@...sunysb.edu>
Subject: Re: [RFC 04/10] Temporary struct vfs_lookup in file_permission

On Wed, Aug 08, 2007 at 11:41:06PM +0200, Andreas Gruenbacher wrote:
> Lookup or not doesn't actually matter. Think of fchdir(2): it does a 
> permission check, and it should also pass down the LOOKUP_CHDIR flag.

fchdir per defintion doesn't do any lookup, and it should not pretend to be
doing one.

> It's true that filesystems should never touch vfsmnts -- except for a few rare 
> exceptions. Filesystem stacking is one. NFS silly-rename is another: if the 
> vfsmnt of the object being silly-renamed were passed down to the file system, 
> we would mntget() it. Right now there is a reference counting bug that allows 
> to blow up the kernel by unmounting that mount point before the silly-renamed 
> file is closed. (It's client-side only of course, but still.)

Wrong. Remember what we call unmount is two underlying operations:

 - detach the subtree from the namespace, this is the vfsmount-based operation.
   this one couldn't care less about an in-progress silly-rename
 - actually teard down the filesystem.  this is a superblock-related
   operation, and you want your reference counting for the above case
   to be on the superblock level if at all.  A good explanation of
   the bug you're seeing and how you intend to fix it outside of this
   slightly heated thread might help, though..

Remember that passing down the vfsmount to the filesystem for namespace
operations is actually harmful, because all the namespace operations must
operate independent of the actual view (aka vfsmount) it's coming from - 
all vfsmounts shared a single dentry subtree and operation on either of
them must give the same results.

> The vfsmnt that 
> this patch passes down in file_permission() is not some crap as you chose to 
> call it, it's the appropriate vfsmnt.

No, it's wrong.  There is no path except for informal purposes attached to
a struct file.  I created file_permission to document that clearly and
people don't try things like this.  There's lots of chances where fs passing
has happened, were into a lazily detached tree, after a pviot_root, etc where
trying to do anything that remotely looks like pathname based operation just
doesn't make any sense in this case.

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