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: <20190123121350.GX4087@dhcp22.suse.cz>
Date:   Wed, 23 Jan 2019 13:13:50 +0100
From:   Michal Hocko <mhocko@...nel.org>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.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 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.

> > 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.
And even if it did, you have marked this patch for stable which would
obviously depend on all such patches to be applied before.

-- 
Michal Hocko
SUSE Labs