linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH BUGFIX 0/2] block, bfq: fix user after free
@ 2019-08-07 14:17 Paolo Valente
  2019-08-07 14:17 ` [PATCH BUGFIX 1/2] block, bfq: reset last_completed_rq_bfqq if the pointed queue is freed Paolo Valente
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Paolo Valente @ 2019-08-07 14:17 UTC (permalink / raw)
  To: Jens Axboe
  Cc: linux-block, linux-kernel, ulf.hansson, linus.walleij,
	bfq-iosched, oleksandr, pavel, Paolo Valente

Hi Jens,
this series contains a pair of fixes for the UAF reported in
[1]. These patches are the result of the testing described in this
Chrome OS issue [2] since Comment 57.

Thanks,
Paolo

[1] https://lkml.org/lkml/2019/7/27/254
[2] https://bugs.chromium.org/p/chromium/issues/detail?id=931295#c57


Paolo Valente (2):
  block, bfq: reset last_completed_rq_bfqq if the pointed queue is freed
  block, bfq: move update of waker and woken list to queue freeing

 block/bfq-iosched.c | 54 ++++++++++++++++++++++++++++++---------------
 1 file changed, 36 insertions(+), 18 deletions(-)

--
2.20.1

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH BUGFIX 1/2] block, bfq: reset last_completed_rq_bfqq if the pointed queue is freed
  2019-08-07 14:17 [PATCH BUGFIX 0/2] block, bfq: fix user after free Paolo Valente
@ 2019-08-07 14:17 ` Paolo Valente
  2019-08-07 14:17 ` [PATCH BUGFIX 2/2] block, bfq: move update of waker and woken list to queue freeing Paolo Valente
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: Paolo Valente @ 2019-08-07 14:17 UTC (permalink / raw)
  To: Jens Axboe
  Cc: linux-block, linux-kernel, ulf.hansson, linus.walleij,
	bfq-iosched, oleksandr, pavel, Paolo Valente, Douglas Anderson

Since commit 13a857a4c4e8 ("block, bfq: detect wakers and
unconditionally inject their I/O"), BFQ stores, in a per-device
pointer last_completed_rq_bfqq, the last bfq_queue that had an I/O
request completed. If some bfq_queue receives new I/O right after the
last request of last_completed_rq_bfqq has been completed, then
last_completed_rq_bfqq may be a waker bfq_queue.

But if the bfq_queue last_completed_rq_bfqq points to is freed, then
last_completed_rq_bfqq becomes a dangling reference. This commit
resets last_completed_rq_bfqq if the pointed bfq_queue is freed.

Fixes: 13a857a4c4e8 ("block, bfq: detect wakers and unconditionally inject their I/O")
Reported-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
---
 block/bfq-iosched.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 586fcfe227ea..b2009650afc2 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -1924,12 +1924,13 @@ static void bfq_add_request(struct request *rq)
 		 * confirmed no later than during the next
 		 * I/O-plugging interval for bfqq.
 		 */
-		if (!bfq_bfqq_has_short_ttime(bfqq) &&
+		if (bfqd->last_completed_rq_bfqq &&
+		    !bfq_bfqq_has_short_ttime(bfqq) &&
 		    ktime_get_ns() - bfqd->last_completion <
 		    200 * NSEC_PER_USEC) {
 			if (bfqd->last_completed_rq_bfqq != bfqq &&
-				   bfqd->last_completed_rq_bfqq !=
-				   bfqq->waker_bfqq) {
+			    bfqd->last_completed_rq_bfqq !=
+			    bfqq->waker_bfqq) {
 				/*
 				 * First synchronization detected with
 				 * a candidate waker queue, or with a
@@ -4808,6 +4809,9 @@ void bfq_put_queue(struct bfq_queue *bfqq)
 			bfqq->bfqd->burst_size--;
 	}
 
+	if (bfqq->bfqd && bfqq->bfqd->last_completed_rq_bfqq == bfqq)
+		bfqq->bfqd->last_completed_rq_bfqq = NULL;
+
 	kmem_cache_free(bfq_pool, bfqq);
 #ifdef CONFIG_BFQ_GROUP_IOSCHED
 	bfqg_and_blkg_put(bfqg);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH BUGFIX 2/2] block, bfq: move update of waker and woken list to queue freeing
  2019-08-07 14:17 [PATCH BUGFIX 0/2] block, bfq: fix user after free Paolo Valente
  2019-08-07 14:17 ` [PATCH BUGFIX 1/2] block, bfq: reset last_completed_rq_bfqq if the pointed queue is freed Paolo Valente
