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