linux-kernel - Re: NULL deref around blkmq in v4.0-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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <5526EE36.7050303@kernel.dk>
Date:	Thu, 09 Apr 2015 15:25:10 -0600
From:	Jens Axboe <axboe@...nel.dk>
To:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Jan Engelhardt <jengelh@...i.de>
CC:	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: NULL deref around blkmq in v4.0-rc1–rc7

On 04/09/2015 03:12 PM, Linus Torvalds wrote:
> On Thu, Apr 9, 2015 at 11:24 AM, Jan Engelhardt <jengelh@...i.de> wrote:
>>
>> It's fairly consistent (reproducible?). Only 1 in 15 or so (have not kept track
>> really) attempts does it not die.
>>
>> With frame pointers:
>>   [<ffffffff81286d59>] scsi_queue_rq+0x2e8/0x3d2
>>   [<ffffffff8119e64d>] __blk_mq_run_hw_queue+0x19b/0x2a2
>>   [<ffffffff8119e901>] ? blk_mq_merge_queue_io+0x75/0x147
>>   [<ffffffffa00fa34a>] ? __xfs_get_blocks+0x2f9/0x2f9 [xfs]
>>   [<ffffffff8119edeb>] blk_mq_run_hw_queue+0x4f/0x99
>>   [<ffffffff8119fab9>] blk_sq_make_request+0x163/0x170
>
> Ok, good.
>
> So the cmd comes from
>
>          struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
>
> which in turn is just
>
>          return (void *) rq + sizeof(*rq);
>
> which in turn is written by some crazy monkey on crack. That's some
> shit code. Why the hell you'd write it that way, when the natural
> thing to do would be just
>
>          return rq + 1;
>
> without the sizeof, and without the cast.
>
> The particular crazy monkey on crack is Jens Axboe, in commit 320ae51feed5c.
>
> Jens, really. This code is shit.

That's a bit rough on a single line of code like that, don't you think? 
But yes, rq + 1 is identical and cleaner...

> That ->sense_buffer thing is supposed to be initialized by the
> blk_mq_ops.init_request() function, which is called - if it exists =
> when the array of requests ('->rqs[]') is initialized.
>
> And that code too looks like crap. It seems to be very clever, trying
> to allocaet big contiguous chunks of RAM for the requests, but then
> the initialization sequence is questionable as hell. It takes that
> nonzeroed allocation, and zeroes a few fields randomly. The rest will
> contain whatever garbage data they used to.
>
> Does this entirely untested patch make any difference?
>
> And Jens, this all really looks very fishy. When I look at these kinds
> of core functions, and find just *stupid* code like this, it makes me
> unhappy.

Not sure why it isn't all zeroed, definitely the saner thing to do at 
init time. So patch looks fine, should be applied regardless of whether 
or not it fixes this issue.

But it's _always_ been like that, so it's not a change in behavior. It 
is fragile, so perhaps some SCSI change modified alloc behavior and it's 
not causing and issue.

And if this is mpt, we recently ran into some list corruption issues due 
to a bug in the driver. It hit on reboot, but it was scan related, so 
could be a boot issue as well.

-- 
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/