Re: BUG: unable to handle kernel paging request in dput (2)

[Greg Kroah-Hartman]

On Thu, Jan 31, 2019 at 11:09:11PM +1300, Kees Cook wrote:
> On Thu, Jan 31, 2019 at 12:41 AM Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Wed, Jan 30, 2019 at 08:26:24PM +0900, Tetsuo Handa wrote:
> > > On 2019/01/30 20:11, Tetsuo Handa wrote:
> > > > Hello, Omar.
> > > >
> > > > syzbot is reporting a crash due to dput(-EINVAL) [1]. I think the location is
> > > >
> > > >         dir = debugfs_lookup(buts->name, blk_debugfs_root);
> > > >         if (!dir)
> > > >                 bt->dir = dir = debugfs_create_dir(buts->name, blk_debugfs_root);
> > > >
> > > > added by commit 6ac93117ab009d39 ("blktrace: use existing disk debugfs directory").
> > > >
> > > > Currently, Greg Kroah-Hartman is posting patches:
> > > >
> > > >   When calling debugfs functions, there is no need to ever check the
> > > >   return value.  The function can work or not, but the code logic should
> > > >   never do something different based on this.
> > > >
> > > > Omar, what do you want to do for this case?
> > > >
> > > > [1] https://syzkaller.appspot.com/bug?extid=b382ba6a802a3d242790
> > > >
> > >
> > > The function which returned -EINVAL instead of NULL seems to be debugfs_lookup()
> > > modified by commit ff9fb72bc07705c0 ("debugfs: return error values, not NULL").
> >
> > Ok, the patch below should fix this up.
> >
> > thanks,
> >
> > greg k-h
> >
> > -------------------------
> >
> > From: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Subject: [PATCH] debugfs: debugfs_lookup() should return NULL if not found
> >
> > Lots of callers of debugfs_lookup() were just checking NULL to see if
> > the file/directory was found or not.  By changing this in ff9fb72bc077
> > ("debugfs: return error values, not NULL") we caused some subsystems to
> > easily crash.
> >
> > Fixes: ff9fb72bc077 ("debugfs: return error values, not NULL")
> > Reported-by: syzbot+b382ba6a802a3d242790@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Reported-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> > Cc: Omar Sandoval <osandov@xxxxxx>
> > Cc: Jens Axboe <axboe@xxxxxx>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > ---
> >  fs/debugfs/inode.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> > index b16f8035b1af..29c68c5d44d5 100644
> > --- a/fs/debugfs/inode.c
> > +++ b/fs/debugfs/inode.c
> > @@ -254,8 +254,8 @@ MODULE_ALIAS_FS("debugfs");
> >   * @parent: a pointer to the parent dentry of the file.
> >   *
> >   * This function will return a pointer to a dentry if it succeeds.  If the file
> > - * doesn't exist or an error occurs, %ERR_PTR(-ERROR) will be returned.  The
> > - * returned dentry must be passed to dput() when it is no longer needed.
> > + * doesn't exist or an error occurs, %NULL will be returned.  The returned
> > + * dentry must be passed to dput() when it is no longer needed.
> >   *
> >   * If debugfs is not enabled in the kernel, the value -%ENODEV will be
> >   * returned.
> > @@ -265,17 +265,17 @@ struct dentry *debugfs_lookup(const char *name, struct dentry *parent)
> >         struct dentry *dentry;
> >
> >         if (IS_ERR(parent))
> > -               return parent;
> > +               return NULL;
> >
> >         if (!parent)
> >                 parent = debugfs_mount->mnt_root;
> >
> >         dentry = lookup_one_len_unlocked(name, parent, strlen(name));
> >         if (IS_ERR(dentry))
> > -               return dentry;
> > +               return NULL;
> >         if (!d_really_is_positive(dentry)) {
> >                 dput(dentry);
> > -               return ERR_PTR(-EINVAL);
> > +               return NULL;
> >         }
> >         return dentry;
> >  }
> > --
> > 2.20.1
> >
> 
> FYI, this patch does not fix the relay.c crash I bisected... I think
> more clean-up is needed?

Yes, you are right, I sent you a patch for that one, here it is as well.

Note, this would be blowing up anyway if debugfs was not enabled, is
relay.c not built if that is not the case?

Ah, no, it isn't, that makes more sense now.

thanks,

greg k-h

diff --git a/kernel/relay.c b/kernel/relay.c
index 04f248644e06..9e0f52375487 100644
--- a/kernel/relay.c
+++ b/kernel/relay.c
@@ -428,6 +428,8 @@ static struct dentry *relay_create_buf_file(struct rchan *chan,
 	dentry = chan->cb->create_buf_file(tmpname, chan->parent,
 					   S_IRUSR, buf,
 					   &chan->is_global);
+	if (IS_ERR(dentry))
+		dentry = NULL;
 
 	kfree(tmpname);
 
@@ -461,7 +463,7 @@ static struct rchan_buf *relay_open_buf(struct rchan *chan, unsigned int cpu)
 		dentry = chan->cb->create_buf_file(NULL, NULL,
 						   S_IRUSR, buf,
 						   &chan->is_global);
-		if (WARN_ON(dentry))
+		if (IS_ERR_OR_NULL(dentry))
 			goto free_buf;
 	}