[PATCH] fs: call security_d_instantiate in d_obtain_alias

[PATCH] fs: call security_d_instantiate in d_obtain_alias

[Josef Bacik]

While trying to track down some NFS problems with BTRFS, I kept noticing I was
getting -EACCESS for no apparent reason.  Eric Paris and printk() helped me
figure out that it was SELinux that was giving me grief, with the following
denial

type=AVC msg=audit(1290013638.413:95): avc:  denied  { 0x800000 } for  pid=1772
comm="nfsd" name="" dev=sda1 ino=256 scontext=system_u:system_r:kernel_t:s0
tcontext=system_u:object_r:unlabeled_t:s0 tclass=file

Turns out this is because in d_obtain_alias if we can't find an alias we create
one and do all the normal instantiation stuff, but we don't do the
security_d_instantiate.  With this patch I'm no longer seeing these errant
-EACCESS return values.  Thanks,

Signed-off-by: Josef Bacik <josef@redhat.com>
---
 fs/dcache.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 23702a9..890a59e 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1201,6 +1201,7 @@ struct dentry *d_obtain_alias(struct inode *inode)
 	spin_unlock(&tmp->d_lock);
 
 	spin_unlock(&dcache_lock);
+	security_d_instantiate(tmp, inode);
 	return tmp;
 
  out_iput:
-- 
1.6.6.1

Re: [PATCH] fs: call security_d_instantiate in d_obtain_alias

[Eric Paris]

On Wed, 2010-11-17 at 12:51 -0500, Josef Bacik wrote:
> While trying to track down some NFS problems with BTRFS, I kept noticing I was
> getting -EACCESS for no apparent reason.  Eric Paris and printk() helped me
> figure out that it was SELinux that was giving me grief, with the following
> denial
> 
> type=AVC msg=audit(1290013638.413:95): avc:  denied  { 0x800000 } for  pid=1772
> comm="nfsd" name="" dev=sda1 ino=256 scontext=system_u:system_r:kernel_t:s0
> tcontext=system_u:object_r:unlabeled_t:s0 tclass=file
> 
> Turns out this is because in d_obtain_alias if we can't find an alias we create
> one and do all the normal instantiation stuff, but we don't do the
> security_d_instantiate.  With this patch I'm no longer seeing these errant
> -EACCESS return values.  Thanks,
> 
> Signed-off-by: Josef Bacik <josef@redhat.com>

Adding a couple of people to the CC just to make sure they notice this
patch.

-Eric

> ---
>  fs/dcache.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 23702a9..890a59e 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -1201,6 +1201,7 @@ struct dentry *d_obtain_alias(struct inode *inode)
>  	spin_unlock(&tmp->d_lock);
>  
>  	spin_unlock(&dcache_lock);
> +	security_d_instantiate(tmp, inode);
>  	return tmp;
>  
>   out_iput:

Re: [PATCH] fs: call security_d_instantiate in d_obtain_alias

[J. Bruce Fields]

On Wed, Nov 17, 2010 at 12:51:03PM -0500, Josef Bacik wrote:
> While trying to track down some NFS problems with BTRFS, I kept noticing I was
> getting -EACCESS for no apparent reason.  Eric Paris and printk() helped me
> figure out that it was SELinux that was giving me grief, with the following
> denial
> 
> type=AVC msg=audit(1290013638.413:95): avc:  denied  { 0x800000 } for  pid=1772
> comm="nfsd" name="" dev=sda1 ino=256 scontext=system_u:system_r:kernel_t:s0
> tcontext=system_u:object_r:unlabeled_t:s0 tclass=file
> 
> Turns out this is because in d_obtain_alias if we can't find an alias we create
> one and do all the normal instantiation stuff, but we don't do the
> security_d_instantiate.  With this patch I'm no longer seeing these errant
> -EACCESS return values.  Thanks,

Possibly dumb question: Is there still a small race here?  Is it
possible for another nfsd thread to find the new alias on the list while
this thread is still:

