On Tue, May 04, 2021 at 11:15:37AM +0100, John Garry wrote:
> On 29/04/2021 03:34, Ming Lei wrote:
> > Hi Jens,
> >
> > This patchset fixes the request UAF issue by one simple approach,
> > without clearing ->rqs[] in fast path.
> >
> > 1) grab request's ref before calling ->fn in blk_mq_tagset_busy_iter,
> > and release it after calling ->fn, so ->fn won't be called for one
> > request if its queue is frozen, done in 2st patch
> >
> > 2) clearing any stale request referred in ->rqs[] before freeing the
> > request pool, one per-tags spinlock is added for protecting
> > grabbing request ref vs. clearing ->rqs[tag], so UAF by refcount_inc_not_zero
> > in bt_tags_iter() is avoided, done in 3rd patch.
> >
>
> I had a go at testing this. Without any modifications for testing, it looks
> ok.
>
> However I also tested by adding an artificial delay in bt_iter() - otherwise
> it may not be realistic to trigger some UAF issues in sane timeframes.
>
> So I made this change:
>
> --- a/block/blk-mq-tag.c
> +++ b/block/blk-mq-tag.c
> @@ -215,8 +215,11 @@ static bool bt_iter(struct sbitmap *bitmap, unsigned
> int bitnr, void *data)
> * We can hit rq == NULL here, because the tagging functions
> * test and set the bit before assigning ->rqs[].
> */
> - if (rq && rq->q == hctx->queue && rq->mq_hctx == hctx)
> - return iter_data->fn(hctx, rq, iter_data->data, reserved);
> + if (rq) {
> + mdelay(50);
> + if (rq->q == hctx->queue && rq->mq_hctx == hctx)
> + return iter_data->fn(hctx, rq, iter_data->data, reserved);
> + }
> return true;
> }
hammmm, forget to cover bt_iter(), please test the following delta
patch:
diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
index a3be267212b9..27815114ee3f 100644
--- a/block/blk-mq-tag.c
+++ b/block/blk-mq-tag.c
@@ -206,18 +206,28 @@ static bool bt_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
struct blk_mq_tags *tags = hctx->tags;
bool reserved = iter_data->reserved;
struct request *rq;
+ unsigned long flags;
+ bool ret = true;
if (!reserved)
bitnr += tags->nr_reserved_tags;
- rq = tags->rqs[bitnr];
+ spin_lock_irqsave(&tags->lock, flags);
+ rq = tags->rqs[bitnr];
/*
* We can hit rq == NULL here, because the tagging functions
* test and set the bit before assigning ->rqs[].
*/
- if (rq && rq->q == hctx->queue && rq->mq_hctx == hctx)
- return iter_data->fn(hctx, rq, iter_data->data, reserved);
- return true;
+ if (!rq || !refcount_inc_not_zero(&rq->ref)) {
+ spin_unlock_irqrestore(&tags->lock, flags);
+ return true;
+ }
+ spin_unlock_irqrestore(&tags->lock, flags);
+
+ if (rq->q == hctx->queue && rq->mq_hctx == hctx)
+ ret = iter_data->fn(hctx, rq, iter_data->data, reserved);
+ blk_mq_put_rq_ref(rq);
+ return ret;
}
/**
Thanks,
Ming
