[PATCH v2] nvme-tcp: fix signedness bug in nvme_tcp_init_connection()

[PATCH v2] nvme-tcp: fix signedness bug in nvme_tcp_init_connection()

[Dan Carpenter]

The kernel_recvmsg() function returns an int which could be either
negative error codes or the number of bytes received.  The problem is
that the condition:

        if (ret < sizeof(*icresp)) {

is type promoted to type unsigned long and negative values are treated
as high positive values which is success, when they should be treated as
failure.  Handle invalid positive returns separately from negative
error codes to avoid this problem.

Fixes: 578539e09690 ("nvme-tcp: fix connect failure on receiving partial ICResp PDU")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
Reviewed-by: Caleb Sander Mateos <csander@purestorage.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
---
v2: Change the style.  Add the Reviewed-by tags.  (I will feel really bad
    if I introduced a bug in between v1 and v2 and cause everyone
    embarrassment with the R-b tags.)

 drivers/nvme/host/tcp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 8a9131c95a3d..b23ce31df97d 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1495,11 +1495,11 @@ static int nvme_tcp_init_connection(struct nvme_tcp_queue *queue)
 	msg.msg_flags = MSG_WAITALL;
 	ret = kernel_recvmsg(queue->sock, &msg, &iov, 1,
 			iov.iov_len, msg.msg_flags);
-	if (ret < sizeof(*icresp)) {
+	if (ret >= 0 && ret < sizeof(*icresp))
+		ret = -ECONNRESET;
+	if (ret < 0) {
 		pr_warn("queue %d: failed to receive icresp, error %d\n",
 			nvme_tcp_queue_id(queue), ret);
-		if (ret >= 0)
-			ret = -ECONNRESET;
 		goto free_icresp;
 	}
 	ret = -ENOTCONN;
-- 
2.47.2

Re: [PATCH v2] nvme-tcp: fix signedness bug in nvme_tcp_init_connection()

[Caleb Sander Mateos]

On Wed, Mar 5, 2025 at 7:53 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> The kernel_recvmsg() function returns an int which could be either
> negative error codes or the number of bytes received.  The problem is
> that the condition:
>
>         if (ret < sizeof(*icresp)) {
>
> is type promoted to type unsigned long and negative values are treated
> as high positive values which is success, when they should be treated as
> failure.  Handle invalid positive returns separately from negative
> error codes to avoid this problem.
>
> Fixes: 578539e09690 ("nvme-tcp: fix connect failure on receiving partial ICResp PDU")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> Reviewed-by: Caleb Sander Mateos <csander@purestorage.com>
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
> v2: Change the style.  Add the Reviewed-by tags.  (I will feel really bad
>     if I introduced a bug in between v1 and v2 and cause everyone
>     embarrassment with the R-b tags.)

Still looks good to me.
Reviewed-by: Caleb Sander Mateos <csander@purestorage.com>

>
>  drivers/nvme/host/tcp.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 8a9131c95a3d..b23ce31df97d 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -1495,11 +1495,11 @@ static int nvme_tcp_init_connection(struct nvme_tcp_queue *queue)
>         msg.msg_flags = MSG_WAITALL;
>         ret = kernel_recvmsg(queue->sock, &msg, &iov, 1,
>                         iov.iov_len, msg.msg_flags);
> -       if (ret < sizeof(*icresp)) {
> +       if (ret >= 0 && ret < sizeof(*icresp))
> +               ret = -ECONNRESET;
> +       if (ret < 0) {
>                 pr_warn("queue %d: failed to receive icresp, error %d\n",
>                         nvme_tcp_queue_id(queue), ret);

This log line is slightly less informative now if a partial PDU is
received, since it will log -ECONNRESET instead of the number of bytes
received before the connection was closed. But I think that's fine.

Best,
Caleb

> -               if (ret >= 0)
> -                       ret = -ECONNRESET;
>                 goto free_icresp;
>         }
>         ret = -ENOTCONN;
> --
> 2.47.2
>

Re: [PATCH v2] nvme-tcp: fix signedness bug in nvme_tcp_init_connection()

[Keith Busch]

On Wed, Mar 05, 2025 at 06:52:59PM +0300, Dan Carpenter wrote:
> The kernel_recvmsg() function returns an int which could be either
> negative error codes or the number of bytes received.  The problem is
> that the condition:
> 
>         if (ret < sizeof(*icresp)) {
> 
> is type promoted to type unsigned long and negative values are treated
> as high positive values which is success, when they should be treated as
> failure.  Handle invalid positive returns separately from negative
> error codes to avoid this problem.

Thanks, applied to nvme-6.14.

I had v1 applied previously, and it was the top commit in that tree;
force pushed this version to replace it.