> 
> Signed-off-by: Josef Bacik <josef@redhat.com>
> ---
>  fs/dcache.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 23702a9..890a59e 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -1201,6 +1201,7 @@ struct dentry *d_obtain_alias(struct inode *inode)
>  	spin_unlock(&tmp->d_lock);
>  
>  	spin_unlock(&dcache_lock);

... right here, so that that other nfsd thread still ends up trying to
do something with a dentry that hasn't had security_d_instantiate called
on it yet?

> +	security_d_instantiate(tmp, inode);
>  	return tmp;
>  
>   out_iput:
> -- 

Or does something else prevent that?

--b.

Re: [PATCH] fs: call security_d_instantiate in d_obtain_alias

[Josef Bacik]

On Wed, Nov 17, 2010 at 02:18:17PM -0500, J. Bruce Fields wrote:
> On Wed, Nov 17, 2010 at 12:51:03PM -0500, Josef Bacik wrote:
> > While trying to track down some NFS problems with BTRFS, I kept noticing I was
> > getting -EACCESS for no apparent reason.  Eric Paris and printk() helped me
> > figure out that it was SELinux that was giving me grief, with the following
> > denial
> > 
> > type=AVC msg=audit(1290013638.413:95): avc:  denied  { 0x800000 } for  pid=1772
> > comm="nfsd" name="" dev=sda1 ino=256 scontext=system_u:system_r:kernel_t:s0
> > tcontext=system_u:object_r:unlabeled_t:s0 tclass=file
> > 
> > Turns out this is because in d_obtain_alias if we can't find an alias we create
> > one and do all the normal instantiation stuff, but we don't do the
> > security_d_instantiate.  With this patch I'm no longer seeing these errant
> > -EACCESS return values.  Thanks,
> 
> Possibly dumb question: Is there still a small race here?  Is it
> possible for another nfsd thread to find the new alias on the list while
> this thread is still:
> 
> > 
> > Signed-off-by: Josef Bacik <josef@redhat.com>
> > ---
> >  fs/dcache.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > diff --git a/fs/dcache.c b/fs/dcache.c
> > index 23702a9..890a59e 100644
> > --- a/fs/dcache.c
> > +++ b/fs/dcache.c
> > @@ -1201,6 +1201,7 @@ struct dentry *d_obtain_alias(struct inode *inode)
> >  	spin_unlock(&tmp->d_lock);
> >  
> >  	spin_unlock(&dcache_lock);
> 
> ... right here, so that that other nfsd thread still ends up trying to
> do something with a dentry that hasn't had security_d_instantiate called
> on it yet?
> 
> > +	security_d_instantiate(tmp, inode);
> >  	return tmp;
> >  
> >   out_iput:
> > -- 
> 
> Or does something else prevent that?
> 

That's a good question, I have no idea actually.  Every other consumer of
security_d_instantiate seems to hold the i_mutex of the parent directory inode,
tho I'm not sure if that is appropriate for d_obtain_alias, maybe somebody else
has an idea?  Thanks,

Josef

Re: [PATCH] fs: call security_d_instantiate in d_obtain_alias

[J. Bruce Fields]

