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

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

[Guixin Liu]

The admin queue is not necessarily installed when the tls_key debugfs
entry is read: it may not have been created yet, or it may have been
destroyed already.  nvmet_queue_tls_keyid() then dereferences a NULL
sq.

Guard the helper and return 0 instead, which also covers the read
racing with admin queue teardown.

Fixes: 316dabe6089fe ("nvmet: add tls_concat and tls_key debugfs entries")
Cc: stable@vger.kernel.org
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---

Changes since v1:
- Simply return 0 when the admin queue is absent in
  nvmet_queue_tls_keyid() instead of dereferencing it, covering the
  not-yet-created and already-destroyed windows (Hannes Reinecke)
 drivers/nvme/target/nvmet.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index e315f1c24e73..4e988488497b 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -957,6 +957,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;
 }
 static inline void nvmet_sq_put_tls_key(struct nvmet_sq *sq)
-- 
2.43.7

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

[sashiko-bot]

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

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

[Christoph Hellwig]

On Thu, Oct 29, 2026 at 11:43:06AM +0800, Guixin Liu wrote:
> The admin queue is not necessarily installed when the tls_key debugfs
> entry is read: it may not have been created yet, or it may have been
> destroyed already.  nvmet_queue_tls_keyid() then dereferences a NULL
> sq.
> 
> Guard the helper and return 0 instead, which also covers the read
> racing with admin queue teardown.

The patch itself looks obviously fine, but do we have any synchronization
that ensure it does not go away while we're looking at the submission
queue?

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

[Guixin Liu]

在 2026/10/29 20:15, Christoph Hellwig 写道:
> On Thu, Oct 29, 2026 at 11:43:06AM +0800, Guixin Liu wrote:
>> The admin queue is not necessarily installed when the tls_key debugfs
>> entry is read: it may not have been created yet, or it may have been
>> destroyed already.  nvmet_queue_tls_keyid() then dereferences a NULL
>> sq.
>>
>> Guard the helper and return 0 instead, which also covers the read
>> racing with admin queue teardown.
> The patch itself looks obviously fine, but do we have any synchronization
> that ensure it does not go away while we're looking at the submission
> queue?
The nvmet_sq has percpu_ref, but it can keep nvmet_sq_destroy() from
freeing sq only, it can't keep nvmet_tcp_release_queue_work() from
setting tls_key to NULL, so we need serialize with nvmet_sq_put_tls_key():

"""""
diff --git a/drivers/nvme/target/debugfs.c b/drivers/nvme/target/debugfs.c
index e85fe1d4c9f8..cc451e25ea1d 100644
--- a/drivers/nvme/target/debugfs.c
+++ b/drivers/nvme/target/debugfs.c
@@ -136,8 +136,19 @@ NVMET_DEBUGFS_ATTR(nvmet_ctrl_host_traddr);
 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]);
-
+       struct nvmet_sq *sq = ctrl->sqs[0];
+       key_serial_t keyid = 0;
+
+       /*
+        * Pin the admin queue against teardown, and serialize with
+        * nvmet_sq_put_tls_key() so the key cannot be dropped mid-read.
+        */
+       if (sq && percpu_ref_tryget_live(&sq->ref)) {
+               mutex_lock(&ctrl->lock);
+               keyid = nvmet_queue_tls_keyid(sq);
+               mutex_unlock(&ctrl->lock);
+               percpu_ref_put(&sq->ref);
+       }
        seq_printf(m, "%08x\n", keyid);
        return 0;
 }
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index e315f1c24e73..24df9e4a1e1c 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -957,13 +957,17 @@ 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;
 }
 static inline void nvmet_sq_put_tls_key(struct nvmet_sq *sq)
 {
        if (sq->tls_key) {
+               mutex_lock(&sq->ctrl->lock);
                key_put(sq->tls_key);
                sq->tls_key = NULL;
+               mutex_unlock(&sq->ctrl->lock);
        }
 }
""""

Best Regards,
Guixin Liu

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

[Guixin Liu]

Hi Christoph,
  Any feedback?

Best Regards,
Guixin Liu

在 2026/10/30 10:36, Guixin Liu 写道:
>
> 在 2026/10/29 20:15, Christoph Hellwig 写道:
>> On Thu, Oct 29, 2026 at 11:43:06AM +0800, Guixin Liu wrote:
>>> The admin queue is not necessarily installed when the tls_key debugfs
>>> entry is read: it may not have been created yet, or it may have been
>>> destroyed already.  nvmet_queue_tls_keyid() then dereferences a NULL
>>> sq.
>>>
>>> Guard the helper and return 0 instead, which also covers the read
>>> racing with admin queue teardown.
>> The patch itself looks obviously fine, but do we have any synchronization
>> that ensure it does not go away while we're looking at the submission
>> queue?
> The nvmet_sq has percpu_ref, but it can keep nvmet_sq_destroy() from
> freeing sq only, it can't keep nvmet_tcp_release_queue_work() from
> setting tls_key to NULL, so we need serialize with nvmet_sq_put_tls_key():
>
> """""
> diff --git a/drivers/nvme/target/debugfs.c b/drivers/nvme/target/debugfs.c
> index e85fe1d4c9f8..cc451e25ea1d 100644
> --- a/drivers/nvme/target/debugfs.c
> +++ b/drivers/nvme/target/debugfs.c
> @@ -136,8 +136,19 @@ NVMET_DEBUGFS_ATTR(nvmet_ctrl_host_traddr);
>  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]);
> -
> +       struct nvmet_sq *sq = ctrl->sqs[0];
> +       key_serial_t keyid = 0;
> +
> +       /*
> +        * Pin the admin queue against teardown, and serialize with
> +        * nvmet_sq_put_tls_key() so the key cannot be dropped mid-read.
> +        */
> +       if (sq && percpu_ref_tryget_live(&sq->ref)) {
> +               mutex_lock(&ctrl->lock);
> +               keyid = nvmet_queue_tls_keyid(sq);
> +               mutex_unlock(&ctrl->lock);
> +               percpu_ref_put(&sq->ref);
> +       }
>         seq_printf(m, "%08x\n", keyid);
>         return 0;
>  }
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index e315f1c24e73..24df9e4a1e1c 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -957,13 +957,17 @@ 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;
>  }
>  static inline void nvmet_sq_put_tls_key(struct nvmet_sq *sq)
>  {
>         if (sq->tls_key) {
> +               mutex_lock(&sq->ctrl->lock);
>                 key_put(sq->tls_key);
>                 sq->tls_key = NULL;
> +               mutex_unlock(&sq->ctrl->lock);
>         }
>  }
> """"
>
> Best Regards,
> Guixin Liu

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

[Christoph Hellwig]

On Mon, Nov 09, 2026 at 10:35:35AM +0800, Guixin Liu wrote:
> Hi Christoph,
>   Any feedback?

I was waiting for a formal patch.  The change looks reasonable.