linux-kernel - Re: [PATCH v2 03/10] blktrace: fix debugfs use after free

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]

Date:   Wed, 22 Apr 2020 01:10:11 -0700
From:   Christoph Hellwig <hch@...radead.org>
To:     Luis Chamberlain <mcgrof@...nel.org>
Cc:     Christoph Hellwig <hch@...radead.org>, axboe@...nel.dk,
        viro@...iv.linux.org.uk, bvanassche@....org,
        gregkh@...uxfoundation.org, rostedt@...dmis.org, mingo@...hat.com,
        jack@...e.cz, ming.lei@...hat.com, nstange@...e.de,
        akpm@...ux-foundation.org, mhocko@...e.com, yukuai3@...wei.com,
        linux-block@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Omar Sandoval <osandov@...com>,
        Hannes Reinecke <hare@...e.com>,
        Michal Hocko <mhocko@...nel.org>,
        syzbot+603294af2d01acfdd6da@...kaller.appspotmail.com
Subject: Re: [PATCH v2 03/10] blktrace: fix debugfs use after free

On Wed, Apr 22, 2020 at 07:48:02AM +0000, Luis Chamberlain wrote:
> > I don't see why we need this check.  If it is valueable enough we
> > should have a debugfs_create_dir_exclusive or so that retunrns an error
> > for an exsting directory, instead of reimplementing it in the caller in
> > a racy way.  But I'm not really sure we need it to start with.
> 
> In short races, and even with synchronous request_queue removal I'm
> seeing the race is still possible, but that's due to some other races
> I'm going to chase down now.
> 
> The easier solution really is to just have a debugfs dir created for
> each partition if debugfs is enabled, this way the directory will
> always be there, and the lookups are gone.

That sounds like the best plan to me.

> 
> > > +
> > > +	q->debugfs_dir = debugfs_create_dir(kobject_name(q->kobj.parent),
> > > +					    blk_debugfs_root);
> > > +	if (!q->debugfs_dir)
> > > +		return -ENOMEM;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +void blk_queue_debugfs_unregister(struct request_queue *q)
> > > +{
> > > +	debugfs_remove_recursive(q->debugfs_dir);
> > > +	q->debugfs_dir = NULL;
> > > +}
> > 
> > Which to me suggests we can just fold these two into the callers,
> > with an IS_ENABLED for the creation case given that we check for errors
> > and the stub will always return an error.
> 
> Sorry not sure I follow this.

Don't both with the two above functions and just open code them in
the callers.  IFF you still want to check for errors after the
discussion with Greg, wrap the call in a

	if (IS_ENABLED(CONFIG_DEBUG_FS))

to ensure that you don't fail queue creation in the !DEBUG_FS
case.