On Wed, Nov 17, 2010 at 02:28:22PM -0500, Josef Bacik wrote:
> On Wed, Nov 17, 2010 at 02:18:17PM -0500, J. Bruce Fields wrote:
> > On Wed, Nov 17, 2010 at 12:51:03PM -0500, Josef Bacik wrote:
> > > While trying to track down some NFS problems with BTRFS, I kept noticing I was
> > > getting -EACCESS for no apparent reason.  Eric Paris and printk() helped me
> > > figure out that it was SELinux that was giving me grief, with the following
> > > denial
> > > 
> > > type=AVC msg=audit(1290013638.413:95): avc:  denied  { 0x800000 } for  pid=1772
> > > comm="nfsd" name="" dev=sda1 ino=256 scontext=system_u:system_r:kernel_t:s0
> > > tcontext=system_u:object_r:unlabeled_t:s0 tclass=file
> > > 
> > > Turns out this is because in d_obtain_alias if we can't find an alias we create
> > > one and do all the normal instantiation stuff, but we don't do the
> > > security_d_instantiate.  With this patch I'm no longer seeing these errant
> > > -EACCESS return values.  Thanks,
> > 
> > Possibly dumb question: Is there still a small race here?  Is it
> > possible for another nfsd thread to find the new alias on the list while
> > this thread is still:
> > 
> > > 
> > > Signed-off-by: Josef Bacik <josef@redhat.com>
> > > ---
> > >  fs/dcache.c |    1 +
> > >  1 files changed, 1 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/fs/dcache.c b/fs/dcache.c
> > > index 23702a9..890a59e 100644
> > > --- a/fs/dcache.c
> > > +++ b/fs/dcache.c
> > > @@ -1201,6 +1201,7 @@ struct dentry *d_obtain_alias(struct inode *inode)
> > >  	spin_unlock(&tmp->d_lock);
> > >  
> > >  	spin_unlock(&dcache_lock);
> > 
> > ... right here, so that that other nfsd thread still ends up trying to
> > do something with a dentry that hasn't had security_d_instantiate called
> > on it yet?
> > 
> > > +	security_d_instantiate(tmp, inode);
> > >  	return tmp;
> > >  
> > >   out_iput:
> > > -- 
> > 
> > Or does something else prevent that?
> > 
> 
> That's a good question, I have no idea actually.  Every other consumer of
> security_d_instantiate seems to hold the i_mutex of the parent directory inode,
> tho I'm not sure if that is appropriate for d_obtain_alias, maybe somebody else
> has an idea?  Thanks,

Actually, I don't get it:

	- Why is selinux using a *dentry* operation to initialize an
	  *inode*?
	- Are security hooks necessarily prepared to handle a
	  disconnected dentry?  (Which has no real parent, name an empty
	  string, etc.)
	- What use is the dentry to the security module in this case
	  anyway?

--b.

Re: [PATCH] fs: call security_d_instantiate in d_obtain_alias

[Eric Paris]

On Wed, 2010-11-17 at 15:26 -0500, J. Bruce Fields wrote:
> On Wed, Nov 17, 2010 at 02:28:22PM -0500, Josef Bacik wrote:
> > On Wed, Nov 17, 2010 at 02:18:17PM -0500, J. Bruce Fields wrote:
> > > On Wed, Nov 17, 2010 at 12:51:03PM -0500, Josef Bacik wrote:
> > > > While trying to track down some NFS problems with BTRFS, I kept noticing I was
> > > > getting -EACCESS for no apparent reason.  Eric Paris and printk() helped me
> > > > figure out that it was SELinux that was giving me grief, with the following
> > > > denial
> > > > 
> > > > type=AVC msg=audit(1290013638.413:95): avc:  denied  { 0x800000 } for  pid=1772
> > > > comm="nfsd" name="" dev=sda1 ino=256 scontext=system_u:system_r:kernel_t:s0
> > > > tcontext=system_u:object_r:unlabeled_t:s0 tclass=file
> > > > 
> > > > Turns out this is because in d_obtain_alias if we can't find an alias we create
> > > > one and do all the normal instantiation stuff, but we don't do the
> > > > security_d_instantiate.  With this patch I'm no longer seeing these errant
> > > > -EACCESS return values.  Thanks,
> > > 
> > > Possibly dumb question: Is there still a small race here?  Is it
> > > possible for another nfsd thread to find the new alias on the list while
> > > this thread is still:
> > > 
> > > > 
> > > > Signed-off-by: Josef Bacik <josef@redhat.com>
> > > > ---
> > > >  fs/dcache.c |    1 +
> > > >  1 files changed, 1 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/fs/dcache.c b/fs/dcache.c
> > > > index 23702a9..890a59e 100644
> > > > --- a/fs/dcache.c
> > > > +++ b/fs/dcache.c
> > > > @@ -1201,6 +1201,7 @@ struct dentry *d_obtain_alias(struct inode *inode)
> > > >  	spin_unlock(&tmp->d_lock);
> > > >  
> > > >  	spin_unlock(&dcache_lock);
> > > 
> > > ... right here, so that that other nfsd thread still ends up trying to
> > > do something with a dentry that hasn't had security_d_instantiate called
> > > on it yet?
> > > 
> > > > +	security_d_instantiate(tmp, inode);
> > > >  	return tmp;
> > > >  
> > > >   out_iput:
> > > > -- 
> > > 
> > > Or does something else prevent that?
> > > 
> > 
> > That's a good question, I have no idea actually.  Every other consumer of
> > security_d_instantiate seems to hold the i_mutex of the parent directory inode,
> > tho I'm not sure if that is appropriate for d_obtain_alias, maybe somebody else
> > has an idea?  Thanks,
> 
> Actually, I don't get it:
> 
> 	- Why is selinux using a *dentry* operation to initialize an
> 	  *inode*?
> 	- Are security hooks necessarily prepared to handle a
> 	  disconnected dentry?  (Which has no real parent, name an empty
> 	  string, etc.)
> 	- What use is the dentry to the security module in this case
> 	  anyway?

