linux-kernel - Re: [GIT PULL] Block pull request for- 4.11-rc1

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0dc039e5-58c5-d328-0b81-bf9e7d782675@kernel.dk>
Date:   Fri, 24 Feb 2017 13:22:42 -0700
From:   Jens Axboe <axboe@...nel.dk>
To:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Bart Van Assche <Bart.VanAssche@...disk.com>
Cc:     "hch@....de" <hch@....de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        "snitzer@...hat.com" <snitzer@...hat.com>
Subject: Re: [GIT PULL] Block pull request for- 4.11-rc1

On 02/24/2017 01:00 PM, Jens Axboe wrote:
> On 02/24/2017 12:43 PM, Linus Torvalds wrote:
>> On Fri, Feb 24, 2017 at 9:39 AM, Bart Van Assche
>> <Bart.VanAssche@...disk.com> wrote:
>>>
>>> So the crash is caused by an attempt to dereference address 0x6b6b6b6b6b6b6b6b
>>> at offset 0x270. I think this means the crash is caused by a use-after-free.
>>
>> Yeah, that's POISON_FREE, and that might explain why you see crashes
>> that others don't - you obviously have SLAB poisoning enabled. Jens
>> may not have.
>>
>> %rdi is "struct mapped_device *md", which came from dm_softirq_done() doing
>>
>>         struct dm_rq_target_io *tio = tio_from_request(rq);
>>         struct request *clone = tio->clone;
>>         int rw;
>>
>>         if (!clone) {
>>                 rq_end_stats(tio->md, rq);
>>                 rw = rq_data_dir(rq);
>>                 if (!rq->q->mq_ops)
>>                         blk_end_request_all(rq, tio->error);
>>                 else
>>                         blk_mq_end_request(rq, tio->error);
>>                 rq_completed(tio->md, rw, false);
>>                 return;
>>         }
>>
>> so it's the 'tio' pointer that has been free'd. But it's worth noting
>> that we did apparently successfully dereference "tio" earlier in that
>> dm_softirq_done() *without* getting the poison value, so what I think
>> might be going on is that the 'tio' thing gets free'd when the code
>> does the blk_end_request_all()/blk_mq_end_request() call.
>>
>> Which makes sense - that ends the lifetime of the request, which in
>> turn also ends the lifetime of the "tio_from_request()", no?
>>
>> So the fix may be as simple as just doing
>>
>>         if (!clone) {
>>                 struct mapped_device *md = tio->md;
>>
>>                 rq_end_stats(md, rq);
>>                 ...
>>                 rq_completed(md, rw, false);
>>                 return;
>>         }
>>
>> because the 'mapped_device' pointer hopefully is still valid, it's
>> just 'tio' that has been freed.
>>
>> Jens? Bart? Christoph? Somebody who knows this code should
>> double-check my thinking above. I don't actually know the tio
>> lifetimes, I'm just going by looking at how earlier accesses seemed to
>> be fine (eg that "tio->clone" got us NULL, not a POISON_FREE pointer,
>> for example).
> 
> I think that is spot on. With the request changes for CDBs, for non
> blk-mq, we know also carry the payload after the request. But since
> blk-mq never frees the request, the above use-after-free with poison
> will only happen for !mq. Caching 'md' and avoiding a dereference of
> 'tio' after calling blk_end_request_all() will likely fix it.
> 
> Bart, can you test that?

Bart, I pushed a fix here:

http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=61febef40bfe8ab68259d8545257686e8a0d91d1

-- 
Jens Axboe