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

[sashiko-bot@kernel.org]

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