I only know a bit from the SELinux world and can't speak at all for any
other LSMs.  SELinux however needs the dentry when an inode first enters
core for a couple of reasons.  (once the inode is in core it should have
already been initialized and we skip all this)

If you have persistent xattr support we need the dentry since the xattr
code requires a dentry.  I have no idea why but that's what
inode->i_op->getxattr() requires.

Then we come to procfs.  In that filesystem we actually label based on
the pathname.  oh no did I say SELinux uses pathnames?  yes, I did, but
we only use the part of the pathname relative to the procfs root, so
really it's a static identifier which is immutable.

>From what I can see SELinux doesn't really care about the dentry, we
don't care about IS_ROOT() or the name or any of that crap (in the
non-procfs case).  All we really about is that if something can find an
inode and use it that security_d_instantiate() was called....

Calling security_d_instantiate() extra times is very low overhead and
not harmful.  the dirtiest (but easiest I guess) fix would be to add it
into the out_iput path as well.  I feel like there has to be an easier
solution, I'm just not sure what it is...

-Eric

Re: [PATCH] fs: call security_d_instantiate in d_obtain_alias

[Josef Bacik]

On Wed, Nov 17, 2010 at 05:12:21PM -0500, Eric Paris wrote:
> On Wed, 2010-11-17 at 15:26 -0500, J. Bruce Fields wrote:
> > On Wed, Nov 17, 2010 at 02:28:22PM -0500, Josef Bacik wrote:
> > > On Wed, Nov 17, 2010 at 02:18:17PM -0500, J. Bruce Fields wrote:
> > > > On Wed, Nov 17, 2010 at 12:51:03PM -0500, Josef Bacik wrote:
> > > > > While trying to track down some NFS problems with BTRFS, I kept noticing I was
> > > > > getting -EACCESS for no apparent reason.  Eric Paris and printk() helped me
> > > > > figure out that it was SELinux that was giving me grief, with the following
> > > > > denial
> > > > > 
> > > > > type=AVC msg=audit(1290013638.413:95): avc:  denied  { 0x800000 } for  pid=1772
> > > > > comm="nfsd" name="" dev=sda1 ino=256 scontext=system_u:system_r:kernel_t:s0
> > > > > tcontext=system_u:object_r:unlabeled_t:s0 tclass=file
> > > > > 
> > > > > Turns out this is because in d_obtain_alias if we can't find an alias we create
> > > > > one and do all the normal instantiation stuff, but we don't do the
> > > > > security_d_instantiate.  With this patch I'm no longer seeing these errant
> > > > > -EACCESS return values.  Thanks,
> > > > 
> > > > Possibly dumb question: Is there still a small race here?  Is it
> > > > possible for another nfsd thread to find the new alias on the list while
> > > > this thread is still:
> > > > 
> > > > > 
> > > > > Signed-off-by: Josef Bacik <josef@redhat.com>
> > > > > ---
> > > > >  fs/dcache.c |    1 +
> > > > >  1 files changed, 1 insertions(+), 0 deletions(-)
> > > > > 
> > > > > diff --git a/fs/dcache.c b/fs/dcache.c
> > > > > index 23702a9..890a59e 100644
> > > > > --- a/fs/dcache.c
> > > > > +++ b/fs/dcache.c
> > > > > @@ -1201,6 +1201,7 @@ struct dentry *d_obtain_alias(struct inode *inode)
> > > > >  	spin_unlock(&tmp->d_lock);
> > > > >  
> > > > >  	spin_unlock(&dcache_lock);
> > > > 
> > > > ... right here, so that that other nfsd thread still ends up trying to
> > > > do something with a dentry that hasn't had security_d_instantiate called
> > > > on it yet?
> > > > 
> > > > > +	security_d_instantiate(tmp, inode);
> > > > >  	return tmp;
> > > > >  
> > > > >   out_iput:
> > > > > -- 
> > > > 
> > > > Or does something else prevent that?
> > > > 
> > > 
> > > That's a good question, I have no idea actually.  Every other consumer of
> > > security_d_instantiate seems to hold the i_mutex of the parent directory inode,
> > > tho I'm not sure if that is appropriate for d_obtain_alias, maybe somebody else
> > > has an idea?  Thanks,
> > 
> > Actually, I don't get it:
> > 
> > 	- Why is selinux using a *dentry* operation to initialize an
> > 	  *inode*?
> > 	- Are security hooks necessarily prepared to handle a
> > 	  disconnected dentry?  (Which has no real parent, name an empty
> > 	  string, etc.)
> > 	- What use is the dentry to the security module in this case
> > 	  anyway?
> 
> I only know a bit from the SELinux world and can't speak at all for any
> other LSMs.  SELinux however needs the dentry when an inode first enters
> core for a couple of reasons.  (once the inode is in core it should have
> already been initialized and we skip all this)
> 
> If you have persistent xattr support we need the dentry since the xattr
> code requires a dentry.  I have no idea why but that's what
> inode->i_op->getxattr() requires.
> 
> Then we come to procfs.  In that filesystem we actually label based on
> the pathname.  oh no did I say SELinux uses pathnames?  yes, I did, but
> we only use the part of the pathname relative to the procfs root, so
> really it's a static identifier which is immutable.
> 
> From what I can see SELinux doesn't really care about the dentry, we
> don't care about IS_ROOT() or the name or any of that crap (in the
> non-procfs case).  All we really about is that if something can find an
> inode and use it that security_d_instantiate() was called....
> 
> Calling security_d_instantiate() extra times is very low overhead and
> not harmful.  the dirtiest (but easiest I guess) fix would be to add it
> into the out_iput path as well.  I feel like there has to be an easier
> solution, I'm just not sure what it is...
>

