From 734c36bc544674ccb740829728b0835ad108a050 Mon Sep 17 00:00:00 2001 From: Todd Kjos Date: Fri, 27 Aug 2021 11:17:37 -0700 Subject: [PATCH 1/5] ANDROID: gki_defconfig: set DEFAULT_MMAP_MIN_ADDR=32768 Set CONFIG_DEFAULT_MMAP_MIN_ADDR=32768 to conform with existing CTS test (which had an issue that prevented reliable detection of this setting). Bug: 197914473 Signed-off-by: Todd Kjos Change-Id: I5a86a7b11d32adf8689657e4559c1d870a5562c4 --- arch/arm64/configs/gki_defconfig | 1 + arch/x86/configs/gki_defconfig | 1 + 2 files changed, 2 insertions(+) diff --git a/arch/arm64/configs/gki_defconfig b/arch/arm64/configs/gki_defconfig index d72ec6f4ee8d..48c5cf94a270 100644 --- a/arch/arm64/configs/gki_defconfig +++ b/arch/arm64/configs/gki_defconfig @@ -99,6 +99,7 @@ CONFIG_GKI_OPT_FEATURES=y # CONFIG_CORE_DUMP_DEFAULT_ELF_HEADERS is not set CONFIG_BINFMT_MISC=y CONFIG_MEMORY_HOTPLUG=y +CONFIG_DEFAULT_MMAP_MIN_ADDR=32768 CONFIG_CLEANCACHE=y CONFIG_CMA=y CONFIG_CMA_AREAS=16 diff --git a/arch/x86/configs/gki_defconfig b/arch/x86/configs/gki_defconfig index 869128096c38..2c9a5bdcac60 100644 --- a/arch/x86/configs/gki_defconfig +++ b/arch/x86/configs/gki_defconfig @@ -77,6 +77,7 @@ CONFIG_GKI_HACKS_TO_FIX=y CONFIG_GKI_OPT_FEATURES=y # CONFIG_CORE_DUMP_DEFAULT_ELF_HEADERS is not set CONFIG_BINFMT_MISC=y +CONFIG_DEFAULT_MMAP_MIN_ADDR=32768 CONFIG_CLEANCACHE=y CONFIG_CMA=y CONFIG_CMA_AREAS=16 From a5d38e7c26ca21a544635732a711176017663168 Mon Sep 17 00:00:00 2001 From: Ming Lei Date: Tue, 11 May 2021 23:22:34 +0800 Subject: [PATCH 2/5] BACKPORT: blk-mq: grab rq->refcount before calling ->fn in blk_mq_tagset_busy_iter Grab rq->refcount before calling ->fn in blk_mq_tagset_busy_iter(), and this way will prevent the request from being re-used when ->fn is running. The approach is same as what we do during handling timeout. Fix request use-after-free(UAF) related with completion race or queue releasing: - If one rq is referred before rq->q is frozen, then queue won't be frozen before the request is released during iteration. - If one rq is referred after rq->q is frozen, refcount_inc_not_zero() will return false, and we won't iterate over this request. However, still one request UAF not covered: refcount_inc_not_zero() may read one freed request, and it will be handled in next patch. Tested-by: John Garry Reviewed-by: Christoph Hellwig Reviewed-by: Bart Van Assche Signed-off-by: Ming Lei Bug: 197804811 Change-Id: Ib80fe8b9f1d76d2489e41f3365fbd12d68a3b097 (cherry picked from commit 2e315dc07df009c3e29d6926871f62a30cfae394) [Pradeep: Resolved conflicts in block/blk-mq-tag.c] Signed-off-by: Pradeep P V K --- block/blk-mq-tag.c | 39 ++++++++++++++++++++++++++++++--------- block/blk-mq.c | 14 +++++++++----- block/blk-mq.h | 1 + 3 files changed, 40 insertions(+), 14 deletions(-) diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c index 008388e82b5c..a70b88587236 100644 --- a/block/blk-mq-tag.c +++ b/block/blk-mq-tag.c @@ -212,6 +212,16 @@ struct bt_iter_data { bool reserved; }; +static struct request *blk_mq_find_and_get_req(struct blk_mq_tags *tags, + unsigned int bitnr) +{ + struct request *rq = tags->rqs[bitnr]; + + if (!rq || !refcount_inc_not_zero(&rq->ref)) + return NULL; + return rq; +} + static bool bt_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data) { struct bt_iter_data *iter_data = data; @@ -219,18 +229,23 @@ 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; + bool ret = true; if (!reserved) bitnr += tags->nr_reserved_tags; - 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) - return iter_data->fn(hctx, rq, iter_data->data, reserved); - return true; + rq = blk_mq_find_and_get_req(tags, bitnr); + if (!rq) + return true; + + 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; } /** @@ -273,6 +288,7 @@ static bool bt_tags_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data) struct blk_mq_tags *tags = iter_data->tags; bool reserved = iter_data->reserved; struct request *rq; + bool ret = true; if (!reserved) bitnr += tags->nr_reserved_tags; @@ -281,11 +297,13 @@ static bool bt_tags_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 assining ->rqs[]. */ - rq = tags->rqs[bitnr]; - if (rq && blk_mq_request_started(rq)) - return iter_data->fn(rq, iter_data->data, reserved); - - return true; + rq = blk_mq_find_and_get_req(tags, bitnr); + if (!rq) + return true; + if (blk_mq_request_started(rq)) + ret = iter_data->fn(rq, iter_data->data, reserved); + blk_mq_put_rq_ref(rq); + return ret; } /** @@ -342,6 +360,9 @@ static void blk_mq_all_tag_busy_iter(struct blk_mq_tags *tags, * indicates whether or not @rq is a reserved request. Return * true to continue iterating tags, false to stop. * @priv: Will be passed as second argument to @fn. + * + * We grab one request reference before calling @fn and release it after + * @fn returns. */ void blk_mq_tagset_busy_iter(struct blk_mq_tag_set *tagset, busy_tag_iter_fn *fn, void *priv) diff --git a/block/blk-mq.c b/block/blk-mq.c index 0674f53c6052..252bb28d879d 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -886,6 +886,14 @@ static bool blk_mq_req_expired(struct request *rq, unsigned long *next) return false; } +void blk_mq_put_rq_ref(struct request *rq) +{ + if (is_flush_rq(rq, rq->mq_hctx)) + rq->end_io(rq, 0); + else if (refcount_dec_and_test(&rq->ref)) + __blk_mq_free_request(rq); +} + static bool blk_mq_check_expired(struct blk_mq_hw_ctx *hctx, struct request *rq, void *priv, bool reserved) { @@ -919,11 +927,7 @@ static bool blk_mq_check_expired(struct blk_mq_hw_ctx *hctx, if (blk_mq_req_expired(rq, next)) blk_mq_rq_timed_out(rq, reserved); - if (is_flush_rq(rq, hctx)) - rq->end_io(rq, 0); - else if (refcount_dec_and_test(&rq->ref)) - __blk_mq_free_request(rq); - + blk_mq_put_rq_ref(rq); return true; } diff --git a/block/blk-mq.h b/block/blk-mq.h index f2075978db50..8fa8a57ac9c2 100644 --- a/block/blk-mq.h +++ b/block/blk-mq.h @@ -47,6 +47,7 @@ void blk_mq_flush_busy_ctxs(struct blk_mq_hw_ctx *hctx, struct list_head *list); bool blk_mq_get_driver_tag(struct request *rq); struct request *blk_mq_dequeue_from_ctx(struct blk_mq_hw_ctx *hctx, struct blk_mq_ctx *start); +void blk_mq_put_rq_ref(struct request *rq); /* * Internal helpers for allocating/freeing the request map From bb96e7f45dc6ac1d6ec12190f1f286e3014fb068 Mon Sep 17 00:00:00 2001 From: Ming Lei Date: Tue, 11 May 2021 23:22:35 +0800 Subject: [PATCH 3/5] BACKPORT: blk-mq: clear stale request in tags->rq[] before freeing one request pool refcount_inc_not_zero() in bt_tags_iter() still may read one freed request. Fix the issue by the following approach: 1) hold a per-tags spinlock when reading ->rqs[tag] and calling refcount_inc_not_zero in bt_tags_iter() 2) clearing stale request referred via ->rqs[tag] before freeing request pool, the per-tags spinlock is held for clearing stale ->rq[tag] So after we cleared stale requests, bt_tags_iter() won't observe freed request any more, also the clearing will wait for pending request reference. The idea of clearing ->rqs[] is borrowed from John Garry's previous patch and one recent David's patch. Tested-by: John Garry Reviewed-by: David Jeffery Reviewed-by: Bart Van Assche Signed-off-by: Ming Lei Bug: 197804811 Change-Id: If49478d7b05d3f5b0a26966ddf9ae764cf2fb6b0 (cherry picked from commit bd63141d585bef14f4caf111f6d0e27fe2300ec6) [ refactored to avoid breaking KMI ] Signed-off-by: Pradeep P V K Signed-off-by: Todd Kjos --- block/blk-mq-tag.c | 17 ++++++++++++---- block/blk-mq-tag.h | 15 +++++++++++++++ block/blk-mq.c | 48 +++++++++++++++++++++++++++++++++++++++++----- 3 files changed, 71 insertions(+), 9 deletions(-) diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c index a70b88587236..e5d8949b922f 100644 --- a/block/blk-mq-tag.c +++ b/block/blk-mq-tag.c @@ -215,10 +215,16 @@ struct bt_iter_data { static struct request *blk_mq_find_and_get_req(struct blk_mq_tags *tags, unsigned int bitnr) { - struct request *rq = tags->rqs[bitnr]; + struct request *rq; + unsigned long flags; + struct ext_blk_mq_tags *etags; + etags = container_of(tags, struct ext_blk_mq_tags, tags); + spin_lock_irqsave(&etags->lock, flags); + rq = tags->rqs[bitnr]; if (!rq || !refcount_inc_not_zero(&rq->ref)) - return NULL; + rq = NULL; + spin_unlock_irqrestore(&etags->lock, flags); return rq; } @@ -486,18 +492,21 @@ struct blk_mq_tags *blk_mq_init_tags(unsigned int total_tags, int node, int alloc_policy) { struct blk_mq_tags *tags; + struct ext_blk_mq_tags *etags; if (total_tags > BLK_MQ_TAG_MAX) { pr_err("blk-mq: tag depth too large\n"); return NULL; } - tags = kzalloc_node(sizeof(*tags), GFP_KERNEL, node); - if (!tags) + etags = kzalloc_node(sizeof(*etags), GFP_KERNEL, node); + if (!etags) return NULL; + tags = &etags->tags; tags->nr_tags = total_tags; tags->nr_reserved_tags = reserved_tags; + spin_lock_init(&etags->lock); return blk_mq_init_bitmap_tags(tags, node, alloc_policy); } diff --git a/block/blk-mq-tag.h b/block/blk-mq-tag.h index 61deab0b5a5a..df345118cb53 100644 --- a/block/blk-mq-tag.h +++ b/block/blk-mq-tag.h @@ -21,6 +21,21 @@ struct blk_mq_tags { struct list_head page_list; }; +/* + * Extended tag address space map. This was needed + * to add a spinlock to blk_mq_tags in a KMI compliant + * way (no changes could be made to struct blk_mq_tags). + */ +struct ext_blk_mq_tags { + struct blk_mq_tags tags; + + /* + * used to clear request reference in rqs[] before freeing one + * request pool + */ + spinlock_t lock; +}; + extern struct blk_mq_tags *blk_mq_init_tags(unsigned int nr_tags, unsigned int reserved_tags, int node, int alloc_policy); extern void blk_mq_free_tags(struct blk_mq_tags *tags); diff --git a/block/blk-mq.c b/block/blk-mq.c index 252bb28d879d..063cdc96169c 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -1975,6 +1975,47 @@ static void blk_add_rq_to_plug(struct blk_plug *plug, struct request *rq) } } +static size_t order_to_size(unsigned int order) +{ + return (size_t)PAGE_SIZE << order; +} + +/* called before freeing request pool in @tags */ +static void blk_mq_clear_rq_mapping(struct blk_mq_tag_set *set, + struct blk_mq_tags *tags, unsigned int hctx_idx) +{ + struct blk_mq_tags *drv_tags = set->tags[hctx_idx]; + struct ext_blk_mq_tags *drv_etags; + struct page *page; + unsigned long flags; + + list_for_each_entry(page, &tags->page_list, lru) { + unsigned long start = (unsigned long)page_address(page); + unsigned long end = start + order_to_size(page->private); + int i; + + for (i = 0; i < set->queue_depth; i++) { + struct request *rq = drv_tags->rqs[i]; + unsigned long rq_addr = (unsigned long)rq; + + if (rq_addr >= start && rq_addr < end) { + WARN_ON_ONCE(refcount_read(&rq->ref) != 0); + cmpxchg(&drv_tags->rqs[i], rq, NULL); + } + } + } + + /* + * Wait until all pending iteration is done. + * + * Request reference is cleared and it is guaranteed to be observed + * after the ->lock is released. + */ + drv_etags = container_of(drv_tags, struct ext_blk_mq_tags, tags); + spin_lock_irqsave(&drv_etags->lock, flags); + spin_unlock_irqrestore(&drv_etags->lock, flags); +} + static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio) { const int is_sync = op_is_sync(bio->bi_opf); @@ -2100,6 +2141,8 @@ void blk_mq_free_rqs(struct blk_mq_tag_set *set, struct blk_mq_tags *tags, } } + blk_mq_clear_rq_mapping(set, tags, hctx_idx); + while (!list_empty(&tags->page_list)) { page = list_first_entry(&tags->page_list, struct page, lru); list_del_init(&page->lru); @@ -2159,11 +2202,6 @@ struct blk_mq_tags *blk_mq_alloc_rq_map(struct blk_mq_tag_set *set, return tags; } -static size_t order_to_size(unsigned int order) -{ - return (size_t)PAGE_SIZE << order; -} - static int blk_mq_init_request(struct blk_mq_tag_set *set, struct request *rq, unsigned int hctx_idx, int node) { From c9a3b51b07a03d515e15e0f79d1d1185e341b8f8 Mon Sep 17 00:00:00 2001 From: Ming Lei Date: Tue, 11 May 2021 23:22:36 +0800 Subject: [PATCH 4/5] BACKPORT: blk-mq: clearing flush request reference in tags->rqs[] Before we free request queue, clearing flush request reference in tags->rqs[], so that potential UAF can be avoided. Based on one patch written by David Jeffery. Tested-by: John Garry Reviewed-by: Bart Van Assche Reviewed-by: David Jeffery Signed-off-by: Ming Lei Bug: 197804811 Change-Id: I9600626e807a4eed546c21be808fabed2a9db9b1 (cherry picked from commit 364b61818f65045479e42e76ed8dd6f051778280) [ refactored to avoid breaking KMI ] Signed-off-by: Pradeep P V K Signed-off-by: Todd Kjos --- block/blk-mq.c | 37 ++++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/block/blk-mq.c b/block/blk-mq.c index 063cdc96169c..230778e4bae2 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -2334,16 +2334,51 @@ static void blk_mq_remove_cpuhp(struct blk_mq_hw_ctx *hctx) &hctx->cpuhp_dead); } +/* + * Before freeing hw queue, clearing the flush request reference in + * tags->rqs[] for avoiding potential UAF. + */ +static void blk_mq_clear_flush_rq_mapping(struct blk_mq_tags *tags, + unsigned int queue_depth, struct request *flush_rq) +{ + int i; + unsigned long flags; + struct ext_blk_mq_tags *etags; + + /* The hw queue may not be mapped yet */ + if (!tags) + return; + + WARN_ON_ONCE(refcount_read(&flush_rq->ref) != 0); + + for (i = 0; i < queue_depth; i++) + cmpxchg(&tags->rqs[i], flush_rq, NULL); + + /* + * Wait until all pending iteration is done. + * + * Request reference is cleared and it is guaranteed to be observed + * after the ->lock is released. + */ + etags = container_of(tags, struct ext_blk_mq_tags, tags); + spin_lock_irqsave(&etags->lock, flags); + spin_unlock_irqrestore(&etags->lock, flags); +} + /* hctx->ctxs will be freed in queue's release handler */ static void blk_mq_exit_hctx(struct request_queue *q, struct blk_mq_tag_set *set, struct blk_mq_hw_ctx *hctx, unsigned int hctx_idx) { + struct request *flush_rq = hctx->fq->flush_rq; + if (blk_mq_hw_queue_mapped(hctx)) blk_mq_tag_idle(hctx); + blk_mq_clear_flush_rq_mapping(set->tags[hctx_idx], + set->queue_depth, flush_rq); if (set->ops->exit_request) - set->ops->exit_request(set, hctx->fq->flush_rq, hctx_idx); + set->ops->exit_request(set, flush_rq, hctx_idx); if (set->ops->exit_hctx) set->ops->exit_hctx(hctx, hctx_idx); From ec1b6ab9fe80cf689780c9ee3b2cdb04cc11895e Mon Sep 17 00:00:00 2001 From: Ming Lei Date: Wed, 18 Aug 2021 09:09:25 +0800 Subject: [PATCH 5/5] BACKPORT: blk-mq: fix is_flush_rq is_flush_rq() is called from bt_iter()/bt_tags_iter(), and runs the following check: hctx->fq->flush_rq == req but the passed hctx from bt_iter()/bt_tags_iter() may be NULL because: 1) memory re-order in blk_mq_rq_ctx_init(): rq->mq_hctx = data->hctx; ... refcount_set(&rq->ref, 1); OR 2) tag re-use and ->rqs[] isn't updated with new request. Fix the issue by re-writing is_flush_rq() as: return rq->end_io == flush_end_io; which turns out simpler to follow and immune to data race since we have ordered WRITE rq->end_io and refcount_set(&rq->ref, 1). Fixes: 2e315dc07df0 ("blk-mq: grab rq->refcount before calling ->fn in blk_mq_tagset_busy_iter") Cc: "Blank-Burian, Markus, Dr." Cc: Yufen Yu Signed-off-by: Ming Lei Bug: 197804811 Change-Id: Iba551d228e5cd96749c13f4c19e13687b0e89196 (cherry picked from commit a9ed27a764156929efe714033edb3e9023c5f321) [Pradeep: Resolved conflicts in block/blk.h] Signed-off-by: Pradeep P V K --- block/blk-flush.c | 5 +++++ block/blk-mq.c | 2 +- block/blk.h | 6 +----- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/block/blk-flush.c b/block/blk-flush.c index 5aa6fada2259..e5735a213db7 100644 --- a/block/blk-flush.c +++ b/block/blk-flush.c @@ -252,6 +252,11 @@ static void flush_end_io(struct request *flush_rq, blk_status_t error) spin_unlock_irqrestore(&fq->mq_flush_lock, flags); } +bool is_flush_rq(struct request *rq) +{ + return rq->end_io == flush_end_io; +} + /** * blk_kick_flush - consider issuing flush request * @q: request_queue being kicked diff --git a/block/blk-mq.c b/block/blk-mq.c index 230778e4bae2..852724a34cb4 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -888,7 +888,7 @@ static bool blk_mq_req_expired(struct request *rq, unsigned long *next) void blk_mq_put_rq_ref(struct request *rq) { - if (is_flush_rq(rq, rq->mq_hctx)) + if (is_flush_rq(rq)) rq->end_io(rq, 0); else if (refcount_dec_and_test(&rq->ref)) __blk_mq_free_request(rq); diff --git a/block/blk.h b/block/blk.h index ee3d5664d962..945f127022eb 100644 --- a/block/blk.h +++ b/block/blk.h @@ -49,11 +49,7 @@ static inline void __blk_get_queue(struct request_queue *q) kobject_get(&q->kobj); } -static inline bool -is_flush_rq(struct request *req, struct blk_mq_hw_ctx *hctx) -{ - return hctx->fq->flush_rq == req; -} +bool is_flush_rq(struct request *req); struct blk_flush_queue *blk_alloc_flush_queue(struct request_queue *q, int node, int cmd_size, gfp_t flags);