LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] blk-mq: Clear out elevator private data
@ 2018-04-17 21:42 Kees Cook
  2018-04-17 21:45 ` Jens Axboe
  2018-04-18  8:47 ` Paolo Valente
  0 siblings, 2 replies; 5+ messages in thread
From: Kees Cook @ 2018-04-17 21:42 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Oleksandr Natalenko, linux-kernel, Paolo Valente,
	Bart Van Assche, David Windsor, James E.J. Bottomley,
	Martin K. Petersen, linux-scsi, Christoph Hellwig,
	Hannes Reinecke, Johannes Thumshirn, linux-block

Some elevators may not correctly check rq->rq_flags & RQF_ELVPRIV, and
may attempt to read rq->elv fields. When requests got reused, this
caused BFQ to think it already had a bfqq (rq->elv.priv[1]) allocated.
This could lead to odd behaviors like having the sense buffer address
slowly start incrementing. This eventually tripped HARDENED_USERCOPY
and KASAN.

This patch wipes all of rq->elv instead of just rq->elv.icq. While
it shouldn't technically be needed, this ends up being a robustness
improvement that should lead to at least finding bugs in elevators faster.

Reported-by: Oleksandr Natalenko <oleksandr@natalenko.name>
Fixes: bd166ef183c26 ("blk-mq-sched: add framework for MQ capable IO schedulers")
Cc: stable@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
In theory, BFQ needs to also check the RQF_ELVPRIV flag, but I'll leave that
to Paolo to figure out. Also, my Fixes line is kind of a best-guess. This
is where icq was originally wiped, so it seemed as good a commit as any.
---
 block/blk-mq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 0dc9e341c2a7..859df3160303 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -363,7 +363,7 @@ static struct request *blk_mq_get_request(struct request_queue *q,
 
 	rq = blk_mq_rq_ctx_init(data, tag, op);
 	if (!op_is_flush(op)) {
-		rq->elv.icq = NULL;
+		memset(&rq->elv, 0, sizeof(rq->elv));
 		if (e && e->type->ops.mq.prepare_request) {
 			if (e->type->icq_cache && rq_ioc(bio))
 				blk_mq_sched_assign_ioc(rq, bio);
@@ -461,7 +461,7 @@ void blk_mq_free_request(struct request *rq)
 			e->type->ops.mq.finish_request(rq);
 		if (rq->elv.icq) {
 			put_io_context(rq->elv.icq->ioc);
-			rq->elv.icq = NULL;
+			memset(&rq->elv, 0, sizeof(rq->elv));
 		}
 	}
 
-- 
2.7.4


-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] blk-mq: Clear out elevator private data
  2018-04-17 21:42 [PATCH] blk-mq: Clear out elevator private data Kees Cook
@ 2018-04-17 21:45 ` Jens Axboe
  2018-04-17 22:57   ` Kees Cook
  2018-04-18  8:47 ` Paolo Valente
  1 sibling, 1 reply; 5+ messages in thread
From: Jens Axboe @ 2018-04-17 21:45 UTC (permalink / raw)
  To: Kees Cook
  Cc: Oleksandr Natalenko, linux-kernel, Paolo Valente,
	Bart Van Assche, David Windsor, James E.J. Bottomley,
	Martin K. Petersen, linux-scsi, Christoph Hellwig,
	Hannes Reinecke, Johannes Thumshirn, linux-block

On 4/17/18 3:42 PM, Kees Cook wrote:
> Some elevators may not correctly check rq->rq_flags & RQF_ELVPRIV, and
> may attempt to read rq->elv fields. When requests got reused, this
> caused BFQ to think it already had a bfqq (rq->elv.priv[1]) allocated.
> This could lead to odd behaviors like having the sense buffer address
> slowly start incrementing. This eventually tripped HARDENED_USERCOPY
> and KASAN.
> 
> This patch wipes all of rq->elv instead of just rq->elv.icq. While
> it shouldn't technically be needed, this ends up being a robustness
> improvement that should lead to at least finding bugs in elevators faster.

Comments from the other email still apply, we should not need to do this
full memset() for every request. From a quick look, BFQ needs to straighten
out its usage of prepare request and interactions with insert_request.

> Reported-by: Oleksandr Natalenko <oleksandr@natalenko.name>
> Fixes: bd166ef183c26 ("blk-mq-sched: add framework for MQ capable IO schedulers")
> Cc: stable@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> In theory, BFQ needs to also check the RQF_ELVPRIV flag, but I'll leave that
> to Paolo to figure out. Also, my Fixes line is kind of a best-guess. This
> is where icq was originally wiped, so it seemed as good a commit as any.

Yeah, that's probably a bit too broad for fixes :-)

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] blk-mq: Clear out elevator private data
  2018-04-17 21:45 ` Jens Axboe
@ 2018-04-17 22:57   ` Kees Cook
  2018-04-17 23:00     ` Jens Axboe
  0 siblings, 1 reply; 5+ messages in thread
From: Kees Cook @ 2018-04-17 22:57 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Oleksandr Natalenko, LKML, Paolo Valente, Bart Van Assche,
	David Windsor, James E.J. Bottomley, Martin K. Petersen,
	linux-scsi, Christoph Hellwig, Hannes Reinecke,
	Johannes Thumshirn, linux-block

On Tue, Apr 17, 2018 at 2:45 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 4/17/18 3:42 PM, Kees Cook wrote:
>> Some elevators may not correctly check rq->rq_flags & RQF_ELVPRIV, and
>> may attempt to read rq->elv fields. When requests got reused, this
>> caused BFQ to think it already had a bfqq (rq->elv.priv[1]) allocated.
>> This could lead to odd behaviors like having the sense buffer address
>> slowly start incrementing. This eventually tripped HARDENED_USERCOPY
>> and KASAN.
>>
>> This patch wipes all of rq->elv instead of just rq->elv.icq. While
>> it shouldn't technically be needed, this ends up being a robustness
>> improvement that should lead to at least finding bugs in elevators faster.
>
> Comments from the other email still apply, we should not need to do this
> full memset() for every request. From a quick look, BFQ needs to straighten
> out its usage of prepare request and interactions with insert_request.

Sure, understood. I would point out, FWIW, that memset() gets unrolled
by the compiler and this is just two more XORs in the same cacheline
(the two words following icq). (And there is SO much more being
cleared during alloc, it didn't seem like hardly any extra cost vs the
robustness it provided.)

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] blk-mq: Clear out elevator private data
  2018-04-17 22:57   ` Kees Cook
@ 2018-04-17 23:00     ` Jens Axboe
  0 siblings, 0 replies; 5+ messages in thread
