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]

Message-ID: <20200422082604.GT11244@42.do-not-panic.com>
Date:   Wed, 22 Apr 2020 08:26:04 +0000
From:   Luis Chamberlain <mcgrof@...nel.org>
To:     Christoph Hellwig <hch@...radead.org>
Cc:     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 01:10:11AM -0700, Christoph Hellwig wrote:
> 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.

Groovy.

> > > > +
> > > > +	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.

Got it, thanks.

  Luis