So we're not worried about calling it multiple times, we're more worried about
somebody finding the dentry before we have a chance to run
security_d_instantiate.  Would it be ok to call security_d_instantiate() right
before we do all the actual instantiation work?  That would solve this problem.
Thanks,

Josef 

Re: [PATCH] fs: call security_d_instantiate in d_obtain_alias

[David Quigley]

[snip]
> If you have persistent xattr support we need the dentry since the xattr
> code requires a dentry.  I have no idea why but that's what
> inode->i_op->getxattr() requires.
>

The original reason that the xattr operations take dentries is because 
of p9fs and CIFS. CIFS uses the name of the file to grab the extended 
attributes and so does p9fs. I had tried to remove this a while ago but 
couldn't find a way around that. When trying to find a solution I also 
got push back from Miklos (FUSE) as he views a filesystem being able to 
make xattr decisions based on the path name being a valid use-case.

Dave

Re: [PATCH] fs: call security_d_instantiate in d_obtain_alias

[J. Bruce Fields]

On Fri, Nov 19, 2010 at 12:28:09AM -0500, David Quigley wrote:
> [snip]
> >If you have persistent xattr support we need the dentry since the xattr
> >code requires a dentry.  I have no idea why but that's what
> >inode->i_op->getxattr() requires.
> >
> 
> The original reason that the xattr operations take dentries is
> because of p9fs and CIFS. CIFS uses the name of the file to grab the
> extended attributes and so does p9fs. I had tried to remove this a
> while ago but couldn't find a way around that.

