[PATCH 1/2] nvmet-tcp: validate the TLS handshake timeout

[PATCH 1/2] nvmet-tcp: validate the TLS handshake timeout

[Yingjie Wang]

tls_handshake_timeout is writable but stored as a signed int. Multiplying
the value by MSEC_PER_SEC and HZ therefore uses signed arithmetic.

With HZ=1000, setting it to 2147484 overflows both multiplications. If
the handshake request is accepted and remains pending, the timer wheel
treats the jiffies delay as already expired and queues the timeout work
on its next advance, while the handshake agent was given about 24.9 days.
Negative values such as -1 cause the same immediate kernel-side timeout.

Store the parameter as an unsigned int and reject values that cannot be
represented both by tls_handshake_args::ta_timeout_ms and by a safe
jiffies delay. Take one snapshot so a concurrent parameter update cannot
give the handshake agent and delayed work different timeouts.

Negative and oversized values now fail with -EINVAL. Keep zero valid,
preserving the existing immediate-timeout behavior.

Fixes: 675b453e0241 ("nvmet-tcp: enable TLS handshake upcall")
Cc: stable@vger.kernel.org
Signed-off-by: Yingjie Wang <1075151112@qq.com>
---
 drivers/nvme/target/tcp.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index e598101752620..7937377436d3b 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -6,6 +6,7 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 #include <linux/module.h>
 #include <linux/init.h>
+#include <linux/jiffies.h>
 #include <linux/slab.h>
 #include <linux/crc32c.h>
 #include <linux/err.h>
@@ -76,8 +77,24 @@ MODULE_PARM_DESC(idle_poll_period_usecs,
 /*
  * TLS handshake timeout
  */
-static int tls_handshake_timeout = 10;
-module_param(tls_handshake_timeout, int, 0644);
+static unsigned int tls_handshake_timeout = 10;
+
+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));
+}
+
+static const struct kernel_param_ops tls_handshake_timeout_ops = {
+	.set = tls_handshake_timeout_set,
+	.get = param_get_uint,
+};
+
+module_param_cb(tls_handshake_timeout, &tls_handshake_timeout_ops,
+		&tls_handshake_timeout, 0644);
+__MODULE_PARM_TYPE(tls_handshake_timeout, "uint");
 MODULE_PARM_DESC(tls_handshake_timeout,
 		 "nvme TLS handshake timeout in seconds (default 10)");
 #endif
@@ -1904,6 +1921,7 @@ static int nvmet_tcp_tls_handshake(struct nvmet_tcp_queue *queue)
 {
 	int ret = -EOPNOTSUPP;
 	struct tls_handshake_args args;
+	unsigned int timeout = READ_ONCE(tls_handshake_timeout);
 
 	if (queue->state != NVMET_TCP_Q_TLS_HANDSHAKE) {
 		pr_warn("cannot start TLS in state %d\n", queue->state);
@@ -1917,7 +1935,7 @@ static int nvmet_tcp_tls_handshake(struct nvmet_tcp_queue *queue)
 	args.ta_done = nvmet_tcp_tls_handshake_done;
 	args.ta_data = queue;
 	args.ta_keyring = key_serial(queue->port->nport->keyring);
-	args.ta_timeout_ms = tls_handshake_timeout * 1000;
+	args.ta_timeout_ms = timeout * MSEC_PER_SEC;
 
 	ret = tls_server_hello_psk(&args, GFP_KERNEL);
 	if (ret) {
@@ -1925,7 +1943,7 @@ static int nvmet_tcp_tls_handshake(struct nvmet_tcp_queue *queue)
 		pr_err("failed to start TLS, err=%d\n", ret);
 	} else {
 		queue_delayed_work(nvmet_wq, &queue->tls_handshake_tmo_work,
-				   tls_handshake_timeout * HZ);
+				   (unsigned long)timeout * HZ);
 	}
 	return ret;
 }
-- 
2.43.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] nvmet-tcp: missing kernel-parameters.txt entry for tls_handshake_timeout
--

commit 88d44e836ad9d55aee22edbc907b852e3d9a66c6
Author: Yingjie Wang <1075151112@qq.com>

nvmet-tcp: validate the TLS handshake timeout

This commit changes the tls_handshake_timeout module parameter from a signed
int to an unsigned int and validates that configured values fit within both
tls_handshake_args::ta_timeout_ms and a valid jiffies delay. It also reads
the timeout once per handshake so the upcall and delayed work use the same
value.

> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
> index c38ea3501ea5d..ba529cfc62577 100644
> --- a/drivers/nvme/target/tcp.c
> +++ b/drivers/nvme/target/tcp.c

[ ... ]

> @@ -76,8 +77,24 @@ MODULE_PARM_DESC(idle_poll_period_usecs,
>  /*
>   * TLS handshake timeout
>   */
> -static int tls_handshake_timeout = 10;
> -module_param(tls_handshake_timeout, int, 0644);
> +static unsigned int tls_handshake_timeout = 10;
> +
> +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));
> +}
> +
> +static const struct kernel_param_ops tls_handshake_timeout_ops = {
> +	.set = tls_handshake_timeout_set,
> +	.get = param_get_uint,
> +};
> +
> +module_param_cb(tls_handshake_timeout, &tls_handshake_timeout_ops,
> +		&tls_handshake_timeout, 0644);
> +__MODULE_PARM_TYPE(tls_handshake_timeout, "uint");

[Severity: Low]
In drivers/nvme/target/tcp.c, this changes the tls_handshake_timeout module
parameter definition from module_param() (int) to module_param_cb() (uint)
with range bounds checked by tls_handshake_timeout_set().

Since nvmet_tcp.tls_handshake_timeout is exposed on the kernel command line
when CONFIG_NVME_TARGET_TCP_TLS is enabled, should an entry also be added to
Documentation/admin-guide/kernel-parameters.txt documenting its format and
valid range?

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

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

[Yingjie Wang]

tls_handshake_timeout is writable but stored as a signed int. Multiplying
the value by MSEC_PER_SEC and HZ therefore uses signed arithmetic.

With HZ=1000, setting it to 2147484 overflows both multiplications. If
the handshake request is accepted and its completion remains pending,
schedule_timeout() reports a wrong timeout value with a stack dump and
returns zero. The handshake then fails immediately with -ETIMEDOUT, while
the handshake agent was given about 24.9 days. Negative values such as -1
have the same kernel-side effect.

Store the parameter as an unsigned int and reject values that cannot be
represented both by tls_handshake_args::ta_timeout_ms and by a safe
jiffies delay. Take one snapshot so a concurrent parameter update cannot
give the handshake agent and completion wait different timeouts.

Negative and oversized values now fail with -EINVAL. Keep zero valid,
preserving the existing immediate-timeout behavior.

Fixes: be8e82caa685 ("nvme-tcp: enable TLS handshake upcall")
Cc: stable@vger.kernel.org
Signed-off-by: Yingjie Wang <1075151112@qq.com>
---
 drivers/nvme/host/tcp.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index f876375dffd21..ac76d333ddceb 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -6,6 +6,7 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 #include <linux/module.h>
 #include <linux/init.h>
+#include <linux/jiffies.h>
 #include <linux/slab.h>
 #include <linux/async.h>
 #include <linux/err.h>
@@ -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));
+}
+
+static const struct kernel_param_ops tls_handshake_timeout_ops = {
+	.set = tls_handshake_timeout_set,
+	.get = param_get_uint,
+};
+
+module_param_cb(tls_handshake_timeout, &tls_handshake_timeout_ops,
+		&tls_handshake_timeout, 0644);
+__MODULE_PARM_TYPE(tls_handshake_timeout, "uint");
 MODULE_PARM_DESC(tls_handshake_timeout,
 		 "nvme TLS handshake timeout in seconds (default 10)");
 #endif
@@ -1800,7 +1816,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();
 
 	dev_dbg(nctrl->device, "queue %d: start TLS with key %x\n",
@@ -1814,7 +1831,7 @@ static int nvme_tcp_start_tls(struct nvme_ctrl *nctrl,
 	if (nctrl->opts->keyring)
 		keyring = key_serial(nctrl->opts->keyring);
 	args.ta_keyring = keyring;
-	args.ta_timeout_ms = tls_handshake_timeout * 1000;
+	args.ta_timeout_ms = timeout * MSEC_PER_SEC;
 	queue->tls_err = -EOPNOTSUPP;
 	init_completion(&queue->tls_complete);
 	ret = tls_client_hello_psk(&args, GFP_KERNEL);
-- 
2.43.0

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

[sashiko-bot]

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