Re: [PATCH] nvmet-fc: Fix potential Use-after-free bug in nvmet_fc_delete_target_queue()

[James Smart <jsmart2021@gmail.com>]

On 9/16/2022 1:29 AM, Liang He wrote:
> In nvmet_fc_delete_target_queue(), the nvmet_fc_tgt_q_put() may free
> queue if its refcount hits 0. And the following code will dereference
> it to get qclock by 'queue->qlock', so there is a potential UAF bug.
> 
> Fixes: 619c62dcc62b ("nvmet-fc: correct ref counting error when deferred rcv used")
> Signed-off-by: Liang He <windhl@126.com>
> ---
>   drivers/nvme/target/fc.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
> index ab2627e17bb9..32e1d62017d2 100644
> --- a/drivers/nvme/target/fc.c
> +++ b/drivers/nvme/target/fc.c
> @@ -881,6 +881,7 @@ nvmet_fc_delete_target_queue(struct nvmet_fc_tgt_queue *queue)
>   	struct nvmet_fc_tgtport *tgtport = queue->assoc->tgtport;
>   	struct nvmet_fc_fcp_iod *fod = queue->fod;
>   	struct nvmet_fc_defer_fcp_req *deferfcp, *tempptr;
> +	spinlock_t *q_lock = &queue->qlock;
>   	unsigned long flags;
>   	int i;
>   	bool disconnect;
> @@ -942,7 +943,7 @@ nvmet_fc_delete_target_queue(struct nvmet_fc_tgt_queue *queue)
>   
>   		kfree(deferfcp);
>   
> -		spin_lock_irqsave(&queue->qlock, flags);
> +		spin_lock_irqsave(q_lock, flags);
>   	}
>   	spin_unlock_irqrestore(&queue->qlock, flags);
>   

several things about this are fishy...

on the surface - why was the following spin_unlock_irqrestore() not 
changed for q_lock as well ?

This sounds like its correcting a symptom not necessarily the problem.
The put that you are correcting after should not have freed it, the put 
at the bottom of the routine should have. So it's pointing at an 
imbalanced reference counting.

Need to take a look at commit 619c62dcc62b9, which added the put that is 
freeing. To see if it wasn't done right.

-- james