Re: [PATCH v2] nvmet: Handle a NULL admin queue in nvmet_queue_tls_keyid()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] nvmet: UAF race in nvmet_queue_tls_keyid() during admin queue teardown
--

commit 7f7e86c9399b0b51dffaf7a9659e2eee6e379123
Author: Guixin Liu <kanie@linux.alibaba.com>

nvmet: Handle a NULL admin queue in nvmet_queue_tls_keyid()

This commit adds a NULL pointer check for sq in nvmet_queue_tls_keyid() to
avoid dereferencing a NULL admin queue when the tls_key debugfs entry is read
before the admin queue is created or after it is destroyed.

> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index 9cec19e6f4857..8bfc953b69f24 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -959,6 +959,8 @@ static inline void nvmet_req_bio_put(struct nvmet_req *req, struct bio *bio)
>  #ifdef CONFIG_NVME_TARGET_TCP_TLS
>  static inline key_serial_t nvmet_queue_tls_keyid(struct nvmet_sq *sq)
>  {
> +	if (!sq)
> +		return 0;
>  	return sq->tls_key ? key_serial(sq->tls_key) : 0;
>  }

[Severity: High]
Does checking for a NULL sq here actually prevent a use-after-free when
nvmet_ctrl_tls_key_show() races with admin queue teardown?

In drivers/nvme/target/debugfs.c, nvmet_ctrl_tls_key_show() reads
ctrl->sqs[0] without holding any lock or reference pinning the queue or its
key:

static int nvmet_ctrl_tls_key_show(struct seq_file *m, void *p)
{
	struct nvmet_ctrl *ctrl = m->private;
	key_serial_t keyid = nvmet_queue_tls_keyid(ctrl->sqs[0]);

	seq_printf(m, "%08x\n", keyid);
	return 0;
}

Concurrently, during admin queue teardown in drivers/nvme/target/tcp.c,
nvmet_tcp_release_queue_work() releases the TLS key, destroys the sq, and
frees the queue:

	nvmet_sq_put_tls_key(&queue->nvme_sq);
	nvmet_tcp_uninit_data_in_cmds(queue);
	nvmet_sq_destroy(&queue->nvme_sq);
	nvmet_cq_put(&queue->nvme_cq);
	...
	kfree(queue);

where nvmet_sq_put_tls_key() drops the key reference before clearing
sq->tls_key:

static inline void nvmet_sq_put_tls_key(struct nvmet_sq *sq)
{
	if (sq->tls_key) {
		key_put(sq->tls_key);
		sq->tls_key = NULL;
	}
}

and nvmet_sq_destroy() in drivers/nvme/target/core.c only clears
sq->ctrl->sqs[sq->qid] afterwards:

	if (ctrl) {
		ctrl->reset_tbkas = true;
		sq->ctrl->sqs[sq->qid] = NULL;
		nvmet_ctrl_put(ctrl);
		sq->ctrl = NULL; /* allows reusing the queue later */
	}

Can this still lead to two use-after-free races when reading the tls_key
debugfs entry while the controller stays alive via other references on
ctrl->ref (such as I/O queues or the keep-alive timer)?

1. nvmet_sq_put_tls_key() runs before nvmet_sq_destroy() clears
   ctrl->sqs[0], and calls key_put(sq->tls_key) before setting
   sq->tls_key = NULL. If nvmet_ctrl_tls_key_show() runs concurrently,
   sq is still non-NULL, so can nvmet_queue_tls_keyid() dereference
   sq->tls_key in key_serial(sq->tls_key) while or after key_put() frees
   the struct key?

2. If nvmet_ctrl_tls_key_show() loads a non-NULL sq = ctrl->sqs[0] just
   before nvmet_sq_destroy() sets sq->ctrl->sqs[0] = NULL and
   nvmet_tcp_release_queue_work() calls kfree(queue), the !sq check in
   nvmet_queue_tls_keyid() will still see the non-NULL pointer and
   dereference sq->tls_key on the freed queue.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029034306.3626274-1-kanie@linux.alibaba.com?part=1