linux-kernel - Re: [PATCH] blk-mq: fix null pointer dereference in blk_mq_clear_rq

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Tue, 11 Oct 2022 21:27:34 +0800
From:   Yu Kuai <yukuai1@...weicloud.com>
To:     John Garry <john.garry@...wei.com>,
        Yu Kuai <yukuai1@...weicloud.com>, axboe@...nel.dk,
        ming.lei@...hat.com, hare@...e.de
Cc:     linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
        yi.zhang@...wei.com, "yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH] blk-mq: fix null pointer dereference in
 blk_mq_clear_rq_mapping()

Hi,

在 2022/10/11 21:24, John Garry 写道:
> On 11/10/2022 13:10, Yu Kuai wrote:
>> From: Yu Kuai <yukuai3@...wei.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@...wei.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@...wei.com>

Thanks for the review, I'll send a new verion with the spelling mistakes
fixed.

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