[PATCH] nvmet: Check the admin queue before reading its TLS key in debugfs

[PATCH] nvmet: Check the admin queue before reading its TLS key in debugfs

[Guixin Liu]

nvmet_ctrl_tls_key_show() dereferences ctrl->sqs[0] unconditionally,
but the admin queue is not installed yet while the controller is
being created, and sqs[0] is set to NULL again when it is destroyed.
Reading the tls_key debugfs entry in either window crashes the kernel.

Only query the queue's TLS key when the admin queue is present and
report 0 otherwise.

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>
---
 drivers/nvme/target/debugfs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/debugfs.c b/drivers/nvme/target/debugfs.c
index e85fe1d4c9f8..a27a501fe61a 100644
--- a/drivers/nvme/target/debugfs.c
+++ b/drivers/nvme/target/debugfs.c
@@ -136,8 +136,10 @@ 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]);
+	key_serial_t keyid = 0;
 
+	if (ctrl->sqs[0])
+		keyid = nvmet_queue_tls_keyid(ctrl->sqs[0]);
 	seq_printf(m, "%08x\n", keyid);
 	return 0;
 }
-- 
2.43.7

Re: [PATCH] nvmet: Check the admin queue before reading its TLS key in debugfs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] nvmet: unsynchronized ctrl->sqs[0] access in nvmet_ctrl_tls_key_show()
--

commit 3cf90339b7ea83703b0eb52aef8d8c4af5c44e19
Author: Guixin Liu <kanie@linux.alibaba.com>

nvmet: Check the admin queue before reading its TLS key in debugfs

This commit checks whether ctrl->sqs[0] is non-NULL in
nvmet_ctrl_tls_key_show() before querying the admin queue's TLS key ID for
debugfs output.

> diff --git a/drivers/nvme/target/debugfs.c b/drivers/nvme/target/debugfs.c
> index e85fe1d4c9f8d..a27a501fe61a4 100644
> --- a/drivers/nvme/target/debugfs.c
> +++ b/drivers/nvme/target/debugfs.c
> @@ -136,8 +136,10 @@ 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]);
> +	key_serial_t keyid = 0;
>  
> +	if (ctrl->sqs[0])
> +		keyid = nvmet_queue_tls_keyid(ctrl->sqs[0]);

[Severity: High]
Can this unlocked check and reload of ctrl->sqs[0] race with admin queue
teardown in nvmet_tcp_release_queue_work() and nvmet_sq_destroy(), or with
the error path in nvmet_install_queue()?

First, ctrl->sqs[0] is checked and then reloaded without READ_ONCE() or any
lock synchronizing against nvmet_sq_destroy() in drivers/nvme/target/core.c:

    if (sq->qid == 0) {
        ctrl->reset_tbkas = true;
        sq->ctrl->sqs[sq->qid] = NULL;
        nvmet_ctrl_put(ctrl);
    }

If ctrl->sqs[0] is cleared to NULL between the if (ctrl->sqs[0]) check and
the argument evaluation for nvmet_queue_tls_keyid(ctrl->sqs[0]), NULL is
passed into nvmet_queue_tls_keyid() in drivers/nvme/target/nvmet.h:

    static inline key_serial_t nvmet_queue_tls_keyid(struct nvmet_sq *sq)
    {
        return sq->tls_key ? key_serial(sq->tls_key) : 0;
    }

which dereferences sq->tls_key on a NULL pointer.

Second, even when ctrl->sqs[0] is read as non-NULL, nothing pins the queue or
sq->tls_key while nvmet_ctrl_tls_key_show() runs. During queue teardown in
drivers/nvme/target/tcp.c, nvmet_tcp_release_queue_work() executes:

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

where nvmet_sq_put_tls_key() in drivers/nvme/target/nvmet.h drops the key
reference before ctrl->sqs[0] is cleared:

    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;
        }
    }