From: Jens Axboe @ 2018-04-17 23:00 UTC (permalink / raw)
  To: Kees Cook
  Cc: Oleksandr Natalenko, LKML, Paolo Valente, Bart Van Assche,
	David Windsor, James E.J. Bottomley, Martin K. Petersen,
	linux-scsi, Christoph Hellwig, Hannes Reinecke,
	Johannes Thumshirn, linux-block

On 4/17/18 4:57 PM, Kees Cook wrote:
> On Tue, Apr 17, 2018 at 2:45 PM, Jens Axboe <axboe@kernel.dk> wrote:
>> On 4/17/18 3:42 PM, Kees Cook wrote:
>>> Some elevators may not correctly check rq->rq_flags & RQF_ELVPRIV, and
>>> may attempt to read rq->elv fields. When requests got reused, this
>>> caused BFQ to think it already had a bfqq (rq->elv.priv[1]) allocated.
>>> This could lead to odd behaviors like having the sense buffer address
>>> slowly start incrementing. This eventually tripped HARDENED_USERCOPY
>>> and KASAN.
>>>
>>> This patch wipes all of rq->elv instead of just rq->elv.icq. While
>>> it shouldn't technically be needed, this ends up being a robustness
>>> improvement that should lead to at least finding bugs in elevators faster.
>>
>> Comments from the other email still apply, we should not need to do this
>> full memset() for every request. From a quick look, BFQ needs to straighten
>> out its usage of prepare request and interactions with insert_request.
> 
> Sure, understood. I would point out, FWIW, that memset() gets unrolled
> by the compiler and this is just two more XORs in the same cacheline
> (the two words following icq). (And there is SO much more being
> cleared during alloc, it didn't seem like hardly any extra cost vs the
> robustness it provided.)