Both CIFS and FUSE are NFS-exportable, so both allow lookup by
filehandle, so neither can count on getting a filename at this point.

So, out of curiosity, do we know what will happen when selinux asks one
of them for an xattr on a DCACHE_DISCONNECTED dentry?

> When trying to find a
> solution I also got push back from Miklos (FUSE) as he views a
> filesystem being able to make xattr decisions based on the path name
> being a valid use-case.

So selinux may initialize an inode differently depending on which
pathname it happened to be looked up under first?

Factoring the name into the xattr return sounds scary to me.

--b.

Re: [PATCH] fs: call security_d_instantiate in d_obtain_alias

[Dave Quigley]

On 11/19/2010 11:42 AM, J. Bruce Fields wrote:
> On Fri, Nov 19, 2010 at 12:28:09AM -0500, David Quigley wrote:
>> [snip]
>>> If you have persistent xattr support we need the dentry since the xattr
>>> code requires a dentry.  I have no idea why but that's what
>>> inode->i_op->getxattr() requires.
>>>
>> The original reason that the xattr operations take dentries is
>> because of p9fs and CIFS. CIFS uses the name of the file to grab the
>> extended attributes and so does p9fs. I had tried to remove this a
>> while ago but couldn't find a way around that.
> Both CIFS and FUSE are NFS-exportable, so both allow lookup by
> filehandle, so neither can count on getting a filename at this point.
>
> So, out of curiosity, do we know what will happen when selinux asks one
> of them for an xattr on a DCACHE_DISCONNECTED dentry?
>

SELinux uses several methods to determine file labeling. In the policy 
filesystems such as xfs and the ext* series of filesystems are marked as 
fs_use_xattr. In this process the file label is pulled from the 
security.selinux xattr on disk. However CIFS and FUSE (and NFS but our 
Labeled NFS changes are trying to fix this) all have the filesystem 
marked as genfs. When mounting the filesystem the fs_type is looked at 
to determine its labeling type. Since its genfs we lookup what label was 
determined to be the default for that filesystem type. In NFS's current 
state all NFS mounts regardless of version get the uniform label of 
nfs_t for everything listed on an nfs mount point. We have a similar 
situation for cifs and fuse. So in this case SELinux should not be 
asking for the security.selinux xattr from these file systems. However 
if a getxattr call to the security.selinux xattr is made on these 
filesystems it will still work I might be wrong but my understanding is 
just the a dentry in the DCACHE_DISCONNECTED state is not negative but 
just isn't in the tree anymore. So looking at vfs_getxattr I had made 
some modifications a while back to it. Assuming we have permissions to 
access the file determined by xattr_permission and 
security_inode_getxattr we check to see if it is in the security 
namespace.  If it is in the security namespace we call xattr_getsecurity 
which will attempt to get the security label from the inode first 
(security_inode_getsecurity). Because the convention is to call 
d_instantiate on inode create this should always work assuming an LSM is 
loaded. If it fails and we don't have an lsm loaded we fall back to 
checking the getxattr i_op and if that doesn't exist we fail with 
EOPNOTSUPP. That is what should happen on the getxattr call. I don't 
know if something is happening higher up that makes it so we never get 
to vfs_getxattr in the event that the dentry is in the 
DCACHE_DISCONNECTED state. If the DENTRY is disconnected though I'm not 
sure how getxattr from userspace would be able to have access to it 
except through a different name in the namespace.

>> When trying to find a
>> solution I also got push back from Miklos (FUSE) as he views a
>> filesystem being able to make xattr decisions based on the path name
>> being a valid use-case.
> So selinux may initialize an inode differently depending on which
> pathname it happened to be looked up under first?
>
> Factoring the name into the xattr return sounds scary to me.
>