@ 2019-08-07 14:17 ` Paolo Valente
  2019-08-08 13:31 ` [PATCH BUGFIX 0/2] block, bfq: fix user after free Jens Axboe
  2019-08-13 11:14 ` Pavel Machek
  3 siblings, 0 replies; 5+ messages in thread
From: Paolo Valente @ 2019-08-07 14:17 UTC (permalink / raw)
  To: Jens Axboe
  Cc: linux-block, linux-kernel, ulf.hansson, linus.walleij,
	bfq-iosched, oleksandr, pavel, Paolo Valente, Douglas Anderson

Since commit 13a857a4c4e8 ("block, bfq: detect wakers and
unconditionally inject their I/O"), every bfq_queue has a pointer to a
waker bfq_queue and a list of the bfq_queues it may wake. In this
respect, when a bfq_queue, say Q, remains with no I/O source attached
to it, Q cannot be woken by any other bfq_queue, and cannot wake any
other bfq_queue. Then Q must be removed from the woken list of its
possible waker bfq_queue, and all bfq_queues in the woken list of Q
must stop having a waker bfq_queue.

Q remains with no I/O source in two cases: when the last process
associated with Q exits or when such a process gets associated with a
different bfq_queue. Unfortunately, commit 13a857a4c4e8 ("block, bfq:
detect wakers and unconditionally inject their I/O") performed the
above updates only in the first case.

This commit fixes this bug by moving these updates to when Q gets
freed. This is a simple and safe way to handle all cases, as both the
above events, process exit and re-association, lead to Q being freed
soon, and because dangling references would come out only after Q gets
freed (if no update were performed).

Fixes: 13a857a4c4e8 ("block, bfq: detect wakers and unconditionally inject their I/O")
Reported-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
---
 block/bfq-iosched.c | 44 +++++++++++++++++++++++++++++---------------
 1 file changed, 29 insertions(+), 15 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index b2009650afc2..5f477501bb3d 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -4765,6 +4765,8 @@ static struct request *bfq_dispatch_request(struct blk_mq_hw_ctx *hctx)
  */
 void bfq_put_queue(struct bfq_queue *bfqq)
 {
+	struct bfq_queue *item;
+	struct hlist_node *n;
 #ifdef CONFIG_BFQ_GROUP_IOSCHED
 	struct bfq_group *bfqg = bfqq_group(bfqq);
 #endif
@@ -4809,6 +4811,33 @@ void bfq_put_queue(struct bfq_queue *bfqq)
 			bfqq->bfqd->burst_size--;
 	}
 
+	/*
+	 * bfqq does not exist any longer, so it cannot be woken by
+	 * any other queue, and cannot wake any other queue. Then bfqq
+	 * must be removed from the woken list of its possible waker
+	 * queue, and all queues in the woken list of bfqq must stop
+	 * having a waker queue. Strictly speaking, these updates
+	 * should be performed when bfqq remains with no I/O source
+	 * attached to it, which happens before bfqq gets freed. In
+	 * particular, this happens when the last process associated
+	 * with bfqq exits or gets associated with a different
+	 * queue. However, both events lead to bfqq being freed soon,
+	 * and dangling references would come out only after bfqq gets
+	 * freed. So these updates are done here, as a simple and safe
+	 * way to handle all cases.
+	 */
+	/* remove bfqq from woken list */
+	if (!hlist_unhashed(&bfqq->woken_list_node))
+		hlist_del_init(&bfqq->woken_list_node);
+
+	/* reset waker for all queues in woken list */
+	hlist_for_each_entry_safe(item, n, &bfqq->woken_list,
+				  woken_list_node) {
+		item->waker_bfqq = NULL;
+		bfq_clear_bfqq_has_waker(item);
+		hlist_del_init(&item->woken_list_node);
+	}
+
 	if (bfqq->bfqd && bfqq->bfqd->last_completed_rq_bfqq == bfqq)
 		bfqq->bfqd->last_completed_rq_bfqq = NULL;
 
@@ -4839,9 +4868,6 @@ static void bfq_put_cooperator(struct bfq_queue *bfqq)
 
 static void bfq_exit_bfqq(struct bfq_data *bfqd, struct bfq_queue *bfqq)
 {
-	struct bfq_queue *item;
-	struct hlist_node *n;
-
 	if (bfqq == bfqd->in_service_queue) {
 		__bfq_bfqq_expire(bfqd, bfqq, BFQQE_BUDGET_TIMEOUT);
 		bfq_schedule_dispatch(bfqd);
@@ -4851,18 +4877,6 @@ static void bfq_exit_bfqq(struct bfq_data *bfqd, struct bfq_queue *bfqq)
 
 	bfq_put_cooperator(bfqq);
 
-	/* remove bfqq from woken list */
-	if (!hlist_unhashed(&bfqq->woken_list_node))
-		hlist_del_init(&bfqq->woken_list_node);
-
-	/* reset waker for all queues in woken list */
-	hlist_for_each_entry_safe(item, n, &bfqq->woken_list,
-				  woken_list_node) {
-		item->waker_bfqq = NULL;
-		bfq_clear_bfqq_has_waker(item);
-		hlist_del_init(&item->woken_list_node);
-	}
-
 	bfq_put_queue(bfqq); /* release process reference */
 }
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH BUGFIX 0/2] block, bfq: fix user after free
  2019-08-07 14:17 [PATCH BUGFIX 0/2] block, bfq: fix user after free Paolo Valente
  2019-08-07 14:17 ` [PATCH BUGFIX 1/2] block, bfq: reset last_completed_rq_bfqq if the pointed queue is freed Paolo Valente
  2019-08-07 14:17 ` [PATCH BUGFIX 2/2] block, bfq: move update of waker and woken list to queue freeing Paolo Valente
@ 2019-08-08 13:31 ` Jens Axboe
  2019-08-13 11:14 ` Pavel Machek
  3 siblings, 0 replies; 5+ messages in thread
From: Jens Axboe @ 2019-08-08 13:31 UTC (permalink / raw)
  To: Paolo Valente
  Cc: linux-block, linux-kernel, ulf.hansson, linus.walleij,
	bfq-iosched, oleksandr, pavel

On 8/7/19 7:17 AM, Paolo Valente wrote:
> Hi Jens,
> this series contains a pair of fixes for the UAF reported in
> [1]. These patches are the result of the testing described in this
> Chrome OS issue [2] since Comment 57.

Applied, thanks.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH BUGFIX 0/2] block, bfq: fix user after free
  2019-08-07 14:17 [PATCH BUGFIX 0/2] block, bfq: fix user after free Paolo Valente
                   ` (2 preceding siblings ...)
  2019-08-08 13:31 ` [PATCH BUGFIX 0/2] block, bfq: fix user after free Jens Axboe
@ 2019-08-13 11:14 ` Pavel Machek
  3 siblings, 0 replies; 5+ messages in thread
