Re: [PATCH V6 0/4] blk-mq: fix request UAF related with iterating over tagset requests

From: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
To: Ming Lei <ming.lei@redhat.com>
Cc: Jens Axboe <axboe@kernel.dk>,
	"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	Bart Van Assche <bvanassche@acm.org>,
	Hannes Reinecke <hare@suse.de>,
	John Garry <john.garry@huawei.com>,
	Christoph Hellwig <hch@lst.de>,
	David Jeffery <djeffery@redhat.com>
Subject: Re: [PATCH V6 0/4] blk-mq: fix request UAF related with iterating over tagset requests
Date: Tue, 11 May 2021 12:23:21 +0000	[thread overview]
Message-ID: <20210511122320.2dimco7hb5qm3rjv@shindev.dhcp.fujisawa.hgst.com> (raw)
In-Reply-To: <YJol/7YGUxWbBdiK@T590>

On May 11, 2021 / 14:36, Ming Lei wrote:
> Hello Shinichiro,
> 
> Thanks for your test!
> 
> On Tue, May 11, 2021 at 05:05:52AM +0000, Shinichiro Kawasaki wrote:
> > On May 07, 2021 / 22:42, Ming Lei wrote:
> > > Hi Jens,
> > > 
> > > This patchset fixes the request UAF issue by one simple approach,
> > > without clearing ->rqs[] in fast path, please consider it for 5.13.
> > > 
> > > 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.
> > 
> > Ming, thank you for your effort to fix the UAF issue. I applied the V6 series to
> > the kernel v5.13-rc1, and confirmed that the series avoids the UAF I had been
> > observing with blktests block/005 and HDD behind HBA. This is good. However, I
> > found that the series triggered block/029 hang. Let me share the kernel message
> > below, which was printed at the hang. KASAN reported null-ptr-deref.
> > 
> > [ 2124.489023] run blktests block/029 at 2021-05-11 13:42:22
> > [ 2124.561386] null_blk: module loaded
> > [ 2125.201166] general protection fault, probably for non-canonical address 0xdffffc0000000012: 0000 [#1] SMP KASAN PTI
> > [ 2125.212387] KASAN: null-ptr-deref in range [0x0000000000000090-0x0000000000000097]
> 
> It is because this hw queue isn't mapped yet and new added hw queue is
> mapped in blk_mq_map_swqueue(), and the following change can fix it, and
> I will post V7 after careful test.
> 
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index fcd5ed79011f..691b555c26fa 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2652,6 +2652,10 @@ static void blk_mq_clear_flush_rq_mapping(struct blk_mq_tags *tags,
>  	int i;
>  	unsigned long flags;
>  
> +	/* return if hw queue isn't mapped */
> +	if (!tags)
> +		return;
> +
>  	WARN_ON_ONCE(refcount_read(&flush_rq->ref) != 0);
>  
>  	for (i = 0; i < queue_depth; i++)

Thank you Ming. I confirmed that this change avoids the hang at block/029.

-- 
Best Regards,
Shin'ichiro Kawasaki