The only current use of determining file label from path name is the 
situation that Eric Paris described with proc. I personally don't agree 
with miklos that the path to the xattr should make it return different 
information (unless im understanding him wrong). However the same thing 
is at work for CIFS as it exposes the windows alternate file streams 
which are accessed by adding the stream name to the end of the filename 
with a separator which I can't remember at the moment. If it was the 
situation that two fuse files shared the same inode and the 
security.selinux xattr was filled differently if it was accessed via 
/fuse/foo and /fuse/bar then yes the situation you described might 
happen. Normally this isn't a problem because file systems don't take 
the path into account so a hardlink to the same inode will still obtain 
the same security label. In reality the xattr is a piece of inode 
metadata and not a piece of dentry metadata.

> --b.
>

Re: [PATCH] fs: call security_d_instantiate in d_obtain_alias

[J. Bruce Fields]

On Sat, Nov 20, 2010 at 11:38:22AM -0500, Dave Quigley wrote:
> On 11/19/2010 11:42 AM, J. Bruce Fields wrote:
> >On Fri, Nov 19, 2010 at 12:28:09AM -0500, David Quigley wrote:
> >>[snip]
> >>>If you have persistent xattr support we need the dentry since the xattr
> >>>code requires a dentry.  I have no idea why but that's what
> >>>inode->i_op->getxattr() requires.
> >>>
> >>The original reason that the xattr operations take dentries is
> >>because of p9fs and CIFS. CIFS uses the name of the file to grab the
> >>extended attributes and so does p9fs. I had tried to remove this a
> >>while ago but couldn't find a way around that.
> >Both CIFS and FUSE are NFS-exportable, so both allow lookup by
> >filehandle, so neither can count on getting a filename at this point.
> >
> >So, out of curiosity, do we know what will happen when selinux asks one
> >of them for an xattr on a DCACHE_DISCONNECTED dentry?
> >
> 
> SELinux uses several methods to determine file labeling. In the
> policy filesystems such as xfs and the ext* series of filesystems
> are marked as fs_use_xattr. In this process the file label is pulled
> from the security.selinux xattr on disk. However CIFS and FUSE (and
> NFS but our Labeled NFS changes are trying to fix this) all have the
> filesystem marked as genfs.

OK, fair enough.  It seems a little fragile to me, but it sounds like
that works....

> When mounting the filesystem the fs_type
> is looked at to determine its labeling type. Since its genfs we
> lookup what label was determined to be the default for that
> filesystem type. In NFS's current state all NFS mounts regardless of
> version get the uniform label of nfs_t for everything listed on an
> nfs mount point. We have a similar situation for cifs and fuse. So
> in this case SELinux should not be asking for the security.selinux
> xattr from these file systems. However if a getxattr call to the
> security.selinux xattr is made on these filesystems it will still
> work I might be wrong but my understanding is just the a dentry in
> the DCACHE_DISCONNECTED state is not negative but just isn't in the
> tree anymore.

More  like "yet" than "anymore"; we've looked up the inode by inode
number (or something similar), not by name, so the dentry in this case
ends up having a name "" and parent itself (like a root dentry).

--b.

