| Message ID | CAGXu5jKVPQi0cdTbe5Y1DNHdiJ6-kEFRBTNv7znuqT9w5eifGg@mail.gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 4/17/18 3:25 PM, Kees Cook wrote: > On Tue, Apr 17, 2018 at 1:46 PM, Kees Cook <keescook@chromium.org> wrote: >> I see elv.priv[1] assignments made in a few places -- is it possible >> there is some kind of uninitialized-but-not-NULL state that can leak >> in there? > > Got it. This fixes it for me: > > 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)); > } > } This looks like a BFQ problem, this should not be necessary. Paolo, you're calling your own prepare request handler from the insert as well, and your prepare request does nothing if rq->elv.icq == NULL.
On Tue, Apr 17, 2018 at 2:39 PM, Jens Axboe <axboe@kernel.dk> wrote: > On 4/17/18 3:25 PM, Kees Cook wrote: >> On Tue, Apr 17, 2018 at 1:46 PM, Kees Cook <keescook@chromium.org> wrote: >>> I see elv.priv[1] assignments made in a few places -- is it possible >>> there is some kind of uninitialized-but-not-NULL state that can leak >>> in there? >> >> Got it. This fixes it for me: >> >> 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)); >> } >> } > > This looks like a BFQ problem, this should not be necessary. Paolo, > you're calling your own prepare request handler from the insert > as well, and your prepare request does nothing if rq->elv.icq == NULL. I sent the patch anyway, since it's kind of a robustness improvement, I'd hope. If you fix BFQ also, please add: Reported-by: Oleksandr Natalenko <oleksandr@natalenko.name> Root-caused-by: Kees Cook <keescook@chromium.org> :) I gotta task-switch to other things! Thanks for the pointers, and thank you Oleksandr for providing the reproducer! -Kees
On 4/17/18 3:47 PM, Kees Cook wrote: > On Tue, Apr 17, 2018 at 2:39 PM, Jens Axboe <axboe@kernel.dk> wrote: >> On 4/17/18 3:25 PM, Kees Cook wrote: >>> On Tue, Apr 17, 2018 at 1:46 PM, Kees Cook <keescook@chromium.org> wrote: >>>> I see elv.priv[1] assignments made in a few places -- is it possible >>>> there is some kind of uninitialized-but-not-NULL state that can leak >>>> in there? >>> >>> Got it. This fixes it for me: >>> >>> 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)); >>> } >>> } >> >> This looks like a BFQ problem, this should not be necessary. Paolo, >> you're calling your own prepare request handler from the insert >> as well, and your prepare request does nothing if rq->elv.icq == NULL. > > I sent the patch anyway, since it's kind of a robustness improvement, > I'd hope. If you fix BFQ also, please add: It's also a memset() in the hot path, would prefer to avoid that... The issue here is really the convoluted bfq usage of insert/prepare, I'm sure Paolo can take it from here.
Hi. 17.04.2018 23:47, Kees Cook wrote: > I sent the patch anyway, since it's kind of a robustness improvement, > I'd hope. If you fix BFQ also, please add: > > Reported-by: Oleksandr Natalenko <oleksandr@natalenko.name> > Root-caused-by: Kees Cook <keescook@chromium.org> > > :) I gotta task-switch to other things! > > Thanks for the pointers, and thank you Oleksandr for providing the > reproducer! That was a great fun to read. Thank you for digging into it. Regards, Oleksandr
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))