Re: [nameidata 1/2] Don't pass NULL nameidata to vfs_create

[Andreas Gruenbacher <agruen@suse.de>]

On Monday 16 April 2007 18:21, Christoph Hellwig wrote:
> On Mon, Apr 16, 2007 at 06:11:30PM +0200, Andreas Gruenbacher wrote:
> > On Thursday 12 April 2007 12:06, Christoph Hellwig wrote:
> > > Once again very strong NACK.  Every conditional passing of vfsmounts get 
> > > my veto.  As mentioned last time if you really want this send a patch
> > > series first that passed the vfsmount consistantly.
> > 
> > I don't consider it fair to NACK this patch just because some other part
> > of the kernel uses vfs_create() in the wrong way -- those are really
> > independent issues. The bug is not that hard to fix though; here is a
> > proposed patch on top of the other AppArmor patches.
> 
> Note that it's not just this particular hunk, all these kinds of conditional
> passing are wrong.

Yes, the vfs uses or passes through a NULL nameidata or vfsmount pointer and 
no intent information in several places:

 (1) vfs_create() is called with a NULL nameidata in two places,

 (2) lookup_one_len() and lookup_one_len_kern() pass a NULL nameidata to
     permission(), d_op->d_revalidate(), and i_op->lookup(),

 (3) file_permission() passes a NULL nameidata to permission().

 (4) permission() is directly called with NULL nameidata in some places, and

In general it is incorrect to use NULL nameidata or vfsmounts because then the
vfsmount->mnt_flags cannot be checked, and no intent information is available. 
Intents are an optional optimization for remote filesystems; we can simply 
ignore them for local filesystems. There are some cases where we are passing 
NULL which are real bugs, but there are also other cases in which the 
operations are not mount related: for example, filesystems like pipefs have 
the MS_NOUSER flag set which indicates that they cannot be mounted at all. On 
filesystems like sysfs, files are created and removed whether or not that 
filesystem is mounted.

The places where we should pass a proper nameidata / vfsmount but don't should 
be fixed. Those are long-standing issues in the vfs though, and at least some 
are not easy to correct. AppArmor is affected by some of the NULL vfsmount 
passing, and we found a few more problems when looking into this more 
closely, but most of that NULL passing doesn't affect AppArmor:

 * In some places, vfs functions are called without nameidata / vfsmount,
   followed by calling lsm hooks with a vfsmount. In those cases, the vfs
   functions cannot check the vfsmount flags, but the lsm hooks will still
   do the appropriate checks.

 * In the AppArmor security model, directory searching is always allowed, and
   the access check are done once the leaf dentry + vfsmount has been looked
   up. For example, granting write access to /var/tmp/foo in a profile is
   sufficient to allow that access; search access to /var and /var/tmp does
   not need to be specified. (The regular inode permission checks are of
   course still performed.)

 * For filesystems like nfsd, the concept of pathname based access control
   doesn't make sense because of properties of the protocol: there is no
   reliable mapping from an nfs file handle to a particular file; aliases
   to the same file cannot be distinguished. (The AppArmor techdoc [1]
   describes this in more detail.)

   Not allowing to confine nfsd by AppArmor doesn't hurt much: nfsd provides
   its own export access control mechanisms. Also, nfsd cannot really be
   confined in a secure way because of how it is implemented in the kernel.
   Gfs2 may have similar properties; I didn't actually look into the protocol.

So I propose to separate the vfs NULL nameidata / vfsmount passing discussion 
from the AppArmor discussion. I have no problem working on the vfs fixes -- 
we could some nice cleanups there -- but that really is independent of 
AppArmor.

We almost have the revised AppArmor patches ready. We'll include all the fixes 
that are truly necessary, and everything beyond that will be posted 
separately.

> But anyway, creating fake nameidata structures is not really helpful.
> If there is a nameidata passed people expect it to be complete, and
> if you pass them to an LSM people will e.g. try to look into lookup
> intents.

Splitting up nameidata helps somewhat here. In cases where we don't have 
intent information available we can zero out the intent flags, and so in the 
worst case, we won't get the intent optimizations.

Thanks,
Andreas