* [PATCH] nvmet-fc: Fix potential Use-after-free bug in nvmet_fc_delete_target_queue()
@ 2022-09-16 8:29 Liang He
2022-09-16 18:09 ` James Smart
2022-09-19 1:46 ` Liang He
0 siblings, 2 replies; 6+ messages in thread
From: Liang He @ 2022-09-16 8:29 UTC (permalink / raw)
To: james.smart, hch, sagi, kch, linux-nvme; +Cc: windhl
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] nvmet-fc: Fix potential Use-after-free bug in nvmet_fc_delete_target_queue()
2022-09-16 8:29 [PATCH] nvmet-fc: Fix potential Use-after-free bug in nvmet_fc_delete_target_queue() Liang He
@ 2022-09-16 18:09 ` James Smart
2022-09-19 1:46 ` Liang He
1 sibling, 0 replies; 6+ messages in thread
From: James Smart @ 2022-09-16 18:09 UTC (permalink / raw)
To: linux-nvme
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re:[PATCH] nvmet-fc: Fix potential Use-after-free bug in nvmet_fc_delete_target_queue()
2022-09-16 8:29 [PATCH] nvmet-fc: Fix potential Use-after-free bug in nvmet_fc_delete_target_queue() Liang He
2022-09-16 18:09 ` James Smart
@ 2022-09-19 1:46 ` Liang He
2022-09-19 17:57 ` [PATCH] " James Smart
1 sibling, 1 reply; 6+ messages in thread
From: Liang He @ 2022-09-19 1:46 UTC (permalink / raw)
To: james.smart, hch, sagi, kch, linux-nvme
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'.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nvmet-fc: Fix potential Use-after-free bug in nvmet_fc_delete_target_queue()
2022-09-19 1:46 ` Liang He
@ 2022-09-19 17:57 ` James Smart
2022-09-20 12:54 ` Liang He
0 siblings, 1 reply; 6+ messages in thread
From: James Smart @ 2022-09-19 17:57 UTC (permalink / raw)
To: Liang He, james.smart, hch, sagi, kch, linux-nvme
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re:Re: [PATCH] nvmet-fc: Fix potential Use-after-free bug in nvmet_fc_delete_target_queue()
2022-09-19 17:57 ` [PATCH] " James Smart
@ 2022-09-20 12:54 ` Liang He
2022-09-20 15:53 ` James Smart
0 siblings, 1 reply; 6+ messages in thread
From: Liang He @ 2022-09-20 12:54 UTC (permalink / raw)
To: James Smart; +Cc: james.smart, hch, sagi, kch, linux-nvme
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nvmet-fc: Fix potential Use-after-free bug in nvmet_fc_delete_target_queue()
2022-09-20 12:54 ` Liang He
@ 2022-09-20 15:53 ` James Smart
0 siblings, 0 replies; 6+ messages in thread
From: James Smart @ 2022-09-20 15:53 UTC (permalink / raw)
To: Liang He; +Cc: james.smart, hch, sagi, kch, linux-nvme
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-20 15:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-16 8:29 [PATCH] nvmet-fc: Fix potential Use-after-free bug in nvmet_fc_delete_target_queue() Liang He
2022-09-16 18:09 ` James Smart
2022-09-19 1:46 ` Liang He
2022-09-19 17:57 ` [PATCH] " James Smart
2022-09-20 12:54 ` Liang He
2022-09-20 15:53 ` James Smart
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.