Re: [PATCH 2/2] nvme-tcp: skip TLS control records while waiting for the ICResp

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] nvme-tcp: missing msg_control in nvme_tcp_skip_tls_record() causes -EIO
- [Medium] nvme-tcp: unhandled EOF in nvme_tcp_recv_icresp() on TLS queues
--

commit e6540f770ca0b1e419e220cc7a58ea6cad9fb1eb
Author: Martin Belanger <martin.belanger@dell.com>

nvme-tcp: skip TLS control records while waiting for the ICResp

This commit updates nvme-tcp connection initialization to skip non-data TLS
records (such as TLS 1.3 NewSessionTicket handshake records) and process
TLS alerts while waiting for the ICResp PDU.

> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index a03476f0f69d5..2d361a9e87101 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
[ ... ]
> @@ -1548,12 +1554,138 @@ static void nvme_tcp_free_queue(struct nvme_ctrl *nctrl, int qid)
[ ... ]
> +static int nvme_tcp_skip_tls_record(struct nvme_tcp_queue *queue,
> +				    struct msghdr *msg, u8 ctype, int len)
> +{
> +	u8 buf[NVME_TCP_TLS_REC_CHUNK];
> +	int ret;
> +
> +	switch (ctype) {
> +	case TLS_RECORD_TYPE_HANDSHAKE:
> +		break;
> +	case TLS_RECORD_TYPE_ALERT:
> +		ret = nvme_tcp_recv_tls_alert(queue, msg, len);
> +		return ret ? : -ENOTCONN;
> +	default:
> +		pr_err("queue %d: unhandled TLS record %u\n",
> +		       nvme_tcp_queue_id(queue), ctype);
> +		return -ENOTCONN;
> +	}
> +
> +	/* the ICResp is in a later record, drain this one first */
> +	while (!(msg->msg_flags & MSG_EOR)) {
> +		struct kvec iov = {
> +			.iov_base = buf,
> +			.iov_len = sizeof(buf),
> +		};
> +		struct msghdr drain = {};
> +
> +		ret = kernel_recvmsg(queue->sock, &drain, &iov, 1, sizeof(buf),
> +				     0);
> +		if (ret <= 0) {
> +			pr_warn("queue %d: failed to drain TLS record: %d\n",
> +				nvme_tcp_queue_id(queue), ret);
> +			return ret ? : -ECONNRESET;
> +		}
> +		msg->msg_flags = drain.msg_flags;
> +	}

[Severity: High]
Will draining a handshake record larger than sizeof(*icresp) (128 bytes) in
drivers/nvme/host/tcp.c:nvme_tcp_skip_tls_record() always fail with -EIO?

When a TLS peer (such as an OpenSSL target sending a TLS 1.3
NewSessionTicket) sends a TLS_RECORD_TYPE_HANDSHAKE record larger than 128
bytes, the initial kernel_recvmsg() in nvme_tcp_recv_icresp() reads the
first 128 bytes and returns without setting MSG_EOR in msg.msg_flags.

In nvme_tcp_skip_tls_record(), the drain msghdr is zero-initialized with
drain.msg_control set to NULL and drain.msg_controllen set to 0 before
calling kernel_recvmsg().

In the kTLS receive path:

  kernel_recvmsg()
    -> tls_sw_recvmsg()
      -> process_rx_list()
        -> tls_record_content_type()
          -> put_cmsg()

net/core/scm.c:put_cmsg() checks msg->msg_control:

	if (!msg->msg_control || msg->msg_controllen < sizeof(struct cmsghdr)) {
		msg->msg_flags |= MSG_CTRUNC;
		return 0; /* XXX: return error? check spec. */
	}

And net/tls/tls_sw.c:tls_record_content_type() rejects non-data records
when MSG_CTRUNC is set:

	if (!*control) {
		*control = tlm->control;
		if (!*control)
			return -EBADMSG;

		err = put_cmsg(msg, SOL_TLS, TLS_GET_RECORD_TYPE,
			       sizeof(*control), control);
		if (*control != TLS_RECORD_TYPE_DATA) {
			if (err || msg->msg_flags & MSG_CTRUNC)
				return -EIO;
		}
	}

Because drain does not provide a control message buffer,
tls_record_content_type() returns -EIO without consuming the rest of the
handshake record, causing nvme_tcp_skip_tls_record() and
nvme_tcp_init_connection() to fail with -EIO.

Should drain.msg_control and drain.msg_controllen be populated with a
cmsg buffer on each iteration of the drain loop?

> +
> +	return 0;
> +}
> +
> +/*
> + * Receive the ICResp. Over TLS, skip the records that a peer may send
> + * ahead of it.
> + *
> + * Returns the length received, or a negative errno.
> + */
> +static int nvme_tcp_recv_icresp(struct nvme_tcp_queue *queue,
> +				struct nvme_tcp_icresp_pdu *icresp)
> +{
> +	char cbuf[CMSG_LEN(sizeof(char))];
> +	struct msghdr msg;
> +	struct kvec iov;
> +	u8 ctype;
> +	int i, ret;
> +
> +	for (i = 0; i < NVME_TCP_ICRESP_MAX_RECORDS; i++) {
> +		memset(&msg, 0, sizeof(msg));
> +		memset(cbuf, 0, sizeof(cbuf));
> +		iov.iov_base = icresp;
> +		iov.iov_len = sizeof(*icresp);
> +		if (nvme_tcp_queue_tls(queue)) {
> +			msg.msg_control = cbuf;
> +			msg.msg_controllen = sizeof(cbuf);
> +		}
> +		msg.msg_flags = MSG_WAITALL;
> +		ret = kernel_recvmsg(queue->sock, &msg, &iov, 1,
> +				     iov.iov_len, msg.msg_flags);
> +		if (ret < 0 || !nvme_tcp_queue_tls(queue))
> +			return ret;

[Severity: Medium]
What happens in drivers/nvme/host/tcp.c:nvme_tcp_recv_icresp() if the TLS
peer closes the connection cleanly before sending the ICResp and
kernel_recvmsg() returns 0?

When kernel_recvmsg() returns 0 on EOF over a TLS-enabled queue, the check
above (ret < 0 || !nvme_tcp_queue_tls(queue)) is false, so execution
continues to tls_get_record_type() with the zeroed cbuf.

Since cmsg->cmsg_level is 0, tls_get_record_type() returns 0, and
nvme_tcp_skip_tls_record() hits the default case:

	default:
		pr_err("queue %d: unhandled TLS record %u\n",
		       nvme_tcp_queue_id(queue), ctype);
		return -ENOTCONN;

This logs a spurious "unhandled TLS record 0" error and returns -ENOTCONN,
bypassing the EOF handling in nvme_tcp_init_connection():

	ret = nvme_tcp_recv_icresp(queue, icresp);
	if (ret >= 0 && ret < sizeof(*icresp))
		ret = -ECONNRESET;

Should the early return condition in nvme_tcp_recv_icresp() check ret <= 0
instead of ret < 0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-nvme-tcp-ctrlrec-fixes-v1-0-ebf6ff7eff34@gmail.com?part=2