* [PATCH 2/2] nvmet: Fix controller use after free
2020-02-04 12:38 [PATCH 1/2] nvmet: Fix error print message at nvmet_install_queue function Israel Rukshin
@ 2020-02-04 12:38 ` Israel Rukshin
2020-02-04 14:49 ` Nadolski, Edmund
` (2 more replies)
2020-02-04 14:48 ` [PATCH 1/2] nvmet: Fix error print message at nvmet_install_queue function Nadolski, Edmund
` (2 subsequent siblings)
3 siblings, 3 replies; 8+ messages in thread
From: Israel Rukshin @ 2020-02-04 12:38 UTC (permalink / raw)
To: Linux-nvme, Sagi Grimberg, Christoph Hellwig; +Cc: Israel Rukshin, Max Gurtovoy
After nvmet_install_queue() sets sq->ctrl calling to nvmet_sq_destroy()
reduces the controller refcount. In case nvmet_install_queue() fails,
calling to nvmet_ctrl_put() is done twice (at nvmet_sq_destroy and
nvmet_execute_io_connect/nvmet_execute_admin_connect) instead of once for
the queue which leads to use after free of the controller. Fix this by set
NULL at sq->ctrl in case of a failure at nvmet_install_queue().
The bug leads to the following Call Trace:
[65857.994862] refcount_t: underflow; use-after-free.
[65858.108304] Workqueue: events nvmet_rdma_release_queue_work [nvmet_rdma]
[65858.115557] RIP: 0010:refcount_warn_saturate+0xe5/0xf0
[65858.208141] Call Trace:
[65858.211203] nvmet_sq_destroy+0xe1/0xf0 [nvmet]
[65858.216383] nvmet_rdma_release_queue_work+0x37/0xf0 [nvmet_rdma]
[65858.223117] process_one_work+0x167/0x370
[65858.227776] worker_thread+0x49/0x3e0
[65858.232089] kthread+0xf5/0x130
[65858.235895] ? max_active_store+0x80/0x80
[65858.240504] ? kthread_bind+0x10/0x10
[65858.244832] ret_from_fork+0x1f/0x30
[65858.249074] ---[ end trace f82d59250b54beb7 ]---
Fixes: bb1cc74790eb ("nvmet: implement valid sqhd values in completions")
Fixes: 1672ddb8d691 ("nvmet: Add install_queue callout")
Signed-off-by: Israel Rukshin <israelr@mellanox.com>
Reviewed-by: Max Gurtovoy <maxg@mellanox.com>
---
drivers/nvme/target/fabrics-cmd.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
index 53ef507..799de18 100644
--- a/drivers/nvme/target/fabrics-cmd.c
+++ b/drivers/nvme/target/fabrics-cmd.c
@@ -109,6 +109,7 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
u16 qid = le16_to_cpu(c->qid);
u16 sqsize = le16_to_cpu(c->sqsize);
struct nvmet_ctrl *old;
+ u16 ret;
old = cmpxchg(&req->sq->ctrl, NULL, ctrl);
if (old) {
@@ -119,7 +120,8 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
if (!sqsize) {
pr_warn("queue size zero!\n");
req->error_loc = offsetof(struct nvmf_connect_command, sqsize);
- return NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR;
+ ret = NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR;
+ goto err;
}
/* note: convert queue size from 0's-based value to 1's-based value */
@@ -132,16 +134,19 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
}
if (ctrl->ops->install_queue) {
- u16 ret = ctrl->ops->install_queue(req->sq);
-
+ ret = ctrl->ops->install_queue(req->sq);
if (ret) {
pr_err("failed to install queue %d cntlid %d ret %x\n",
qid, ctrl->cntlid, ret);
- return ret;
+ goto err;
}
}
return 0;
+
+err:
+ req->sq->ctrl = NULL;
+ return ret;
}
static void nvmet_execute_admin_connect(struct nvmet_req *req)
--
1.8.3.1
_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 2/2] nvmet: Fix controller use after free
2020-02-04 12:38 ` [PATCH 2/2] nvmet: Fix controller use after free Israel Rukshin
@ 2020-02-04 14:49 ` Nadolski, Edmund
2020-02-04 16:03 ` Christoph Hellwig
2020-02-07 20:03 ` Sagi Grimberg
2 siblings, 0 replies; 8+ messages in thread
From: Nadolski, Edmund @ 2020-02-04 14:49 UTC (permalink / raw)
To: Israel Rukshin, Linux-nvme, Sagi Grimberg, Christoph Hellwig; +Cc: Max Gurtovoy
On 2/4/2020 5:38 AM, Israel Rukshin wrote:
> After nvmet_install_queue() sets sq->ctrl calling to nvmet_sq_destroy()
> reduces the controller refcount. In case nvmet_install_queue() fails,
> calling to nvmet_ctrl_put() is done twice (at nvmet_sq_destroy and
> nvmet_execute_io_connect/nvmet_execute_admin_connect) instead of once for
> the queue which leads to use after free of the controller. Fix this by set
> NULL at sq->ctrl in case of a failure at nvmet_install_queue().
>
> The bug leads to the following Call Trace:
>
> [65857.994862] refcount_t: underflow; use-after-free.
> [65858.108304] Workqueue: events nvmet_rdma_release_queue_work [nvmet_rdma]
> [65858.115557] RIP: 0010:refcount_warn_saturate+0xe5/0xf0
> [65858.208141] Call Trace:
> [65858.211203] nvmet_sq_destroy+0xe1/0xf0 [nvmet]
> [65858.216383] nvmet_rdma_release_queue_work+0x37/0xf0 [nvmet_rdma]
> [65858.223117] process_one_work+0x167/0x370
> [65858.227776] worker_thread+0x49/0x3e0
> [65858.232089] kthread+0xf5/0x130
> [65858.235895] ? max_active_store+0x80/0x80
> [65858.240504] ? kthread_bind+0x10/0x10
> [65858.244832] ret_from_fork+0x1f/0x30
> [65858.249074] ---[ end trace f82d59250b54beb7 ]---
>
> Fixes: bb1cc74790eb ("nvmet: implement valid sqhd values in completions")
> Fixes: 1672ddb8d691 ("nvmet: Add install_queue callout")
> Signed-off-by: Israel Rukshin <israelr@mellanox.com>
> Reviewed-by: Max Gurtovoy <maxg@mellanox.com>
> ---
> drivers/nvme/target/fabrics-cmd.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
> index 53ef507..799de18 100644
> --- a/drivers/nvme/target/fabrics-cmd.c
> +++ b/drivers/nvme/target/fabrics-cmd.c
> @@ -109,6 +109,7 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
> u16 qid = le16_to_cpu(c->qid);
> u16 sqsize = le16_to_cpu(c->sqsize);
> struct nvmet_ctrl *old;
> + u16 ret;
>
> old = cmpxchg(&req->sq->ctrl, NULL, ctrl);
> if (old) {
> @@ -119,7 +120,8 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
> if (!sqsize) {
> pr_warn("queue size zero!\n");
> req->error_loc = offsetof(struct nvmf_connect_command, sqsize);
> - return NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR;
> + ret = NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR;
> + goto err;
> }
>
> /* note: convert queue size from 0's-based value to 1's-based value */
> @@ -132,16 +134,19 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
> }
>
> if (ctrl->ops->install_queue) {
> - u16 ret = ctrl->ops->install_queue(req->sq);
> -
> + ret = ctrl->ops->install_queue(req->sq);
> if (ret) {
> pr_err("failed to install queue %d cntlid %d ret %x\n",
> qid, ctrl->cntlid, ret);
> - return ret;
> + goto err;
> }
> }
>
> return 0;
> +
> +err:
> + req->sq->ctrl = NULL;
> + return ret;
> }
>
> static void nvmet_execute_admin_connect(struct nvmet_req *req)
>
Looks good.
Ed
_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 2/2] nvmet: Fix controller use after free
2020-02-04 12:38 ` [PATCH 2/2] nvmet: Fix controller use after free Israel Rukshin
2020-02-04 14:49 ` Nadolski, Edmund
@ 2020-02-04 16:03 ` Christoph Hellwig
2020-02-07 20:03 ` Sagi Grimberg
2 siblings, 0 replies; 8+ messages in thread
From: Christoph Hellwig @ 2020-02-04 16:03 UTC (permalink / raw)
To: Israel Rukshin; +Cc: Max Gurtovoy, Sagi Grimberg, Linux-nvme, Christoph Hellwig
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] nvmet: Fix controller use after free
2020-02-04 12:38 ` [PATCH 2/2] nvmet: Fix controller use after free Israel Rukshin
2020-02-04 14:49 ` Nadolski, Edmund
2020-02-04 16:03 ` Christoph Hellwig
@ 2020-02-07 20:03 ` Sagi Grimberg
2 siblings, 0 replies; 8+ messages in thread
From: Sagi Grimberg @ 2020-02-07 20:03 UTC (permalink / raw)
To: Israel Rukshin, Linux-nvme, Christoph Hellwig; +Cc: Max Gurtovoy
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] nvmet: Fix error print message at nvmet_install_queue function
2020-02-04 12:38 [PATCH 1/2] nvmet: Fix error print message at nvmet_install_queue function Israel Rukshin
2020-02-04 12:38 ` [PATCH 2/2] nvmet: Fix controller use after free Israel Rukshin
@ 2020-02-04 14:48 ` Nadolski, Edmund
2020-02-04 15:59 ` Christoph Hellwig
2020-02-07 20:03 ` Sagi Grimberg
3 siblings, 0 replies; 8+ messages in thread
From: Nadolski, Edmund @ 2020-02-04 14:48 UTC (permalink / raw)
To: Israel Rukshin, Linux-nvme, Sagi Grimberg, Christoph Hellwig; +Cc: Max Gurtovoy
On 2/4/2020 5:38 AM, Israel Rukshin wrote:
> Place the arguments in the correct order.
>
> Fixes: 1672ddb8d691 ("nvmet: Add install_queue callout")
> Signed-off-by: Israel Rukshin <israelr@mellanox.com>
> Reviewed-by: Max Gurtovoy <maxg@mellanox.com>
> ---
> drivers/nvme/target/fabrics-cmd.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
> index 7211996..53ef507 100644
> --- a/drivers/nvme/target/fabrics-cmd.c
> +++ b/drivers/nvme/target/fabrics-cmd.c
> @@ -136,7 +136,7 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
>
> if (ret) {
> pr_err("failed to install queue %d cntlid %d ret %x\n",
> - qid, ret, ctrl->cntlid);
> + qid, ctrl->cntlid, ret);
> return ret;
> }
> }
>
Looks good.
Ed
_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/2] nvmet: Fix error print message at nvmet_install_queue function
2020-02-04 12:38 [PATCH 1/2] nvmet: Fix error print message at nvmet_install_queue function Israel Rukshin
2020-02-04 12:38 ` [PATCH 2/2] nvmet: Fix controller use after free Israel Rukshin
2020-02-04 14:48 ` [PATCH 1/2] nvmet: Fix error print message at nvmet_install_queue function Nadolski, Edmund
@ 2020-02-04 15:59 ` Christoph Hellwig
2020-02-07 20:03 ` Sagi Grimberg
3 siblings, 0 replies; 8+ messages in thread
From: Christoph Hellwig @ 2020-02-04 15:59 UTC (permalink / raw)
To: Israel Rukshin; +Cc: Max Gurtovoy, Sagi Grimberg, Linux-nvme, Christoph Hellwig
On Tue, Feb 04, 2020 at 02:38:09PM +0200, Israel Rukshin wrote:
> Place the arguments in the correct order.
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] nvmet: Fix error print message at nvmet_install_queue function
2020-02-04 12:38 [PATCH 1/2] nvmet: Fix error print message at nvmet_install_queue function Israel Rukshin
` (2 preceding siblings ...)
2020-02-04 15:59 ` Christoph Hellwig
@ 2020-02-07 20:03 ` Sagi Grimberg
3 siblings, 0 replies; 8+ messages in thread
From: Sagi Grimberg @ 2020-02-07 20:03 UTC (permalink / raw)
To: Israel Rukshin, Linux-nvme, Christoph Hellwig; +Cc: Max Gurtovoy
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 8+ messages in thread