UAF in blk_add_rq_to_plug()?

DISCUSSION Al Viro 2 participants 7 messages Oct 31, 2022 – Nov 1, 2022 Block layer

Share & tools

Al Viro

author

2022-10-31 22:13 UTC #0 lore raw

static void blk_add_rq_to_plug(struct blk_plug *plug, struct request *rq)

{
        struct request *last = rq_list_peek(&plug->mq_list);

Suppose it's not NULL...

        if (!plug->rq_count) {
                trace_block_plug(rq->q);
        } else if (plug->rq_count >= blk_plug_max_rq_count(plug) ||
                   (!blk_queue_nomerges(rq->q) &&
                    blk_rq_bytes(last) >= BLK_PLUG_FLUSH_SIZE)) {
... and we went here:
                blk_mq_flush_plug_list(plug, false);

All requests, including the one last points to, might get fed ->queue_rq() here. At which point there seems to be nothing to prevent them getting completed and freed on another CPU, possibly before we return here.

                trace_block_plug(rq->q);
        }

        if (!plug->multiple_queues && last && last->q != rq->q)

... and here we dereference last.

Shouldn't we reset last to NULL after the call of blk_mq_flush_plug_list() above?

Jens Axboe

maintainer

2022-10-31 22:42 UTC #1 lore raw

Quoted 17 lines · Al Viro

static void blk_add_rq_to_plug(struct blk_plug *plug, struct request *rq)
{
        struct request *last = rq_list_peek(&plug->mq_list);
Suppose it's not NULL...
        if (!plug->rq_count) {
                trace_block_plug(rq->q);
        } else if (plug->rq_count >= blk_plug_max_rq_count(plug) ||
                   (!blk_queue_nomerges(rq->q) &&
                    blk_rq_bytes(last) >= BLK_PLUG_FLUSH_SIZE)) {
... and we went here:
                blk_mq_flush_plug_list(plug, false);
All requests, including the one last points to, might get fed ->queue_rq() here. At which point there seems to be nothing to prevent them getting completed and freed on another CPU, possibly before we return here.

Quoted 8 lines

                trace_block_plug(rq->q);
        }

        if (!plug->multiple_queues && last && last->q != rq->q)
... and here we dereference last.
Shouldn't we reset last to NULL after the call of blk_mq_flush_plug_list() above?

There's no UAF here as the requests aren't freed. We could clear 'last' to make the code more explicit, and that would avoid any potential suboptimal behavior with ->multiple_queues being wrong.

Signature · 1 line

Jens Axboe

Al Viro

author

2022-10-31 23:35 UTC #2 lore raw

Quoted 24 lines · Jens Axboe

Quoted 23 lines · Al Viro
static void blk_add_rq_to_plug(struct blk_plug *plug, struct request *rq)
{
        struct request *last = rq_list_peek(&plug->mq_list);
Suppose it's not NULL...
        if (!plug->rq_count) {
                trace_block_plug(rq->q);
        } else if (plug->rq_count >= blk_plug_max_rq_count(plug) ||
                   (!blk_queue_nomerges(rq->q) &&
                    blk_rq_bytes(last) >= BLK_PLUG_FLUSH_SIZE)) {
... and we went here:
                blk_mq_flush_plug_list(plug, false);
All requests, including the one last points to, might get fed ->queue_rq() here. At which point there seems to be nothing to prevent them getting completed and freed on another CPU, possibly before we return here.
                trace_block_plug(rq->q);
        }

        if (!plug->multiple_queues && last && last->q != rq->q)
... and here we dereference last.

Quoted 6 lines

Quoted 2 lines
Shouldn't we reset last to NULL after the call of blk_mq_flush_plug_list() above?
There's no UAF here as the requests aren't freed. We could clear 'last' to make the code more explicit, and that would avoid any potential suboptimal behavior with ->multiple_queues being wrong.

Umm...
Suppose ->has_elevator is false and so's ->multiple_queues.
No ->queue_rqs(), so blk_mq_flush_plug_list() grabs rcu_read_lock() and hit blk_mq_plug_issue_direct().
blk_mq_plug_issue_direct() picks the first request off the list and passes it to blk_mq_request_issue_directly(), which passes it to __blk_mq_request_issue_directly(). There we grab a tag and proceed to __blk_mq_issue_directly(), which feeds request to ->queue_rq().

What's to stop e.g. worker on another CPU from picking that sucker, completing it and calling blk_mq_end_request() which completes all bio involved and calls blk_mq_free_request()?

If all of that manages to happen before blk_mq_flush_plug_list() returns to caller... Sure, you probably won't hit in on bare metal, but if you are in a KVM and this virtual CPU happens to lose the host timeslice... I've seen considerably more narrow race windows getting hit on such setups.

Am I missing something subtle here? It's been a long time since I've read through that area - as the matter of fact, I'm trying to refresh my memories of the bio_submit()-related code paths at the moment...

Jens Axboe

maintainer

2022-11-01 00:03 UTC #3 lore raw

Quoted 51 lines · Al Viro

Quoted 24 lines · Jens Axboe
Quoted 23 lines · Al Viro
static void blk_add_rq_to_plug(struct blk_plug *plug, struct request *rq)
{
        struct request *last = rq_list_peek(&plug->mq_list);
Suppose it's not NULL...
        if (!plug->rq_count) {
                trace_block_plug(rq->q);
        } else if (plug->rq_count >= blk_plug_max_rq_count(plug) ||
                   (!blk_queue_nomerges(rq->q) &&
                    blk_rq_bytes(last) >= BLK_PLUG_FLUSH_SIZE)) {
... and we went here:
                blk_mq_flush_plug_list(plug, false);
All requests, including the one last points to, might get fed ->queue_rq() here. At which point there seems to be nothing to prevent them getting completed and freed on another CPU, possibly before we return here.
                trace_block_plug(rq->q);
        }

        if (!plug->multiple_queues && last && last->q != rq->q)
... and here we dereference last.
Quoted 6 lines
Quoted 2 lines
Shouldn't we reset last to NULL after the call of blk_mq_flush_plug_list() above?
There's no UAF here as the requests aren't freed. We could clear 'last' to make the code more explicit, and that would avoid any potential suboptimal behavior with ->multiple_queues being wrong.
Umm...
Suppose ->has_elevator is false and so's ->multiple_queues.
No ->queue_rqs(), so blk_mq_flush_plug_list() grabs rcu_read_lock() and hit blk_mq_plug_issue_direct().
blk_mq_plug_issue_direct() picks the first request off the list and passes it to blk_mq_request_issue_directly(), which passes it to __blk_mq_request_issue_directly(). There we grab a tag and proceed to __blk_mq_issue_directly(), which feeds request to ->queue_rq().
What's to stop e.g. worker on another CPU from picking that sucker, completing it and calling blk_mq_end_request() which completes all bio involved and calls blk_mq_free_request()?
If all of that manages to happen before blk_mq_flush_plug_list() returns to caller... Sure, you probably won't hit in on bare metal, but if you are in a KVM and this virtual CPU happens to lose the host timeslice... I've seen considerably more narrow race windows getting hit on such setups.

Quoted 4 lines

Am I missing something subtle here? It's been a long time since I've read through that area - as the matter of fact, I'm trying to refresh my memories of the bio_submit()-related code paths at the moment...

With blk-mq, which all drivers are these days, the request memory is statically allocated when the driver is setup. I'm not denying that you could very well have issued AND completed the request which 'last' points to by the time we dereference it, but that won't be a UAF unless the device has also been quiesced and removed in between. Which I guess could indeed be a possibility since it is by definition on a different queue (->multiple_queues would be true, however, but that's also what would be required to reach that far into that statement).

This is different from the older days of a request being literally freed when it completes, which is what I initially reacted to.

As mentioned in the original reply, I do think we should just clear 'last' as you suggest. But it's not something we've seen on the FB fleet of servers, even with the majority of hosts running this code (and on VMs).

Signature · 1 line

Jens Axboe

Jens Axboe

maintainer

2022-11-01 00:06 UTC #4 lore raw

Quoted 66 lines · Jens Axboe

Quoted 51 lines · Al Viro
Quoted 24 lines · Jens Axboe
Quoted 23 lines · Al Viro
static void blk_add_rq_to_plug(struct blk_plug *plug, struct request *rq)
{
        struct request *last = rq_list_peek(&plug->mq_list);
Suppose it's not NULL...
        if (!plug->rq_count) {
                trace_block_plug(rq->q);
        } else if (plug->rq_count >= blk_plug_max_rq_count(plug) ||
                   (!blk_queue_nomerges(rq->q) &&
                    blk_rq_bytes(last) >= BLK_PLUG_FLUSH_SIZE)) {
... and we went here:
                blk_mq_flush_plug_list(plug, false);
All requests, including the one last points to, might get fed ->queue_rq() here. At which point there seems to be nothing to prevent them getting completed and freed on another CPU, possibly before we return here.
                trace_block_plug(rq->q);
        }

        if (!plug->multiple_queues && last && last->q != rq->q)
... and here we dereference last.
Quoted 6 lines
Quoted 2 lines
Shouldn't we reset last to NULL after the call of blk_mq_flush_plug_list() above?
There's no UAF here as the requests aren't freed. We could clear 'last' to make the code more explicit, and that would avoid any potential suboptimal behavior with ->multiple_queues being wrong.
Umm...
Suppose ->has_elevator is false and so's ->multiple_queues.
No ->queue_rqs(), so blk_mq_flush_plug_list() grabs rcu_read_lock() and hit blk_mq_plug_issue_direct().
blk_mq_plug_issue_direct() picks the first request off the list and passes it to blk_mq_request_issue_directly(), which passes it to __blk_mq_request_issue_directly(). There we grab a tag and proceed to __blk_mq_issue_directly(), which feeds request to ->queue_rq().
What's to stop e.g. worker on another CPU from picking that sucker, completing it and calling blk_mq_end_request() which completes all bio involved and calls blk_mq_free_request()?
If all of that manages to happen before blk_mq_flush_plug_list() returns to caller... Sure, you probably won't hit in on bare metal, but if you are in a KVM and this virtual CPU happens to lose the host timeslice... I've seen considerably more narrow race windows getting hit on such setups.
Quoted 4 lines
Am I missing something subtle here? It's been a long time since I've read through that area - as the matter of fact, I'm trying to refresh my memories of the bio_submit()-related code paths at the moment...
With blk-mq, which all drivers are these days, the request memory is statically allocated when the driver is setup. I'm not denying that you could very well have issued AND completed the request which 'last' points to by the time we dereference it, but that won't be a UAF unless the device has also been quiesced and removed in between. Which I guess could indeed be a possibility since it is by definition on a different queue (->multiple_queues would be true, however, but that's also what would be required to reach that far into that statement).

Quoted 7 lines

This is different from the older days of a request being literally freed when it completes, which is what I initially reacted to.
As mentioned in the original reply, I do think we should just clear 'last' as you suggest. But it's not something we've seen on the FB fleet of servers, even with the majority of hosts running this code (and on VMs).

Forgot to ask - do you want to send a patch for that, or do you just want me to cook one up with a reported-by for you?

Signature · 1 line

Jens Axboe

Al Viro

author

2022-11-01 00:54 UTC #5 lore raw

Quoted 14 lines · Jens Axboe

Quoted 14 lines
Quoted 4 lines
Am I missing something subtle here? It's been a long time since I've read through that area - as the matter of fact, I'm trying to refresh my memories of the bio_submit()-related code paths at the moment...
With blk-mq, which all drivers are these days, the request memory is statically allocated when the driver is setup. I'm not denying that you could very well have issued AND completed the request which 'last' points to by the time we dereference it, but that won't be a UAF unless the device has also been quiesced and removed in between. Which I guess could indeed be a possibility since it is by definition on a different queue (->multiple_queues would be true, however, but that's also what would be required to reach that far into that statement).

Quoted 2 lines

Quoted 2 lines
This is different from the older days of a request being literally freed when it completes, which is what I initially reacted to.

Got it (and my memories of struct request lifetime rules had been stale, indeed).

Quoted 7 lines

Quoted 4 lines
As mentioned in the original reply, I do think we should just clear 'last' as you suggest. But it's not something we've seen on the FB fleet of servers, even with the majority of hosts running this code (and on VMs).
Forgot to ask - do you want to send a patch for that, or do you just want me to cook one up with a reported-by for you?

You mean, try and put together a commit message for that one-liner? ;-)

[PATCH] blk_add_rq_to_plug(): 'last' is stale after flush, if we end up doing one

blk_mq_flush_plug_list() empties ->mq_list and request we'd peeked there before that call is gone; in any case, we are not dealing with a mix of requests for different queues now - there's no requests left in the plug.

Signed-off-by Al Viro <[email protected]>

---

block/blk-mq.c +1 -0

index 8070b6c10e8d..a0e044a645b3 100644
@@ -1257,6 +1257,7 @@ static void blk_add_rq_to_plug(struct blk_plug *plug, struct request *rq)
 		   (!blk_queue_nomerges(rq->q) &&
 		    blk_rq_bytes(last) >= BLK_PLUG_FLUSH_SIZE)) {
 		blk_mq_flush_plug_list(plug, false);
+		last = NULL;
 		trace_block_plug(rq->q);
 	}

Jens Axboe

maintainer

2022-11-01 02:23 UTC #6 lore raw

Quoted 27 lines · Al Viro

Quoted 14 lines · Jens Axboe
Quoted 14 lines
Quoted 4 lines
Am I missing something subtle here? It's been a long time since I've read through that area - as the matter of fact, I'm trying to refresh my memories of the bio_submit()-related code paths at the moment...
With blk-mq, which all drivers are these days, the request memory is statically allocated when the driver is setup. I'm not denying that you could very well have issued AND completed the request which 'last' points to by the time we dereference it, but that won't be a UAF unless the device has also been quiesced and removed in between. Which I guess could indeed be a possibility since it is by definition on a different queue (->multiple_queues would be true, however, but that's also what would be required to reach that far into that statement).
Quoted 2 lines
Quoted 2 lines
This is different from the older days of a request being literally freed when it completes, which is what I initially reacted to.
Got it (and my memories of struct request lifetime rules had been stale, indeed).
Quoted 5 lines
Quoted 4 lines
As mentioned in the original reply, I do think we should just clear 'last' as you suggest. But it's not something we've seen on the FB fleet of servers, even with the majority of hosts running this code (and on VMs).

Quoted 4 lines

Quoted 2 lines
Forgot to ask - do you want to send a patch for that, or do you just want me to cook one up with a reported-by for you?
You mean, try and put together a commit message for that one-liner? ;-)

Right, thanks for doing that! Queued it up for 6.1.

Signature · 1 line

Jens Axboe