nvme_tcp_init_connection() attempts to receive an ICResp PDU but only
checks that the return value from recvmsg() is non-negative. If the
sender closes the TCP connection or sends fewer than 128 bytes, this
check will pass even though the full PDU wasn't received.

Ensure the full ICResp PDU is received by checking that recvmsg()
returns the expected 128 bytes.

Additionally set the MSG_WAITALL flag for recvmsg(), as a sender could
split the ICResp over multiple TCP frames. Without MSG_WAITALL,
recvmsg() could return prematurely with only part of the PDU.

Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
Fixes: 3f2304f8c6d6 ("nvme-tcp: add NVMe over TCP host driver")
---
 drivers/nvme/host/tcp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index dc5bbca58c6d..d23c6243da5d 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1447,13 +1447,14 @@ static int nvme_tcp_init_connection(struct nvme_tcp_queue *queue)
 	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) {
+	if (ret < sizeof(*icresp)) {
 		pr_warn("queue %d: failed to receive icresp, error %d\n",
 			nvme_tcp_queue_id(queue), ret);
 		goto free_icresp;
 	}
 	ret = -ENOTCONN;
-- 
2.45.2

čt 23. 1. 2025 v 23:50 odesílatel Caleb Sander Mateos
<csander@purestorage.com> napsal:
>
>         ret = kernel_recvmsg(queue->sock, &msg, &iov, 1,
>                         iov.iov_len, msg.msg_flags);
> -       if (ret < 0) {
> +       if (ret < sizeof(*icresp)) {
>                 pr_warn("queue %d: failed to receive icresp, error %d\n",
>                         nvme_tcp_queue_id(queue), ret);
>                 goto free_icresp;
>         }

There is a small problem here, suppose ret is a positive number but
smaller than sizeof(*icresp),
you will print a bogus error code, then you "goto free_icresp;" and
return this random positive number to the caller.

I think that if ret > 0 you should set it to -EINVAL.

Maurizio

Good catch! I will send a v3.

On Fri, Jan 24, 2025 at 12:08 AM Maurizio Lombardi <mlombard@redhat.com> wrote:
>
> čt 23. 1. 2025 v 23:50 odesílatel Caleb Sander Mateos
> <csander@purestorage.com> napsal:
> >
> >         ret = kernel_recvmsg(queue->sock, &msg, &iov, 1,
> >                         iov.iov_len, msg.msg_flags);
> > -       if (ret < 0) {
> > +       if (ret < sizeof(*icresp)) {
> >                 pr_warn("queue %d: failed to receive icresp, error %d\n",
> >                         nvme_tcp_queue_id(queue), ret);
> >                 goto free_icresp;
> >         }
>
> There is a small problem here, suppose ret is a positive number but
> smaller than sizeof(*icresp),
> you will print a bogus error code, then you "goto free_icresp;" and
> return this random positive number to the caller.
>
> I think that if ret > 0 you should set it to -EINVAL.
>
> Maurizio
>

pá 24. 1. 2025 v 9:08 odesílatel Maurizio Lombardi <mlombard@redhat.com> napsal:
> I think that if ret > 0 you should set it to -EINVAL.

Small correction: if ret >= 0 set it to -EINVAL.

Maurizio