Re: [PATCH 2/2] nvmet: Fix controller use after free

["Nadolski, Edmund" <edmund.nadolski@intel.com>]

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