linux-kernel - Re: [PATCH 2/2] debugfs: return error values, not NULL

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190123122626.GA27968@kroah.com>
Date:   Wed, 23 Jan 2019 13:26:26 +0100
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Michal Hocko <mhocko@...nel.org>
Cc:     linux-kernel@...r.kernel.org,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        Gary R Hook <ghook@....com>,
        Heiko Carstens <heiko.carstens@...ibm.com>
Subject: Re: [PATCH 2/2] debugfs: return error values, not NULL

On Wed, Jan 23, 2019 at 01:13:50PM +0100, Michal Hocko wrote:
> On Wed 23-01-19 12:55:35, Greg KH wrote:
> > On Wed, Jan 23, 2019 at 12:06:28PM +0100, Michal Hocko wrote:
> > > On Wed 23-01-19 11:28:14, Greg KH wrote:
> > > > When an error happens, debugfs should return an error pointer value, not
> > > > NULL.  This will prevent the totally theoretical error where a debugfs
> > > > call fails due to lack of memory, returning NULL, and that dentry value
> > > > is then passed to another debugfs call, which would end up succeeding,
> > > > creating a file at the root of the debugfs tree, but would then be
> > > > impossible to remove (because you can not remove the directory NULL).
> > > > 
> > > > So, to make everyone happy, always return errors, this makes the users
> > > > of debugfs much simpler (they do not have to ever check the return
> > > > value), and everyone can rest easy.
> > > 
> > > How come this is safe at all? Say you are creating a directory by
> > > debugfs_create_dir and then feed the return value to debugfs_create_files
> > > as a parent. In case of error you are giving it an invalid pointer and
> > > likely blow up unless I miss something.
> > 
> > debugfs_create_files checks for invalid parents and will just refuse to
> > create the file.  It's always done that.
> 
> I must be missing something because debugfs_create_files does
> 	d_inode(parent)->i_private = data;
> as the very first thing and that means that it dereferences an invalid
> pointer right there.

debugfs_create_file() -> __debugfs_create_file() -> start_creating()
and that function checks if parent is an error, which it aborts on, or
if it is NULL, it sets parent to a valid value:

        /* If the parent is not specified, we create it in the root.
         * We need the root dentry to do this, which is in the super
         * block. A pointer to that is in the struct vfsmount that we
         * have around.
         */
        if (!parent)
                parent = debugfs_mount->mnt_root;

I don't see any line that looks like:
>       d_inode(parent)->i_private = data;
in Linus's tree right now, what kernel version are you referring to?

> > > I do agree that reporting errors is better than a simple catch all NULL
> > > but this should have been done when introduced rather than now when most
> > > callers simply check for NULL as a failure.
> > 
> > I'm fixing up all the "NULL is a failure" callsites in the kernel, see
> > lkml for the first round of those patches.
> 
> You are merely removing them, which doesn't really help for this patch.

It doesn't hurt either, as if you really wanted to handle errors from debugfs
properly, you have to check for IS_ERR() as well, because the filesystem can be
compiled out (and then it returns an error pointer)

> And even if it did, you have marked this patch for stable which would
> obviously depend on all such patches to be applied before.

No, all should still work just fine.

thanks,

greg k-h