Because the controller debugfs directory stays alive until the final
ctrl->ref is dropped in nvmet_ctrl_free() -> nvmet_debugfs_ctrl_free(ctrl)
(for example, while queue->nvme_cq or I/O queues still hold controller
references), a concurrent reader of the tls_key debugfs file can dereference
sq->tls_key after key_put() or dereference sq after kfree(queue).

Could this be avoided by reading the key directly from ctrl->tls_key (or
caching the key serial on struct nvmet_ctrl), or by properly serializing
access to the queue and its key?

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

Re: [PATCH] nvmet: Check the admin queue before reading its TLS key in debugfs

[Hannes Reinecke]

On 10/28/26 7:44 AM, Guixin Liu wrote:
> nvmet_ctrl_tls_key_show() dereferences ctrl->sqs[0] unconditionally,
> but the admin queue is not installed yet while the controller is
> being created, and sqs[0] is set to NULL again when it is destroyed.
> Reading the tls_key debugfs entry in either window crashes the kernel.
> 
> Only query the queue's TLS key when the admin queue is present and
> report 0 otherwise.
> 
> 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>
> ---
>   drivers/nvme/target/debugfs.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/target/debugfs.c b/drivers/nvme/target/debugfs.c
> index e85fe1d4c9f8..a27a501fe61a 100644
> --- a/drivers/nvme/target/debugfs.c
> +++ b/drivers/nvme/target/debugfs.c
> @@ -136,8 +136,10 @@ 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]);
> +	key_serial_t keyid = 0;
>   
> +	if (ctrl->sqs[0])
> +		keyid = nvmet_queue_tls_keyid(ctrl->sqs[0]);
>   	seq_printf(m, "%08x\n", keyid);
>   	return 0;
>   }

I think it might be easier if 'nvmet_queue_tls_keyid()' would accept
a NULL argument.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH] nvmet: Check the admin queue before reading its TLS key in debugfs

[Guixin Liu]

在 2026/10/28 15:42, Hannes Reinecke 写道:
> On 10/28/26 7:44 AM, Guixin Liu wrote:
>> nvmet_ctrl_tls_key_show() dereferences ctrl->sqs[0] unconditionally,
>> but the admin queue is not installed yet while the controller is
>> being created, and sqs[0] is set to NULL again when it is destroyed.
>> Reading the tls_key debugfs entry in either window crashes the kernel.
>>
>> Only query the queue's TLS key when the admin queue is present and
>> report 0 otherwise.
>>
>> 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>
>> ---
>>   drivers/nvme/target/debugfs.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvme/target/debugfs.c b/drivers/nvme/target/debugfs.c
>> index e85fe1d4c9f8..a27a501fe61a 100644
>> --- a/drivers/nvme/target/debugfs.c
>> +++ b/drivers/nvme/target/debugfs.c
>> @@ -136,8 +136,10 @@ 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]);
>> +    key_serial_t keyid = 0;
>>   +    if (ctrl->sqs[0])
>> +        keyid = nvmet_queue_tls_keyid(ctrl->sqs[0]);
>>       seq_printf(m, "%08x\n", keyid);
>>       return 0;
>>   }
>
> I think it might be easier if 'nvmet_queue_tls_keyid()' would accept
> a NULL argument.
This is simpler, however Sashiko's review points out that after the
NULL check, sq->tls_key clould be freed by nvmet_sq_put_tls_key(),
or sq itself could be destroyed. 

My idea is store admin queue's tls_key to nvmet_ctrl to prevent access
sq and sq->tls_key in debugfs interface, what do you think?

Best Regards,
Guixin Liu
>
> Cheers,
>
> Hannes

Re: [PATCH] nvmet: Check the admin queue before reading its TLS key in debugfs

[Hannes Reinecke]