Yeah, it's not super pricey, but it's not needed. BFQ is the user of
the members, and the one that assigns them. You're saying leftover
assignments, since it doesn't always assign them. Hence I think that's
a better fix, just sent out a test patch a few minutes ago.

You did all the hard work, I'm just coasting on your findings.

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] blk-mq: Clear out elevator private data
  2018-04-17 21:42 [PATCH] blk-mq: Clear out elevator private data Kees Cook
  2018-04-17 21:45 ` Jens Axboe
@ 2018-04-18  8:47 ` Paolo Valente
  1 sibling, 0 replies; 5+ messages in thread
From: Paolo Valente @ 2018-04-18  8:47 UTC (permalink / raw)
  To: Kees Cook
  Cc: Jens Axboe, Oleksandr Natalenko, Linux Kernel Mailing List,
	Bart Van Assche, David Windsor, James E.J. Bottomley,
	Martin K. Petersen, linux-scsi, Christoph Hellwig,
	Hannes Reinecke, Johannes Thumshirn, linux-block



> Il giorno 17 apr 2018, alle ore 23:42, Kees Cook <keescook@chromium.org> ha scritto:
> 
> Some elevators may not correctly check rq->rq_flags & RQF_ELVPRIV, and
> may attempt to read rq->elv fields. When requests got reused, this
> caused BFQ to think it already had a bfqq (rq->elv.priv[1]) allocated.

Hi Kees,
where does BFQ gets confused and operates on a request not destined to
it?  I'm asking because I paid attention to always avoid such a
mistake.

Thanks,
Paolo

> This could lead to odd behaviors like having the sense buffer address
> slowly start incrementing. This eventually tripped HARDENED_USERCOPY
> and KASAN.
> 
> This patch wipes all of rq->elv instead of just rq->elv.icq. While
> it shouldn't technically be needed, this ends up being a robustness
> improvement that should lead to at least finding bugs in elevators faster.
> 
> Reported-by: Oleksandr Natalenko <oleksandr@natalenko.name>
> Fixes: bd166ef183c26 ("blk-mq-sched: add framework for MQ capable IO schedulers")
> Cc: stable@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> In theory, BFQ needs to also check the RQF_ELVPRIV flag, but I'll leave that
> to Paolo to figure out. Also, my Fixes line is kind of a best-guess. This
> is where icq was originally wiped, so it seemed as good a commit as any.
> ---
> block/blk-mq.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 0dc9e341c2a7..859df3160303 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -363,7 +363,7 @@ static struct request *blk_mq_get_request(struct request_queue *q,
> 
> 	rq = blk_mq_rq_ctx_init(data, tag, op);
> 	if (!op_is_flush(op)) {
> -		rq->elv.icq = NULL;
> +		memset(&rq->elv, 0, sizeof(rq->elv));
> 		if (e && e->type->ops.mq.prepare_request) {
> 			if (e->type->icq_cache && rq_ioc(bio))
> 				blk_mq_sched_assign_ioc(rq, bio);
> @@ -461,7 +461,7 @@ void blk_mq_free_request(struct request *rq)
> 			e->type->ops.mq.finish_request(rq);
> 		if (rq->elv.icq) {
> 			put_io_context(rq->elv.icq->ioc);
> -			rq->elv.icq = NULL;
> +			memset(&rq->elv, 0, sizeof(rq->elv));
> 		}
> 	}
> 
> -- 
> 2.7.4
> 
> 
> -- 
> Kees Cook
> Pixel Security

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2018-04-18  8:47 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-17 21:42 [PATCH] blk-mq: Clear out elevator private data Kees Cook
2018-04-17 21:45 ` Jens Axboe
2018-04-17 22:57   ` Kees Cook
2018-04-17 23:00     ` Jens Axboe
2018-04-18  8:47 ` Paolo Valente

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).