[PATCH] debugfs: debugfs_create_* shouldn't be checking permissions

[PATCH] debugfs: debugfs_create_* shouldn't be checking permissions

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Subject: [PATCH] debugfs: debugfs_create_* shouldn't be checking permissions

Debugfs files and and directories are created by kernel subsystems not
directly by users, so we shouldn't be using lookup_one_len, which checks
permissions.

This was causing krb5 mounts to fail to Fedora servers using gss-proxy
if selinux was enabled, on kernels since 388f0c776781 "sunrpc: add a
debugfs rpc_xprt directory with an info file in it", which creates a new
debugfs directory for each new rpc client.

Reported-by: Anthony Messina <amessina-jVqiHVlHlyFWk0Htik3J/w@public.gmane.org>
Reported-by: Jason Tibbits <tibbs-6Fsk2Ie8wHGVc3sceRu5cw@public.gmane.org>
Cc: Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>
Signed-off-by: J. Bruce Fields <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/debugfs/inode.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

I swiped this code fragment from net/sunrpc/rpc_pipe.c, and it's gotten
only minimal testing.  (It does fix krb5 mounts, though.)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 96400ab42d13..75e5daa6a63f 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -251,6 +251,7 @@ static struct dentry *start_creating(const char *name, struct dentry *parent)
 {
 	struct dentry *dentry;
 	int error;
+	struct qstr q = QSTR_INIT(name, strlen(name));
 
 	pr_debug("debugfs: creating file '%s'\n",name);
 
@@ -268,11 +269,19 @@ static struct dentry *start_creating(const char *name, struct dentry *parent)
 		parent = debugfs_mount->mnt_root;
 
 	mutex_lock(&parent->d_inode->i_mutex);
-	dentry = lookup_one_len(name, parent, strlen(name));
-	if (!IS_ERR(dentry) && dentry->d_inode) {
+	dentry = d_hash_and_lookup(parent, &q);
+	if (!dentry) {
+		dentry = d_alloc(parent, &q);
+		if (!dentry) {
+			dentry = ERR_PTR(-ENOMEM);
+			goto out;
+		}
+	}
+	if (dentry->d_inode) {
 		dput(dentry);
 		dentry = ERR_PTR(-EEXIST);
 	}
+out:
 	if (IS_ERR(dentry))
 		mutex_unlock(&parent->d_inode->i_mutex);
 	return dentry;
@@ -340,7 +349,7 @@ struct dentry *debugfs_create_file(const char *name, umode_t mode,
 	inode->i_mode = mode;
 	inode->i_fop = fops ? fops : &debugfs_file_operations;
 	inode->i_private = data;
-	d_instantiate(dentry, inode);
+	d_add(dentry, inode);
 	fsnotify_create(dentry->d_parent->d_inode, dentry);
 	return end_creating(dentry);
 }
@@ -422,7 +431,7 @@ struct dentry *debugfs_create_dir(const char *name, struct dentry *parent)
 
 	/* directory inodes start off with i_nlink == 2 (for "." entry) */
 	inc_nlink(inode);
-	d_instantiate(dentry, inode);
+	d_add(dentry, inode);
 	inc_nlink(dentry->d_parent->d_inode);
 	fsnotify_mkdir(dentry->d_parent->d_inode, dentry);
 	return end_creating(dentry);
-- 
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] debugfs: debugfs_create_* shouldn't be checking permissions

[Greg Kroah-Hartman]

On Mon, Mar 30, 2015 at 10:23:10AM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> Subject: [PATCH] debugfs: debugfs_create_* shouldn't be checking permissions
> 
> Debugfs files and and directories are created by kernel subsystems not
> directly by users, so we shouldn't be using lookup_one_len, which checks
> permissions.
> 
> This was causing krb5 mounts to fail to Fedora servers using gss-proxy
> if selinux was enabled, on kernels since 388f0c776781 "sunrpc: add a
> debugfs rpc_xprt directory with an info file in it", which creates a new
> debugfs directory for each new rpc client.

No kernel code should care / fail if a debugfs function fails, so please
fix up the sunrpc code first.



> 
> Reported-by: Anthony Messina <amessina-jVqiHVlHlyFWk0Htik3J/w@public.gmane.org>
> Reported-by: Jason Tibbits <tibbs-6Fsk2Ie8wHGVc3sceRu5cw@public.gmane.org>
> Cc: Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>
> Signed-off-by: J. Bruce Fields <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/debugfs/inode.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> I swiped this code fragment from net/sunrpc/rpc_pipe.c, and it's gotten
> only minimal testing.  (It does fix krb5 mounts, though.)
> 
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 96400ab42d13..75e5daa6a63f 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -251,6 +251,7 @@ static struct dentry *start_creating(const char *name, struct dentry *parent)
>  {
>  	struct dentry *dentry;
>  	int error;
> +	struct qstr q = QSTR_INIT(name, strlen(name));
>  
>  	pr_debug("debugfs: creating file '%s'\n",name);
>  
> @@ -268,11 +269,19 @@ static struct dentry *start_creating(const char *name, struct dentry *parent)
>  		parent = debugfs_mount->mnt_root;
>  
>  	mutex_lock(&parent->d_inode->i_mutex);
> -	dentry = lookup_one_len(name, parent, strlen(name));
> -	if (!IS_ERR(dentry) && dentry->d_inode) {
> +	dentry = d_hash_and_lookup(parent, &q);
> +	if (!dentry) {
> +		dentry = d_alloc(parent, &q);
> +		if (!dentry) {
> +			dentry = ERR_PTR(-ENOMEM);
> +			goto out;
> +		}
> +	}
> +	if (dentry->d_inode) {


No, I'd rather not "open code" lookup_one_len() if at all possible
please.

What exactly is the problem here that the sunrpc code is failing from?
Is it just interacting with selinux?  How is the debugfs code to blame
here?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] debugfs: debugfs_create_* shouldn't be checking permissions

[J. Bruce Fields]

On Mon, Mar 30, 2015 at 04:38:23PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Mar 30, 2015 at 10:23:10AM -0400, J. Bruce Fields wrote:
> > From: "J. Bruce Fields" <bfields@redhat.com>
> > 
> > Subject: [PATCH] debugfs: debugfs_create_* shouldn't be checking permissions
> > 
> > Debugfs files and and directories are created by kernel subsystems not
> > directly by users, so we shouldn't be using lookup_one_len, which checks
> > permissions.
> > 
> > This was causing krb5 mounts to fail to Fedora servers using gss-proxy
> > if selinux was enabled, on kernels since 388f0c776781 "sunrpc: add a
> > debugfs rpc_xprt directory with an info file in it", which creates a new
> > debugfs directory for each new rpc client.
> 
> No kernel code should care / fail if a debugfs function fails, so please
> fix up the sunrpc code first.

The sunrpc code is using debugfs to keep a list of all rpc clients
together with some basic information about each one.

If the permissions issue is fixed then I think the only possible
failures are lack of debugfs support and ENOMEM?

If there are situations where ENOMEM's actually possible, I'd worry a
bit about this debugging information becoming unreliable after you go
through some extreme memory shortage (possibly because that's what was
necessary to reproduce the bug you're chasing).

But, I don't know, maybe I'm excessively paranoid; Jeff?

> > Reported-by: Anthony Messina <amessina@messinet.com>
> > Reported-by: Jason Tibbits <tibbs@math.uh.edu>
> > Cc: Jeff Layton <jlayton@primarydata.com>
> > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > ---
> >  fs/debugfs/inode.c | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> > 
> > I swiped this code fragment from net/sunrpc/rpc_pipe.c, and it's gotten
> > only minimal testing.  (It does fix krb5 mounts, though.)
> > 
> > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> > index 96400ab42d13..75e5daa6a63f 100644
> > --- a/fs/debugfs/inode.c
> > +++ b/fs/debugfs/inode.c
> > @@ -251,6 +251,7 @@ static struct dentry *start_creating(const char *name, struct dentry *parent)
> >  {
> >  	struct dentry *dentry;
> >  	int error;
> > +	struct qstr q = QSTR_INIT(name, strlen(name));
> >  
> >  	pr_debug("debugfs: creating file '%s'\n",name);
> >  
> > @@ -268,11 +269,19 @@ static struct dentry *start_creating(const char *name, struct dentry *parent)
> >  		parent = debugfs_mount->mnt_root;
> >  
> >  	mutex_lock(&parent->d_inode->i_mutex);
> > -	dentry = lookup_one_len(name, parent, strlen(name));
> > -	if (!IS_ERR(dentry) && dentry->d_inode) {
> > +	dentry = d_hash_and_lookup(parent, &q);
> > +	if (!dentry) {
> > +		dentry = d_alloc(parent, &q);
> > +		if (!dentry) {
> > +			dentry = ERR_PTR(-ENOMEM);
> > +			goto out;
> > +		}
> > +	}
> > +	if (dentry->d_inode) {
> 
> 
> No, I'd rather not "open code" lookup_one_len() if at all possible
> please.
> 
> What exactly is the problem here that the sunrpc code is failing from?

Sunrpc is creating a new rpc client (to talk to an rpc server).

Debugfs will fail to do that whenever the user doesn't have execute
permissions on a debugfs directory.  The user in this case is whoever
gss-proxy's running as, but I think it could just as well be someone
doing an nfs mount, for example.

> Is it just interacting with selinux?  How is the debugfs code to blame
> here?

As long as the debugfs directories permit "x" to everone, I guess you're
only going to hit this with selinux (or some other security module).

We don't want to require updated selinux policies just because some
subsystem started creating debugfs entries as a side-effect of some
system call (which is what happened here).

--b.

Re: [PATCH] debugfs: debugfs_create_* shouldn't be checking permissions

[Jeff Layton]

On Mon, 30 Mar 2015 11:27:25 -0400
"J. Bruce Fields" <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org> wrote:

> On Mon, Mar 30, 2015 at 04:38:23PM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Mar 30, 2015 at 10:23:10AM -0400, J. Bruce Fields wrote:
> > > From: "J. Bruce Fields" <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > 
> > > Subject: [PATCH] debugfs: debugfs_create_* shouldn't be checking permissions
> > > 
> > > Debugfs files and and directories are created by kernel subsystems not
> > > directly by users, so we shouldn't be using lookup_one_len, which checks
> > > permissions.
> > > 
> > > This was causing krb5 mounts to fail to Fedora servers using gss-proxy
> > > if selinux was enabled, on kernels since 388f0c776781 "sunrpc: add a
> > > debugfs rpc_xprt directory with an info file in it", which creates a new
> > > debugfs directory for each new rpc client.
> > 
> > No kernel code should care / fail if a debugfs function fails, so please
> > fix up the sunrpc code first.
> 
> The sunrpc code is using debugfs to keep a list of all rpc clients
> together with some basic information about each one.
> 
> If the permissions issue is fixed then I think the only possible
> failures are lack of debugfs support and ENOMEM?
> 
> If there are situations where ENOMEM's actually possible, I'd worry a
> bit about this debugging information becoming unreliable after you go
> through some extreme memory shortage (possibly because that's what was
> necessary to reproduce the bug you're chasing).
> 
> But, I don't know, maybe I'm excessively paranoid; Jeff?
> 

The debugfs files are really just nice-to-have things. I don't think
you can read too much into it if they don't show up for some reason.
So, I'm fine with turning those into non-fatal errors.

> > > Reported-by: Anthony Messina <amessina-jVqiHVlHlyFWk0Htik3J/w@public.gmane.org>
> > > Reported-by: Jason Tibbits <tibbs-6Fsk2Ie8wHGVc3sceRu5cw@public.gmane.org>
> > > Cc: Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>
> > > Signed-off-by: J. Bruce Fields <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > ---
> > >  fs/debugfs/inode.c | 17 +++++++++++++----
> > >  1 file changed, 13 insertions(+), 4 deletions(-)
> > > 
> > > I swiped this code fragment from net/sunrpc/rpc_pipe.c, and it's gotten
> > > only minimal testing.  (It does fix krb5 mounts, though.)
> > > 
> > > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> > > index 96400ab42d13..75e5daa6a63f 100644
> > > --- a/fs/debugfs/inode.c
> > > +++ b/fs/debugfs/inode.c
> > > @@ -251,6 +251,7 @@ static struct dentry *start_creating(const char *name, struct dentry *parent)
> > >  {
> > >  	struct dentry *dentry;
> > >  	int error;
> > > +	struct qstr q = QSTR_INIT(name, strlen(name));
> > >  
> > >  	pr_debug("debugfs: creating file '%s'\n",name);
> > >  
> > > @@ -268,11 +269,19 @@ static struct dentry *start_creating(const char *name, struct dentry *parent)
> > >  		parent = debugfs_mount->mnt_root;
> > >  
> > >  	mutex_lock(&parent->d_inode->i_mutex);
> > > -	dentry = lookup_one_len(name, parent, strlen(name));
> > > -	if (!IS_ERR(dentry) && dentry->d_inode) {
> > > +	dentry = d_hash_and_lookup(parent, &q);
> > > +	if (!dentry) {
> > > +		dentry = d_alloc(parent, &q);
> > > +		if (!dentry) {
> > > +			dentry = ERR_PTR(-ENOMEM);
> > > +			goto out;
> > > +		}
> > > +	}
> > > +	if (dentry->d_inode) {
> > 
> > 
> > No, I'd rather not "open code" lookup_one_len() if at all possible
> > please.
> > 
> > What exactly is the problem here that the sunrpc code is failing from?
> 
> Sunrpc is creating a new rpc client (to talk to an rpc server).
> 
> Debugfs will fail to do that whenever the user doesn't have execute
> permissions on a debugfs directory.  The user in this case is whoever
> gss-proxy's running as, but I think it could just as well be someone
> doing an nfs mount, for example.
> 
> > Is it just interacting with selinux?  How is the debugfs code to blame
> > here?
> 
> As long as the debugfs directories permit "x" to everone, I guess you're
> only going to hit this with selinux (or some other security module).
> 
> We don't want to require updated selinux policies just because some
> subsystem started creating debugfs entries as a side-effect of some
> system call (which is what happened here).
> 
> --b.

Right.

-- 
Jeff Layton <jeff.layton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] debugfs: debugfs_create_* shouldn't be checking permissions

[Greg Kroah-Hartman]

On Mon, Mar 30, 2015 at 10:23:10AM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> Subject: [PATCH] debugfs: debugfs_create_* shouldn't be checking permissions

Side-note, why are you putting the Subject: again here in the body of
the patch?

thanks,

greg k-h

Re: [PATCH] debugfs: debugfs_create_* shouldn't be checking permissions

[J. Bruce Fields]

On Mon, Mar 30, 2015 at 05:16:38PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Mar 30, 2015 at 10:23:10AM -0400, J. Bruce Fields wrote:
> > From: "J. Bruce Fields" <bfields@redhat.com>
> > 
> > Subject: [PATCH] debugfs: debugfs_create_* shouldn't be checking permissions
> 
> Side-note, why are you putting the Subject: again here in the body of
> the patch?

Just sloppy cut-and-pasting from git-format-patch output, apologies!

--b.