> So looking at vfs_getxattr I had made some
> modifications a while back to it. Assuming we have permissions to
> access the file determined by xattr_permission and
> security_inode_getxattr we check to see if it is in the security
> namespace.  If it is in the security namespace we call
> xattr_getsecurity which will attempt to get the security label from
> the inode first (security_inode_getsecurity). Because the convention
> is to call d_instantiate on inode create this should always work
> assuming an LSM is loaded. If it fails and we don't have an lsm
> loaded we fall back to checking the getxattr i_op and if that
> doesn't exist we fail with EOPNOTSUPP. That is what should happen on
> the getxattr call. I don't know if something is happening higher up
> that makes it so we never get to vfs_getxattr in the event that the
> dentry is in the DCACHE_DISCONNECTED state. If the DENTRY is
> disconnected though I'm not sure how getxattr from userspace would
> be able to have access to it except through a different name in the
> namespace.
> 
> >>When trying to find a
> >>solution I also got push back from Miklos (FUSE) as he views a
> >>filesystem being able to make xattr decisions based on the path name
> >>being a valid use-case.
> >So selinux may initialize an inode differently depending on which
> >pathname it happened to be looked up under first?
> >
> >Factoring the name into the xattr return sounds scary to me.
> >
> 
> The only current use of determining file label from path name is the
> situation that Eric Paris described with proc. I personally don't
> agree with miklos that the path to the xattr should make it return
> different information (unless im understanding him wrong). However
> the same thing is at work for CIFS as it exposes the windows
> alternate file streams which are accessed by adding the stream name
> to the end of the filename with a separator which I can't remember
> at the moment. If it was the situation that two fuse files shared
> the same inode and the security.selinux xattr was filled differently
> if it was accessed via /fuse/foo and /fuse/bar then yes the
> situation you described might happen. Normally this isn't a problem
> because file systems don't take the path into account so a hardlink
> to the same inode will still obtain the same security label. In
> reality the xattr is a piece of inode metadata and not a piece of
> dentry metadata.
> 
> >--b.
> >
> 

Re: [PATCH] fs: call security_d_instantiate in d_obtain_alias

[Josef Bacik]

On Wed, Nov 17, 2010 at 02:28:22PM -0500, Josef Bacik wrote:
> On Wed, Nov 17, 2010 at 02:18:17PM -0500, J. Bruce Fields wrote:
> > On Wed, Nov 17, 2010 at 12:51:03PM -0500, Josef Bacik wrote:
> > > While trying to track down some NFS problems with BTRFS, I kept noticing I was
> > > getting -EACCESS for no apparent reason.  Eric Paris and printk() helped me
> > > figure out that it was SELinux that was giving me grief, with the following
> > > denial
> > > 
> > > type=AVC msg=audit(1290013638.413:95): avc:  denied  { 0x800000 } for  pid=1772
> > > comm="nfsd" name="" dev=sda1 ino=256 scontext=system_u:system_r:kernel_t:s0
> > > tcontext=system_u:object_r:unlabeled_t:s0 tclass=file
> > > 
> > > Turns out this is because in d_obtain_alias if we can't find an alias we create
> > > one and do all the normal instantiation stuff, but we don't do the
> > > security_d_instantiate.  With this patch I'm no longer seeing these errant
> > > -EACCESS return values.  Thanks,
> > 
> > Possibly dumb question: Is there still a small race here?  Is it
> > possible for another nfsd thread to find the new alias on the list while
> > this thread is still:
> > 
> > > 
> > > Signed-off-by: Josef Bacik <josef@redhat.com>
> > > ---
> > >  fs/dcache.c |    1 +
> > >  1 files changed, 1 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/fs/dcache.c b/fs/dcache.c
> > > index 23702a9..890a59e 100644
> > > --- a/fs/dcache.c
> > > +++ b/fs/dcache.c
> > > @@ -1201,6 +1201,7 @@ struct dentry *d_obtain_alias(struct inode *inode)
> > >  	spin_unlock(&tmp->d_lock);
> > >  
> > >  	spin_unlock(&dcache_lock);
> > 
> > ... right here, so that that other nfsd thread still ends up trying to
> > do something with a dentry that hasn't had security_d_instantiate called
> > on it yet?
> > 
> > > +	security_d_instantiate(tmp, inode);
> > >  	return tmp;
> > >  
> > >   out_iput:
> > > -- 
> > 
> > Or does something else prevent that?
> > 
> 
> That's a good question, I have no idea actually.  Every other consumer of
> security_d_instantiate seems to hold the i_mutex of the parent directory inode,
> tho I'm not sure if that is appropriate for d_obtain_alias, maybe somebody else
> has an idea?  Thanks,
> 

I'd like some input from the SELinux guys on this, talking with Bruce we can't
really figure out a good way to deal with this.  If we're using the parent
inode->i_mutex to guard the security initalization stuff we're screwed when it
comes to this case, since we have no way to know who the parent is at this point
in time.  Any suggestions?  Thanks,

Josef