Re: general protection fault in relay_open_buf

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, Feb 01, 2019 at 07:31:48AM +1300, Kees Cook wrote:
> On Thu, Jan 31, 2019 at 11:51 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > Can you test the patch below?
> >
> > 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;
> >         }
> >
> 
> Thanks! (Can we find other cases of this with static analysis?)

Probably.  I have over 100 patches to help clean up a lot of the debugfs
mess.  But it is very rare that someone actually tries to use the result
of a debugfs call as a "real" dentry, except to pass it back into
another debugfs call.  I "think" I have now caught all of those cases,
and if you can come up with some kind of rule for this, that would be
great.

But note, the create_buf_file() callback is the one that does the
debugfs call, so trying to figure out where that is coming from, what it
does, and what the dentry is later used for, spans lots of different
subsystems and files.  I don't think we have tools to do that, other
than grep :)

greg k-h