Re: [RFC] block: fix access of uninitialized pointer address in bt_for_each()

[Bart Van Assche]

On 2020-04-17 05:51, yu kuai wrote:
> I recently got a KASAN warning like this in our 4.19 kernel:
> 
>  ==================================================================
>  BUG: KASAN: slab-out-of-bounds in bt_for_each+0x1dc/0x2c0
>  Read of size 8 at addr ffff8000c0865000 by task sh/2023305
> 
>  Call trace:
>  dump_backtrace+0x0/0x310
>  show_stack+0x28/0x38
>  dump_stack+0xd8/0x108
>  print_address_description+0x68/0x2d0
>  kasan_report+0x124/0x2e0
>  __asan_load8+0x88/0xb0
>  bt_for_each+0x1dc/0x2c0
>  blk_mq_queue_tag_busy_iter+0x1f0/0x3e8
>  blk_mq_in_flight+0xb4/0xe0
>  part_in_flight+0x124/0x178
>  part_round_stats+0x128/0x3b0
>  blk_account_io_start+0x2b4/0x3f0
>  blk_mq_bio_to_request+0x170/0x258
>  blk_mq_make_request+0x734/0xdd8
>  generic_make_request+0x388/0x740
>  submit_bio+0xd8/0x3d0
>  ext4_io_submit+0xb4/0xe0 [ext4]
>  ext4_writepages+0xb44/0x1c00 [ext4]
>  do_writepages+0xc8/0x1f8
>  __filemap_fdatawrite_range+0x200/0x2a0
>  filemap_flush+0x30/0x40
>  ext4_alloc_da_blocks+0x54/0x200 [ext4]
>  ext4_release_file+0xfc/0x150 [ext4]
>  __fput+0x15c/0x3a8
>  ____fput+0x24/0x30
>  task_work_run+0x1a4/0x208
>  do_notify_resume+0x1a8/0x1c0
>  work_pending+0x8/0x10
> 
>  Allocated by task 3515778:
>  kasan_kmalloc+0xe0/0x190
>  kmem_cache_alloc_trace+0x18c/0x418
>  alloc_pipe_info+0x74/0x240
>  create_pipe_files+0x74/0x2f8
>  __do_pipe_flags+0x48/0x168
>  do_pipe2+0xa0/0x1d0
>  __arm64_sys_pipe2+0x3c/0x50
>  el0_svc_common+0xb4/0x1d8
>  el0_svc_handler+0x50/0xa8
>  el0_svc+0x8/0xc
> 
>  Freed by task 3515778:
>  __kasan_slab_free+0x120/0x228
>  kasan_slab_free+0x10/0x18
>  kfree+0x88/0x3d8
>  free_pipe_info+0x150/0x178
>  put_pipe_info+0x138/0x1c0
>  pipe_release+0xe8/0x120
>  __fput+0x15c/0x3a8
>  ____fput+0x24/0x30
>  task_work_run+0x1a4/0x208
>  do_notify_resume+0x1a8/0x1c0
>  work_pending+0x8/0x10

The alloc/free info refers to a data structure owned by the pipe
implementation. The use-after-free report refers to a data structure
owned by the block layer. How can that report make sense?

> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 7ed16ed13976..48b74d0085c7 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -485,6 +485,7 @@ static void __blk_mq_free_request(struct request *rq)
>  	struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(q, ctx->cpu);
>  	const int sched_tag = rq->internal_tag;
>  
> +	hctx->tags->rqs[rq->tag] = NULL;
>  	if (rq->tag != -1)
>  		blk_mq_put_tag(hctx, hctx->tags, ctx, rq->tag);
>  	if (sched_tag != -1)

Can the above change trigger the following assignment?

hctx->tags->rqs[-1] = NULL?

> @@ -1999,7 +2000,7 @@ struct blk_mq_tags *blk_mq_alloc_rq_map(struct blk_mq_tag_set *set,
>  	if (!tags)
>  		return NULL;
>  
> -	tags->rqs = kcalloc_node(nr_tags, sizeof(struct request *),
> +	tags->rqs = kzalloc_node(nr_tags, sizeof(struct request *),
>  				 GFP_NOIO | __GFP_NOWARN | __GFP_NORETRY,
>  				 node);

>From include/linux/slab.h:

static inline void *kcalloc_node(size_t n, size_t size, gfp_t flags,
                                 int node)
{
	return kmalloc_array_node(n, size, flags | __GFP_ZERO, node);
}

I think this means that kcalloc_node() already zeroes the allocated
memory and hence that changing kcalloc() into kzalloc() is not necessary.

>  	if (!tags->rqs) {
> diff --git a/block/blk-mq.h b/block/blk-mq.h
> index a6094c27b827..2a55292d3d51 100644
> --- a/block/blk-mq.h
> +++ b/block/blk-mq.h
> @@ -196,6 +196,7 @@ static inline void blk_mq_put_driver_tag_hctx(struct blk_mq_hw_ctx *hctx,
>  	if (rq->tag == -1 || rq->internal_tag == -1)
>  		return;
>  
> +	hctx->tags->rqs[rq->tag] = NULL;
>  	__blk_mq_put_driver_tag(hctx, rq);
>  }
>  
> @@ -207,6 +208,7 @@ static inline void blk_mq_put_driver_tag(struct request *rq)
>  		return;
>  
>  	hctx = blk_mq_map_queue(rq->q, rq->mq_ctx->cpu);
> +	hctx->tags->rqs[rq->tag] = NULL;
>  	__blk_mq_put_driver_tag(hctx, rq);
>  }

I don't think the above changes are sufficient to fix the
use-after-free. Has it been considered to free the memory that backs
tags->bitmap_tags only after an RCU grace period has expired? See also
blk_mq_free_tags().

Thanks,

Bart.