linux-kernel - Re: [PATCH v6 6/6] 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: <20200609175359.GR11244@42.do-not-panic.com>
Date:   Tue, 9 Jun 2020 17:53:59 +0000
From:   Luis Chamberlain <mcgrof@...nel.org>
To:     Christoph Hellwig <hch@...radead.org>
Cc:     Jan Kara <jack@...e.cz>, axboe@...nel.dk, viro@...iv.linux.org.uk,
        bvanassche@....org, gregkh@...uxfoundation.org,
        rostedt@...dmis.org, mingo@...hat.com, ming.lei@...hat.com,
        nstange@...e.de, akpm@...ux-foundation.org, mhocko@...e.com,
        yukuai3@...wei.com, martin.petersen@...cle.com, jejb@...ux.ibm.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 v6 6/6] blktrace: fix debugfs use after free

On Tue, Jun 09, 2020 at 10:32:18AM -0700, Christoph Hellwig wrote:
> On Tue, Jun 09, 2020 at 05:29:22PM +0000, Luis Chamberlain wrote:
> > Is scsi-generic is the only unwanted ugly child blktrace has to deal
> > with? For some reason I thought drivers/md/md.c was one but it seems
> > like it is not. Do we have an easy way to search for these? I think
> > this would just affect how we express the comment only.
> 
> grep for blk_trace_setup.  For all blk devices that setup comes in
> through the block device ioctl path, and that relies on having a
> struct block_device and queue.  sg on the other hand calls
> blk_trace_setup directly with a NULL bdev argument.

Alright, then we should be good.

> > >  		 */
> > > -		dir = q->sg_debugfs_dir;
> > > +		dir = debugfs_create_dir(buts->name, blk_debugfs_root);
> > > +		bt->dir = dir;
> > 
> > The other chicken and egg problem to consider at least in the comments
> > is that the debugfs directory for these types of devices *have* an
> > exposed path, but the data structure is rather opaque to the device and
> > even blktrace.  Fortunately given the recent set of changes around the
> > q->blk_trace and clarifications around its use we have made it clear now
> > that so long as hold the q->blk_trace_mutex *and* check q->blk_trace we
> > *should* not race against two separate creations of debugfs directories,
> > so I think this is safe, so long as these indpendent drivers don't end
> > up re-using the same path for some other things later in the future, and
> > since we have control over what goes under debugfsroot block / I think
> > we should be good.
> > 
> > But I think that the concern for race on names may still be worth
> > explaining a bit here.
> 
> Feel free to add more comments, but please try to keep them short
> and crisp.  At the some point long comments really distract from what
> is going on.

Sure.

Come to think of it, given the above, I think we can also do way with
the the partition stuff too, and rely on the buts->name too. I'll try
this out, and test it.

  Luis