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:   Sun, 19 Apr 2020 17:38:59 -0700
From:   Bart Van Assche <bvanassche@....org>
To:     Luis Chamberlain <mcgrof@...nel.org>
Cc:     axboe@...nel.dk, viro@...iv.linux.org.uk,
        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 4/19/20 5:04 PM, Luis Chamberlain wrote:
> On Sun, Apr 19, 2020 at 02:55:42PM -0700, Bart Van Assche wrote:
>> On 4/19/20 12:45 PM, Luis Chamberlain wrote:
>>> +int __must_check blk_queue_debugfs_register(struct request_queue *q)
>>> +{
>>> +	struct dentry *dir = NULL;
>>> +
>>> +	/* This can happen if we have a bug in the lower layers */
>>
>> What does "this" refer to? Which layers does "lower layers" refer to? Most
>> software developers consider a module that calls directly into another
>> module as a higher layer (callbacks through function pointers do not count;
>> see also https://en.wikipedia.org/wiki/Modular_programming). According to
>> that definition block drivers are a software layer immediately above the
>> block layer core.
>>
>> How about changing that comment into the following to make it unambiguous
>> (if this is what you meant)?
>>
>> 	/*
>> 	 * Check whether the debugfs directory already exists. This can
>> 	 * only happen as the result of a bug in a block driver.
>> 	 */
> 
> But I didn't mean on a block driver. I meant the block core. In our
> case, the async request_queue removal is an example. There is nothing
> that block drivers could have done to help much with that.
> 
> I could just change "lower layers" to "block layer" ?

That sounds good to me.

>> Independent of what the purpose of the above code is, can that code be
>> rewritten such that it does not depend on the details of how names are
>> assigned to disks and partitions? Would disk_get_part() be useful here?
> 
> I did try, but couldn't figure out a way. I'll keep looking but likewise
> let me know if you find a way.

How about making blk_trace_setup() pass the result of the following 
expression as an additional argument to blk_trace_setup():

	bdev != bdev->bd_contains

I think that is a widely used approach to verify after a block device 
has been opened whether or not 'bdev' refers to a partition (bdev != 
bdev->bd_contains means that 'bdev' represents a partition).

Thanks,

Bart.