Re: [AppArmor 01/41] Pass struct vfsmount to the inode_create LSM hook

Andreas Gruenbacher
2007-05-23T19:06:38Z

On Thursday 12 April 2007 12:12, Al Viro wrote:
> On Thu, Apr 12, 2007 at 02:08:10AM -0700, jjohansen@suse.de wrote:
> > This is needed for computing pathnames in the AppArmor LSM.
>
> Which is an argument against said LSM in current form.

The fundamental model of AppArmor is to perform access checks based on the
location of a file in the filesystem namespace, i.e., the pathname, and this
can only be done with both the dentry and vfsmount. My understanding was that
there was agreement at the last kernel summit that pathname based approaches
should be allowed.

> > - error = security_inode_create(dir, dentry, mode);
> > + error = security_inode_create(dir, dentry, nd ? nd->mnt : NULL, mode);
>
> is a clear sign that interface is wrong.

No. There are callers of vfs_create() that use a NULL nameidata, and that's
what causes the problem here. Struct nameidata is pretty big, and so we don't
want to allocate temporary nameidata objects all over the place. So to me the
above NULL check seems the lesser evil.

One way to deal with the nameidata size problem is to split it up into one
part for the real path lookups, and a much smaller part that only contains
the dentry, vfsmount, and intent flags. This would allow to pass around the
smaller nameidata much more consistently.

John has posted patches for that on May 14; subject [RFD Patch 0/4]. Feedback
appreciated.

In several places, the NULL nameidata is wrong because we can't check the
vfsmount flags or intent. Not having this information is causing problems for
nfs too for example -- it's not an AppArmor specific problem.

> Leaving aside the general idiocy of "we prohibit to do something with file
> if mounted here, but if there's another mountpoint, well, we just miss", an
> API of that kind is just plain wrong. Either you can live without seeing
> vfsmount in that method (in which case you shouldn't pass it at all), or you
> have a hole.

This is backwards from what AppArmor does. The policy defines which paths may
be accessed; all paths not explicitly listed are denied. If files are mounted
at multiple locations, then the policy may allow access to some locations but
not to others. That's not a hole.

In fact this is not much different from traditional permissions on parent
directories: even if the same files are mounted at several locations, parent
directory permissions may allow accessing only some of those locations.

Thanks,
Andreas