Re: [RFC 0/28] Patches to pass vfsmount to LSM inode security hooks

[Christoph Hellwig]

On Mon, Feb 05, 2007 at 06:13:26PM -0800, Andreas Gruenbacher wrote:
> On Monday 05 February 2007 10:44, Christoph Hellwig wrote:
> > Looking at the actual patches I see you're lazy in a lot of places.
> > Please make sure that when you introduce a vfsmount argument somewhere
> > that it is _always_ passed and not just when it's conveniant.  Yes, that's
> > more work, but then again if you're not consistant anyone half-serious
> > will laught at a security model using this infrasturcture.
> 
> It may appear like laziness, but it's not. Let's look at where we're passing 
> NULL at the moment:

You know, I've tracked a lot of this down previously when I submitted patches
to add vfsmount arguments to the vfs_ helpers, just to get tought by Al
that this is a bad idea :)

> fs/hpfs/namei.c
> 
> 	In hpfs_unlink, hpfs truncates one of its own inodes through
> 	notify_change(). You definitely don't want any lsms to interfere here,
> 	pathname based or not; hpfs should probably truncate its inode itself
> 	instead. But given that hpfs goes via the vfs, we at least pass NULL
> 	to indicate that this file really has no meaningful paths to it
> 	anymore. (In addition, we don't really have a vfsmount at this
> 	point anymore, and neither would it make sense to pass it there.)
> 
> 	To play more nicely with other lsms, hpfs could mark the inode as
> 	private before attempting the truncate.

The right fix would be to not go through the vfs.  Or even better to
remove the utter hack.  See my previous patch on this subject and the
discussion started on it.

> fs/reiserfs/xattr.c
> 
> 	The directories an files that reiserfs uses internally to store xattrs
> 	are hanging off ".reiserfs_priv/xattrs" in the filesystem. This part
> 	of the namespace is not accessible or visible from user space though
> 	except through the xattr syscalls.
> 
> 	Reiserfs should probably just mark all its xattr inodes as private in order
> 	to play nicely with other lsms. As far as pathname based lsms are concerned,
> 	pathnames to those fs-internal objects are meaningless though, and so we
> 	pass NULL here.

Well, the 100% correct fix would be to rip out the braindead reiserfs
xattr code :)
Of course that's not an option, but reiserfs would be much better of
stop using vfs_rmdir.  It really doesn't need most of the checks in there
and should just call into reiserfs_rmdir instead of doing this useless
trip into vfs land.

> fs/sysfs/file.c

> fs/nfsd/vfs.c and fs/nfsd/nfs4recover.c
> 
> 	For nfsd, the concept of pathnames is a bit peculiar. I'll try to respond to
> 	that separately.

the actually nfsd filehandle code is conceptually trivially fixable,
although with quite a bit of code churn.  As mention I'll soon submit
a patch for it.  nfs4recover.c is broken beyond repair and should just
be deleted from the tree.
-
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/