Re: [PATCH] blk-mq: fix null pointer dereference in blk_mq_clear_rq_mapping()

[John Garry]

On 11/10/2022 13:10, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>

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@xxxxxxxxxx>

> ---
>   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) {