On 10/28/26 9:24 AM, Guixin Liu wrote:
> 
> 
> 在 2026/10/28 15:42, Hannes Reinecke 写道:
>> On 10/28/26 7:44 AM, Guixin Liu wrote:
>>> nvmet_ctrl_tls_key_show() dereferences ctrl->sqs[0] unconditionally,
>>> but the admin queue is not installed yet while the controller is
>>> being created, and sqs[0] is set to NULL again when it is destroyed.
>>> Reading the tls_key debugfs entry in either window crashes the kernel.
>>>
>>> Only query the queue's TLS key when the admin queue is present and
>>> report 0 otherwise.
>>>
>>> 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>
>>> ---
>>>    drivers/nvme/target/debugfs.c | 4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/nvme/target/debugfs.c b/drivers/nvme/target/debugfs.c
>>> index e85fe1d4c9f8..a27a501fe61a 100644
>>> --- a/drivers/nvme/target/debugfs.c
>>> +++ b/drivers/nvme/target/debugfs.c
>>> @@ -136,8 +136,10 @@ 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]);
>>> +    key_serial_t keyid = 0;
>>>    +    if (ctrl->sqs[0])
>>> +        keyid = nvmet_queue_tls_keyid(ctrl->sqs[0]);
>>>        seq_printf(m, "%08x\n", keyid);
>>>        return 0;
>>>    }
>>
>> I think it might be easier if 'nvmet_queue_tls_keyid()' would accept
>> a NULL argument.
> This is simpler, however Sashiko's review points out that after the
> NULL check, sq->tls_key clould be freed by nvmet_sq_put_tls_key(),
> or sq itself could be destroyed.
> 
> My idea is store admin queue's tls_key to nvmet_ctrl to prevent access
> sq and sq->tls_key in debugfs interface, what do you think?
> 
Argl. Don't.

I would do something like

key_serial_t keyid = ctrl->sqs ? nvmet_queue_tls_keyid(ctrl->sqs[0]):0;

to make it clear that ctrl->sqs might be NULL here.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH] nvmet: Check the admin queue before reading its TLS key in debugfs

[Guixin Liu]

在 2026/10/28 22:48, Hannes Reinecke 写道:
> On 10/28/26 9:24 AM, Guixin Liu wrote:
>>
>>
>> 在 2026/10/28 15:42, Hannes Reinecke 写道:
>>> On 10/28/26 7:44 AM, Guixin Liu wrote:
>>>> nvmet_ctrl_tls_key_show() dereferences ctrl->sqs[0] unconditionally,
>>>> but the admin queue is not installed yet while the controller is
>>>> being created, and sqs[0] is set to NULL again when it is destroyed.
>>>> Reading the tls_key debugfs entry in either window crashes the kernel.
>>>>
>>>> Only query the queue's TLS key when the admin queue is present and
>>>> report 0 otherwise.
>>>>
>>>> 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>
>>>> ---
>>>>    drivers/nvme/target/debugfs.c | 4 +++-
>>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/nvme/target/debugfs.c b/drivers/nvme/target/debugfs.c
>>>> index e85fe1d4c9f8..a27a501fe61a 100644
>>>> --- a/drivers/nvme/target/debugfs.c
>>>> +++ b/drivers/nvme/target/debugfs.c
>>>> @@ -136,8 +136,10 @@ 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]);
>>>> +    key_serial_t keyid = 0;
>>>>    +    if (ctrl->sqs[0])
>>>> +        keyid = nvmet_queue_tls_keyid(ctrl->sqs[0]);
>>>>        seq_printf(m, "%08x\n", keyid);
>>>>        return 0;
>>>>    }
>>>
>>> I think it might be easier if 'nvmet_queue_tls_keyid()' would accept
>>> a NULL argument.
>> This is simpler, however Sashiko's review points out that after the
>> NULL check, sq->tls_key clould be freed by nvmet_sq_put_tls_key(),
>> or sq itself could be destroyed.
>>
>> My idea is store admin queue's tls_key to nvmet_ctrl to prevent access
>> sq and sq->tls_key in debugfs interface, what do you think?
>>
> Argl. Don't.
>
> I would do something like
>
> key_serial_t keyid = ctrl->sqs ? nvmet_queue_tls_keyid(ctrl->sqs[0]):0;
>
> to make it clear that ctrl->sqs might be NULL here.
>
> Cheers,
>
> Hannes
OK, I will send a v2 patch.

Best Regards,
Guixin Liu