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]

Date:	Thu, 9 Apr 2015 14:12:22 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Jan Engelhardt <jengelh@...i.de>
Cc:	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
	Jens Axboe <axboe@...nel.dk>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: NULL deref around blkmq in v4.0-rc1–rc7

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

Anyway, I assume that the actual "disk" in question is mpt fusion?

                             Linus

View attachment "patch.diff" of type "text/plain" (870 bytes)