Re: [PATCH RFC] blk-mq: fix potential uaf for 'queue_hw_ctx'

[Ming Lei <ming.lei@redhat.com>]

On Wed, Feb 23, 2022 at 07:26:01PM +0800, Yu Kuai wrote:
> blk_mq_realloc_hw_ctxs() will free the 'queue_hw_ctx'(e.g. undate
> submit_queues through configfs for null_blk), while it might still be
> used from other context(e.g. switch elevator to none):
> 
> t1					t2
> elevator_switch
>  blk_mq_unquiesce_queue
>   blk_mq_run_hw_queues
>    queue_for_each_hw_ctx
>     // assembly code for hctx = (q)->queue_hw_ctx[i]
>     mov    0x48(%rbp),%rdx -> read old queue_hw_ctx
> 
> 					__blk_mq_update_nr_hw_queues
> 					 blk_mq_realloc_hw_ctxs
> 					  hctxs = q->queue_hw_ctx
> 					  q->queue_hw_ctx = new_hctxs
> 					  kfree(hctxs)
>     movslq %ebx,%rax
>     mov    (%rdx,%rax,8),%rdi ->uaf
> 
> This problem was found by code review, and I comfirmed that the concurrent
> scenarios do exist(specifically 'q->queue_hw_ctx' can be changed during
> blk_mq_run_hw_queues), however, the uaf problem hasn't been repoduced yet
> without hacking the kernel.
> 
> Sicne the queue is freezed in __blk_mq_update_nr_hw_queues, fix the
> problem by protecting 'queue_hw_ctx' through rcu where it can be accessed
> without grabbing 'q_usage_counter'.
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>  block/blk-mq.c         |  8 +++++++-
>  include/linux/blk-mq.h |  2 +-
>  include/linux/blkdev.h | 13 ++++++++++++-
>  3 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 6c59ffe765fd..79367457d555 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -3955,7 +3955,13 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set,
>  		if (hctxs)
>  			memcpy(new_hctxs, hctxs, q->nr_hw_queues *
>  			       sizeof(*hctxs));
> -		q->queue_hw_ctx = new_hctxs;
> +
> +		rcu_assign_pointer(q->queue_hw_ctx, new_hctxs);
> +		/*
> +		 * Make sure reading the old queue_hw_ctx from other
> +		 * context concurrently won't trigger uaf.
> +		 */
> +		synchronize_rcu();
>  		kfree(hctxs);
>  		hctxs = new_hctxs;
>  	}
> diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
> index d319ffa59354..edcf8ead76c6 100644
> --- a/include/linux/blk-mq.h
> +++ b/include/linux/blk-mq.h
> @@ -918,7 +918,7 @@ static inline void *blk_mq_rq_to_pdu(struct request *rq)
>  
>  #define queue_for_each_hw_ctx(q, hctx, i)				\
>  	for ((i) = 0; (i) < (q)->nr_hw_queues &&			\
> -	     ({ hctx = (q)->queue_hw_ctx[i]; 1; }); (i)++)
> +	     ({ hctx = queue_hctx((q), i); 1; }); (i)++)
>  
>  #define hctx_for_each_ctx(hctx, ctx, i)					\
>  	for ((i) = 0; (i) < (hctx)->nr_ctx &&				\
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 3bfc75a2a450..2018a4dd2028 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -354,7 +354,7 @@ struct request_queue {
>  	unsigned int		queue_depth;
>  
>  	/* hw dispatch queues */
> -	struct blk_mq_hw_ctx	**queue_hw_ctx;
> +	struct blk_mq_hw_ctx __rcu	**queue_hw_ctx;
>  	unsigned int		nr_hw_queues;
>  
>  	/*
> @@ -622,6 +622,17 @@ static inline bool queue_is_mq(struct request_queue *q)
>  	return q->mq_ops;
>  }
>  
> +static inline struct blk_mq_hw_ctx *queue_hctx(struct request_queue *q, int id)
> +{
> +	struct blk_mq_hw_ctx *hctx;
> +
> +	rcu_read_lock();
> +	hctx = *(rcu_dereference(q->queue_hw_ctx) + id);
> +	rcu_read_unlock();
> +
> +	return hctx;
> +}

queue_hctx() should be moved into linux/blk-mq.h, otherwise feel free to
add:

Reviewed-by: Ming Lei <ming.lei@redhat.com>

Also it should be fine to implement queue_for_each_hw_ctx() as list, then we
can avoid the allocation for q->queue_hw_ctx without extra cost. I will work
toward that direction for improving the code.

Thanks,
Ming