Re: [PATCH 2/2] nvme-tcp: validate the TLS handshake timeout

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] nvme-tcp: truncation of remaining jiffies to int in nvme_tcp_start_tls()
--

commit 078728fd450c0f2c7191dfe3e169bb181ef6783e
Author: Yingjie Wang <1075151112@qq.com>

nvme-tcp: validate the TLS handshake timeout

This commit changes tls_handshake_timeout to an unsigned int and validates
that its value fits within both tls_handshake_args::ta_timeout_ms and
MAX_JIFFY_OFFSET / HZ. It also snapshots tls_handshake_timeout with
READ_ONCE() in nvme_tcp_start_tls() and widens tmo to unsigned long.

> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 7190eb10c2481..bc7a60ecfa356 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c

[ ... ]

> @@ -48,9 +49,24 @@ MODULE_PARM_DESC(wq_unbound, "Use unbound workqueue for nvme-tcp IO context (def
>  /*
>   * TLS handshake timeout
>   */
> -static int tls_handshake_timeout = 10;
> +static unsigned int tls_handshake_timeout = 10;
>  #ifdef CONFIG_NVME_TCP_TLS
> -module_param(tls_handshake_timeout, int, 0644);
> +static int tls_handshake_timeout_set(const char *val,
> +				     const struct kernel_param *kp)
> +{
> +	return param_set_uint_minmax(val, kp, 0,
> +		min_t(unsigned long, UINT_MAX / MSEC_PER_SEC,
> +		      MAX_JIFFY_OFFSET / HZ));
> +}

[ ... ]

> @@ -1796,7 +1812,8 @@ static int nvme_tcp_start_tls(struct nvme_ctrl *nctrl,
>  	int qid = nvme_tcp_queue_id(queue);
>  	int ret;
>  	struct tls_handshake_args args;
> -	unsigned long tmo = tls_handshake_timeout * HZ;
> +	unsigned int timeout = READ_ONCE(tls_handshake_timeout);
> +	unsigned long tmo = (unsigned long)timeout * HZ;
>  	key_serial_t keyring = nvme_keyring_id();

[Severity: High]
When tmo exceeds INT_MAX, can wait_for_completion_interruptible_timeout()
truncate the remaining jiffies into int ret and treat a completed TLS
handshake as a failure?

In tls_handshake_timeout_set(), values up to UINT_MAX / MSEC_PER_SEC
(4,294,967 seconds on 64-bit systems) are accepted. With HZ=1000 and
tls_handshake_timeout set to a value in [2147484, 4294967] (including
2147484 from the commit message), tmo = (unsigned long)timeout * HZ is in
[2147484000, 4294967000], which exceeds INT_MAX.

Later in nvme_tcp_start_tls(), the signed long return value from
wait_for_completion_interruptible_timeout() is stored in int ret:

drivers/nvme/host/tcp.c:nvme_tcp_start_tls() {
	...
	ret = wait_for_completion_interruptible_timeout(&queue->tls_complete, tmo);
	if (ret <= 0) {
		if (ret == 0)
			ret = -ETIMEDOUT;

		dev_err(nctrl->device,
			"queue %d: TLS handshake failed, error %d\n",
			qid, ret);
		tls_handshake_cancel(queue->sock->sk);
	} else {
	...
	return ret;
}

When the handshake completes normally while more than INT_MAX jiffies
remain, assigning the positive long return value to int ret truncates it to
a negative int (for example, around -2147483296). That causes ret <= 0 to
trigger, cancelling the handshake and returning a large negative value
smaller than -MAX_ERRNO.

Through the call chain:

  nvmf_dev_write()
    -> nvmf_create_ctrl()
      -> nvme_tcp_create_ctrl()
        -> nvme_tcp_setup_ctrl()
          -> nvme_tcp_configure_admin_queue()
            -> nvme_tcp_alloc_admin_queue()
              -> nvme_tcp_alloc_queue()
                -> nvme_tcp_start_tls()

nvme_tcp_create_ctrl() then converts that truncated ret via ERR_PTR(ret):

drivers/nvme/host/tcp.c:nvme_tcp_create_ctrl() {
	...
	if (ret > 0)
		ret = -EIO;
	return ERR_PTR(ret);
}

Because ret is less than -MAX_ERRNO, IS_ERR() in nvmf_create_ctrl() and
nvmf_dev_write() evaluates to false on the freed controller pointer, storing
an invalid pointer in seq_file->private that is dereferenced on subsequent
read or release of /dev/nvme-fabrics.

Should the wait return value in nvme_tcp_start_tls() be stored in a separate
long variable (or should ret be widened to long across the wait check)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_39AF670751CA25EA49D4BFD8E76C509F5506@qq.com?part=2