* [PATCH v2] nvmet: Avoid potential UAF in nvmet_req_complete()
@ 2023-03-06 1:13 Damien Le Moal
2023-03-06 5:04 ` Chaitanya Kulkarni
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Damien Le Moal @ 2023-03-06 1:13 UTC (permalink / raw)
To: linux-nvme, Christoph Hellwig, Sagi Grimberg, Chaitanya Kulkarni
An nvme target ->queue_response() operation implementation may free the
request passed as argument. Such implementation potentially could result
in a use after free of the request pointer when percpu_ref_put() is
called in nvmet_req_complete().
Avoid such problem by using a local variable to save the sq pointer
before calling __nvmet_req_complete(), thus avoiding dereferencing the
req pointer after that function call.
Fixes: a07b4970f464 ("nvmet: add a generic NVMe target")
Cc: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
Changes from v1:
* Added Fixes tag and cc stable
drivers/nvme/target/core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index f66ed13d7c11..3935165048e7 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -756,8 +756,10 @@ static void __nvmet_req_complete(struct nvmet_req *req, u16 status)
void nvmet_req_complete(struct nvmet_req *req, u16 status)
{
+ struct nvmet_sq *sq = req->sq;
+
__nvmet_req_complete(req, status);
- percpu_ref_put(&req->sq->ref);
+ percpu_ref_put(&sq->ref);
}
EXPORT_SYMBOL_GPL(nvmet_req_complete);
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] nvmet: Avoid potential UAF in nvmet_req_complete()
2023-03-06 1:13 [PATCH v2] nvmet: Avoid potential UAF in nvmet_req_complete() Damien Le Moal
@ 2023-03-06 5:04 ` Chaitanya Kulkarni
2023-03-06 13:52 ` Sagi Grimberg
2023-03-09 9:42 ` Christoph Hellwig
2 siblings, 0 replies; 6+ messages in thread
From: Chaitanya Kulkarni @ 2023-03-06 5:04 UTC (permalink / raw)
To: Damien Le Moal, linux-nvme@lists.infradead.org, Christoph Hellwig,
Sagi Grimberg, Chaitanya Kulkarni
On 3/5/2023 5:13 PM, Damien Le Moal wrote:
> An nvme target ->queue_response() operation implementation may free the
> request passed as argument. Such implementation potentially could result
> in a use after free of the request pointer when percpu_ref_put() is
> called in nvmet_req_complete().
>
> Avoid such problem by using a local variable to save the sq pointer
> before calling __nvmet_req_complete(), thus avoiding dereferencing the
> req pointer after that function call.
>
> Fixes: a07b4970f464 ("nvmet: add a generic NVMe target")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>
Looks good.
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
-ck
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] nvmet: Avoid potential UAF in nvmet_req_complete()
2023-03-06 1:13 [PATCH v2] nvmet: Avoid potential UAF in nvmet_req_complete() Damien Le Moal
2023-03-06 5:04 ` Chaitanya Kulkarni
@ 2023-03-06 13:52 ` Sagi Grimberg
2023-03-06 13:57 ` Damien Le Moal
2023-03-09 9:42 ` Christoph Hellwig
2 siblings, 1 reply; 6+ messages in thread
From: Sagi Grimberg @ 2023-03-06 13:52 UTC (permalink / raw)
To: Damien Le Moal, linux-nvme, Christoph Hellwig, Chaitanya Kulkarni
Hey Damien,
> An nvme target ->queue_response() operation implementation may free the
> request passed as argument. Such implementation potentially could result
> in a use after free of the request pointer when percpu_ref_put() is
> called in nvmet_req_complete().
Can you point me to which transport frees the request?
>
> Avoid such problem by using a local variable to save the sq pointer
> before calling __nvmet_req_complete(), thus avoiding dereferencing the
> req pointer after that function call.
>
> Fixes: a07b4970f464 ("nvmet: add a generic NVMe target")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>
> Changes from v1:
> * Added Fixes tag and cc stable
>
> drivers/nvme/target/core.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index f66ed13d7c11..3935165048e7 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -756,8 +756,10 @@ static void __nvmet_req_complete(struct nvmet_req *req, u16 status)
>
> void nvmet_req_complete(struct nvmet_req *req, u16 status)
> {
> + struct nvmet_sq *sq = req->sq;
> +
> __nvmet_req_complete(req, status);
> - percpu_ref_put(&req->sq->ref);
> + percpu_ref_put(&sq->ref);
> }
> EXPORT_SYMBOL_GPL(nvmet_req_complete);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] nvmet: Avoid potential UAF in nvmet_req_complete()
2023-03-06 13:52 ` Sagi Grimberg
@ 2023-03-06 13:57 ` Damien Le Moal
2023-03-06 14:00 ` Sagi Grimberg
0 siblings, 1 reply; 6+ messages in thread
From: Damien Le Moal @ 2023-03-06 13:57 UTC (permalink / raw)
To: Sagi Grimberg, linux-nvme, Christoph Hellwig, Chaitanya Kulkarni
On 3/6/23 22:52, Sagi Grimberg wrote:
> Hey Damien,
>
>> An nvme target ->queue_response() operation implementation may free the
>> request passed as argument. Such implementation potentially could result
>> in a use after free of the request pointer when percpu_ref_put() is
>> called in nvmet_req_complete().
>
> Can you point me to which transport frees the request?
My prototype PCIe endpoint nvme function driver, not upstream yet.
The endpoint board keeps randomly crashing without this patch and not freeing
the request (embedded in a struct) in the ->queue_response() operation would
require *a lot* more code.
>
>>
>> Avoid such problem by using a local variable to save the sq pointer
>> before calling __nvmet_req_complete(), thus avoiding dereferencing the
>> req pointer after that function call.
>>
>> Fixes: a07b4970f464 ("nvmet: add a generic NVMe target")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> ---
>>
>> Changes from v1:
>> * Added Fixes tag and cc stable
>>
>> drivers/nvme/target/core.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
>> index f66ed13d7c11..3935165048e7 100644
>> --- a/drivers/nvme/target/core.c
>> +++ b/drivers/nvme/target/core.c
>> @@ -756,8 +756,10 @@ static void __nvmet_req_complete(struct nvmet_req *req, u16 status)
>>
>> void nvmet_req_complete(struct nvmet_req *req, u16 status)
>> {
>> + struct nvmet_sq *sq = req->sq;
>> +
>> __nvmet_req_complete(req, status);
>> - percpu_ref_put(&req->sq->ref);
>> + percpu_ref_put(&sq->ref);
>> }
>> EXPORT_SYMBOL_GPL(nvmet_req_complete);
>>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] nvmet: Avoid potential UAF in nvmet_req_complete()
2023-03-06 13:57 ` Damien Le Moal
@ 2023-03-06 14:00 ` Sagi Grimberg
0 siblings, 0 replies; 6+ messages in thread
From: Sagi Grimberg @ 2023-03-06 14:00 UTC (permalink / raw)
To: Damien Le Moal, linux-nvme, Christoph Hellwig, Chaitanya Kulkarni
>> Hey Damien,
>>
>>> An nvme target ->queue_response() operation implementation may free the
>>> request passed as argument. Such implementation potentially could result
>>> in a use after free of the request pointer when percpu_ref_put() is
>>> called in nvmet_req_complete().
>>
>> Can you point me to which transport frees the request?
>
> My prototype PCIe endpoint nvme function driver, not upstream yet.
>
> The endpoint board keeps randomly crashing without this patch and not freeing
> the request (embedded in a struct) in the ->queue_response() operation would
> require *a lot* more code.
Got it,
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] nvmet: Avoid potential UAF in nvmet_req_complete()
2023-03-06 1:13 [PATCH v2] nvmet: Avoid potential UAF in nvmet_req_complete() Damien Le Moal
2023-03-06 5:04 ` Chaitanya Kulkarni
2023-03-06 13:52 ` Sagi Grimberg
@ 2023-03-09 9:42 ` Christoph Hellwig
2 siblings, 0 replies; 6+ messages in thread
From: Christoph Hellwig @ 2023-03-09 9:42 UTC (permalink / raw)
To: Damien Le Moal
Cc: linux-nvme, Christoph Hellwig, Sagi Grimberg, Chaitanya Kulkarni
Thanks,
applied to nvme-6.3.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-03-09 9:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-06 1:13 [PATCH v2] nvmet: Avoid potential UAF in nvmet_req_complete() Damien Le Moal
2023-03-06 5:04 ` Chaitanya Kulkarni
2023-03-06 13:52 ` Sagi Grimberg
2023-03-06 13:57 ` Damien Le Moal
2023-03-06 14:00 ` Sagi Grimberg
2023-03-09 9:42 ` Christoph Hellwig
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox