Re: UAF in blk_add_rq_to_plug()?

[Al Viro <viro@zeniv.linux.org.uk>]

On Mon, Oct 31, 2022 at 06:06:34PM -0600, Jens Axboe wrote:

> >> 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.

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

> > 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 <viro@zeniv.linux.org.uk>
---
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 8070b6c10e8d..a0e044a645b3 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -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);
 	}