Skip to contentSkip to conversation

nvmet-tcp: validate the TLS handshake timeout

PATCH Superseded Yingjie Wang 3 participants 5 messages Oct 27 – Oct 28 1 message not in the archive

Subsystems (3)
Documentation NVMe NVMe target

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Yingjie Wang

author
#1 lore raw

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

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. Document the accepted range for built-in use.

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 [email protected]
Signed-off-by Yingjie Wang <[email protected]>
---
 .../admin-guide/kernel-parameters.txt         |  5 ++++
 drivers/nvme/target/tcp.c                     | 26 ++++++++++++++++---
 2 files changed, 27 insertions(+), 4 deletions(-)
Documentation/admin-guide/kernel-parameters.txt +5 -0
index 68647ff4bdd24..cc54d5f169f67 100644
@@ -4801,6 +4801,11 @@ Kernel parameters
 			Example:
 			nvme.quirks=7710:2267:bogus_nid,^identify_cns-9900:7711:broken_msi
 
+	nvmet_tcp.tls_handshake_timeout=
+			[NVME] TLS handshake timeout in seconds. Valid values
+			range from 0 to the smaller of UINT_MAX / 1000 and
+			MAX_JIFFY_OFFSET / HZ. Default: 10
+
 	ohci1394_dma=early	[HW,EARLY] enable debugging via the ohci1394 driver.
 			See Documentation/core-api/debugging-via-ohci1394.rst for more
 			info.
drivers/nvme/target/tcp.c +22 -4
index e598101752620..7937377436d3b 100644
@@ -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;
 }
Signature · 1 line
2.43.0
Reviewed-by on v2 1/2
#2 lore raw
Quoted 19 lines · 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. Document the accepted range for built-in use.

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

Quoted 8 lines
Fixes 675b453e0241 ("nvmet-tcp: enable TLS handshake upcall")
Cc [email protected]
Signed-off-by Yingjie Wang <[email protected]>
---
  .../admin-guide/kernel-parameters.txt         |  5 ++++
  drivers/nvme/target/tcp.c                     | 26 ++++++++++++++++---
  2 files changed, 27 insertions(+), 4 deletions(-)
Reviewed-by Hannes Reinecke <[email protected]>

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke                  Kernel Storage Architect
[email protected]                                +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
author
#3 lore raw

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

In reply to a message not in the archive

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. Document the accepted range for built-in use.

wait_for_completion_interruptible_timeout() returns the remaining jiffies as a long after a successful completion. Keep that result in a long because an accepted timeout can exceed INT_MAX jiffies on 64-bit systems.

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 [email protected]
Signed-off-by Yingjie Wang <[email protected]>
---
 .../admin-guide/kernel-parameters.txt         |  5 +++
 drivers/nvme/host/tcp.c                       | 34 ++++++++++++++-----
 2 files changed, 31 insertions(+), 8 deletions(-)
Documentation/admin-guide/kernel-parameters.txt +5 -0
index cc54d5f169f67..a5adbf58fb8f7 100644
@@ -4801,6 +4801,11 @@ Kernel parameters
 			Example:
 			nvme.quirks=7710:2267:bogus_nid,^identify_cns-9900:7711:broken_msi
 
+	nvme_tcp.tls_handshake_timeout=
+			[NVME] TLS handshake timeout in seconds. Valid values
+			range from 0 to the smaller of UINT_MAX / 1000 and
+			MAX_JIFFY_OFFSET / HZ. Default: 10
+
 	nvmet_tcp.tls_handshake_timeout=
 			[NVME] TLS handshake timeout in seconds. Valid values
 			range from 0 to the smaller of UINT_MAX / 1000 and
drivers/nvme/host/tcp.c +26 -8
index f876375dffd21..f0c18e1844eb9 100644
@@ -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

@@ -1799,8 +1815,10 @@ static int nvme_tcp_start_tls(struct nvme_ctrl *nctrl,
 {
 	int qid = nvme_tcp_queue_id(queue);
 	int ret;
+	long wait_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 +1832,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);

@@ -1823,10 +1841,10 @@ static int nvme_tcp_start_tls(struct nvme_ctrl *nctrl,
 			qid, ret);
 		return ret;
 	}
-	ret = wait_for_completion_interruptible_timeout(&queue->tls_complete, tmo);
-	if (ret <= 0) {
-		if (ret == 0)
-			ret = -ETIMEDOUT;
+	wait_ret = wait_for_completion_interruptible_timeout(&queue->tls_complete,
+							     tmo);
+	if (wait_ret <= 0) {
+		ret = wait_ret < 0 ? -ERESTARTSYS : -ETIMEDOUT;
 
 		dev_err(nctrl->device,
 			"queue %d: TLS handshake failed, error %d\n",
Signature · 1 line
2.43.0
reviewer
#4 lore raw

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

Documentation/admin-guide/kernel-parameters.txt Kernel parameters · line 4803

Quoted 8 lines · Yingjie Wang
Documentation/admin-guide/kernel-parameters.txt +0 -0
index cc54d5f169f67..a5adbf58fb8f7 100644
@@ -4801,6 +4801,11 @@ Kernel parameters
 			Example:
 			nvme.quirks=7710:2267:bogus_nid,^identify_cns-9900:7711:broken_msi

Maybe add "host" and "target" in the descriptions? or something to clarify/distinguish them.

Quoted 8 lines
Documentation/admin-guide/kernel-parameters.txt +5 -0
+	nvme_tcp.tls_handshake_timeout=
+			[NVME] TLS handshake timeout in seconds. Valid values
+			range from 0 to the smaller of UINT_MAX / 1000 and
+			MAX_JIFFY_OFFSET / HZ. Default: 10
+
 	nvmet_tcp.tls_handshake_timeout=
 			[NVME] TLS handshake timeout in seconds. Valid values
 			range from 0 to the smaller of UINT_MAX / 1000 and
Signature · 1 line
~Randy
Reviewed-by on v2 2/2
#5 lore raw

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

In reply to Yingjie Wang
Quoted 24 lines · 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. Document the accepted range for built-in use.

wait_for_completion_interruptible_timeout() returns the remaining jiffies as a long after a successful completion. Keep that result in a long because an accepted timeout can exceed INT_MAX jiffies on 64-bit systems.

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

Quoted 8 lines
Fixes be8e82caa685 ("nvme-tcp: enable TLS handshake upcall")
Cc [email protected]
Signed-off-by Yingjie Wang <[email protected]>
---
  .../admin-guide/kernel-parameters.txt         |  5 +++
  drivers/nvme/host/tcp.c                       | 34 ++++++++++++++-----
  2 files changed, 31 insertions(+), 8 deletions(-)
Reviewed-by Hannes Reinecke <[email protected]>

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke                  Kernel Storage Architect
[email protected]                                +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