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

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

[Liang He]

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);
 
-- 
2.25.1

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

[James Smart]

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

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

[Liang He]

At 2022-09-16 16:29:53, "Liang He" <windhl@126.com> 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);
> 
>-- 
>2.25.1

Sorry, my patch is totally wrong as the 'qlock' is embeded into queue.
So if queue is freed, the 'qlock' will also be freed.

Now, we can only hope the 'nvmet_fc_tgt_q_put' in lin 941 will never really free the 'queue'.

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

[James Smart]

On 9/18/2022 6:46 PM, Liang He wrote:
>>
>> 		kfree(deferfcp);
>>
>> -		spin_lock_irqsave(&queue->qlock, flags);
>> +		spin_lock_irqsave(q_lock, flags);
>> 	}
>> 	spin_unlock_irqrestore(&queue->qlock, flags);
>>
>> -- 
>> 2.25.1
> 
> Sorry, my patch is totally wrong as the 'qlock' is embeded into queue.
> So if queue is freed, the 'qlock' will also be freed.
> 
> Now, we can only hope the 'nvmet_fc_tgt_q_put' in lin 941 will never really free the 'queue'.

Did you actually see that occur (line 941 freed the queue) ?

-- james

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

[Liang He]

At 2022-09-20 01:57:05, "James Smart" <jsmart2021@gmail.com> wrote:
>On 9/18/2022 6:46 PM, Liang He wrote:
>>>
>>> 		kfree(deferfcp);
>>>
>>> -		spin_lock_irqsave(&queue->qlock, flags);
>>> +		spin_lock_irqsave(q_lock, flags);
>>> 	}
>>> 	spin_unlock_irqrestore(&queue->qlock, flags);
>>>
>>> -- 
>>> 2.25.1
>> 
>> Sorry, my patch is totally wrong as the 'qlock' is embeded into queue.
>> So if queue is freed, the 'qlock' will also be freed.
>> 
>> Now, we can only hope the 'nvmet_fc_tgt_q_put' in lin 941 will never really free the 'queue'.
>
>Did you actually see that occur (line 941 freed the queue) ?
>
>-- james

Hi, James,

I actually have not seen this as I use static method to detect it.

While there will be no UAF in current version, I think we should not use the 
reference after we put it, right?

Liang

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

[James Smart]

On 9/20/2022 5:54 AM, Liang He wrote:
> 
> At 2022-09-20 01:57:05, "James Smart" <jsmart2021@gmail.com> wrote:
>> On 9/18/2022 6:46 PM, Liang He wrote:
>>>>
>>>> 		kfree(deferfcp);
>>>>
>>>> -		spin_lock_irqsave(&queue->qlock, flags);
>>>> +		spin_lock_irqsave(q_lock, flags);
>>>> 	}
>>>> 	spin_unlock_irqrestore(&queue->qlock, flags);
>>>>
>>>> -- 
>>>> 2.25.1
>>>
>>> Sorry, my patch is totally wrong as the 'qlock' is embeded into queue.
>>> So if queue is freed, the 'qlock' will also be freed.
>>>
>>> Now, we can only hope the 'nvmet_fc_tgt_q_put' in lin 941 will never really free the 'queue'.
>>
>> Did you actually see that occur (line 941 freed the queue) ?
>>
>> -- james
> 
> Hi, James,
> 
> I actually have not seen this as I use static method to detect it.
> 
> While there will be no UAF in current version, I think we should not use the
> reference after we put it, right?
> 
> Liang

there are multiple gets thus puts for it. All depends on the heirarchy 
of what's happening. Have to track that through. Expectation, based on 
the implementation, is that wouldn't be the last reference so it 
wouldn't free it. now need to prove the truth of that.

-- james