From: Pavel Machek @ 2019-08-13 11:14 UTC (permalink / raw)
  To: Paolo Valente
  Cc: Jens Axboe, linux-block, linux-kernel, ulf.hansson,
	linus.walleij, bfq-iosched, oleksandr, pavel

[-- Attachment #1: Type: text/plain, Size: 508 bytes --]

Hi!

> this series contains a pair of fixes for the UAF reported in
> [1]. These patches are the result of the testing described in this
> Chrome OS issue [2] since Comment 57.

This seems to have solved crashes with chromium on x220 from
"v5.3-rc2: crashes and scrolling in web browser now has audio
feedback" thread.

Best regards,
							Pavel

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-08-13 11:14 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-07 14:17 [PATCH BUGFIX 0/2] block, bfq: fix user after free Paolo Valente
2019-08-07 14:17 ` [PATCH BUGFIX 1/2] block, bfq: reset last_completed_rq_bfqq if the pointed queue is freed Paolo Valente
2019-08-07 14:17 ` [PATCH BUGFIX 2/2] block, bfq: move update of waker and woken list to queue freeing Paolo Valente
2019-08-08 13:31 ` [PATCH BUGFIX 0/2] block, bfq: fix user after free Jens Axboe
2019-08-13 11:14 ` Pavel Machek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).