lkml.org 
[lkml]   [2022]   [Oct]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH] blk-mq: fix null pointer dereference in blk_mq_clear_rq_mapping()
From
On 11/10/2022 13:10, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
>
> Our syzkaller report a null pointer dereference, root cause is
> following:
>
> __blk_mq_alloc_map_and_rqs
> set->tags[hctx_idx] = blk_mq_alloc_map_and_rqs
> blk_mq_alloc_map_and_rqs
> blk_mq_alloc_rqso

blk_mq_alloc_rqs

> // failed due to oom
> alloc_pages_node
> // set->stags[hctx_idx] is still NULL

set->tags

> blk_mq_free_rqs
> drv_tags = set->tags[hctx_idx];
> // null pointer dereference is triggered
> blk_mq_clear_rq_mapping(drv_tags, ...)
>
> This is because commit 63064be150e4 ("blk-mq:
> Add blk_mq_alloc_map_and_rqs()") merged the two steps:
>
> 1) set->tags[hctx_idx] = blk_mq_alloc_rq_map()
> 2) blk_mq_alloc_rqs(..., set->tags[hctx_idx])
>
> into one step:
>
> set->tags[hctx_idx] = blk_mq_alloc_map_and_rqs()
>
> Since tags is not initialized yet in this case, fix the problem by
> checking if tags is NULL pointer in blk_mq_clear_rq_mapping().
>
> Fixes: 63064be150e4 ("blk-mq: Add blk_mq_alloc_map_and_rqs()")
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>

We could have something like the following to ensure the drv tags are
assigned before we try to allocate the rqs (and fail), but prob not
worth the churn since it's not nice to pass &set->tags[hctx_idx]:

--->8---

-struct blk_mq_tags *blk_mq_alloc_map_and_rqs(struct blk_mq_tag_set *set,
+bool blk_mq_alloc_map_and_rqs(struct blk_mq_tag_set *set,
unsigned int hctx_idx,
- unsigned int depth)
+ unsigned int depth,
+ struct blk_mq_tags **tags)
{
- struct blk_mq_tags *tags;
int ret;

- tags = blk_mq_alloc_rq_map(set, hctx_idx, depth,
set->reserved_tags);
- if (!tags)
- return NULL;
+ *tags = blk_mq_alloc_rq_map(set, hctx_idx, depth,
set->reserved_tags);
+ if (!*tags)
+ return false;

- ret = blk_mq_alloc_rqs(set, tags, hctx_idx, depth);
+ ret = blk_mq_alloc_rqs(set, *tags, hctx_idx, depth);
if (ret) {
blk_mq_free_rq_map(tags);
+ *tags = NULL;
- return NULL;
+ return false;
}

- return tags;
+ return true;
}

static bool __blk_mq_alloc_map_and_rqs(struct blk_mq_tag_set *set,
@@ -3632,10 +3632,9 @@ static bool __blk_mq_alloc_map_and_rqs(struct
blk_mq_tag_set *set,
return true;
}

- set->tags[hctx_idx] = blk_mq_alloc_map_and_rqs(set, hctx_idx,
- set->queue_depth);
-
- return set->tags[hctx_idx];
+ return blk_mq_alloc_map_and_rqs(set, hctx_idx,
+ set->queue_depth,
+ &set->tags[hctx_idx]);
}

---8<----

So,

Reviewed-by: John Garry <john.garry@huawei.com>

> ---
> block/blk-mq.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 8070b6c10e8d..33292c01875d 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -3112,8 +3112,11 @@ static void blk_mq_clear_rq_mapping(struct blk_mq_tags *drv_tags,
> struct page *page;
> unsigned long flags;
>
> - /* There is no need to clear a driver tags own mapping */
> - if (drv_tags == tags)
> + /*
> + * There is no need to clear mapping if driver tags is not initialized
> + * or the mapping belongs to the driver tags.
> + */
> + if (!drv_tags || drv_tags == tags)
> return;
>
> list_for_each_entry(page, &tags->page_list, lru) {

\
 
 \ /
  Last update: 2022-10-11 15:25    [W:0.401 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site