linux-kernel - Re: [PATCH net-next] vsock/test: fix test for null ptr deref when transport changes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2bqmwv7oyo4zs2fszonhkt5vup5zhbuai4p6ylevkwktihm6au@kokjoj5dgnbb>
Date: Tue, 8 Jul 2025 14:15:58 +0200
From: Luigi Leonardi <leonardi@...hat.com>
To: Stefano Garzarella <sgarzare@...hat.com>
Cc: netdev@...r.kernel.org, virtualization@...ts.linux.dev, 
	Jakub Kicinski <kuba@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] vsock/test: fix test for null ptr deref when
 transport changes

On Tue, Jul 08, 2025 at 01:17:01PM +0200, Stefano Garzarella wrote:
>From: Stefano Garzarella <sgarzare@...hat.com>
>
>In test_stream_transport_change_client(), the client sends CONTROL_CONTINUE
>on each iteration, even when connect() is unsuccessful. This causes a flood
>of control messages in the server that hangs around for more than 10
>seconds after the test finishes, triggering several timeouts and causing
>subsequent tests to fail. This was discovered in testing a newly proposed
>test that failed in this way on the client side:
>    ...
>    33 - SOCK_STREAM transport change null-ptr-deref...ok
>    34 - SOCK_STREAM ioctl(SIOCINQ) functionality...recv timed out
>
>The CONTROL_CONTINUE message is used only to tell to the server to call
>accept() to consume successful connections, so that subsequent connect()
>will not fail for finding the queue full.
>
>Send CONTROL_CONTINUE message only when the connect() has succeeded, or
>found the queue full. Note that the second connect() can also succeed if
>the first one was interrupted after sending the request.
>
>Fixes: 3a764d93385c ("vsock/test: Add test for null ptr deref when transport changes")
>Cc: leonardi@...hat.com
>Signed-off-by: Stefano Garzarella <sgarzare@...hat.com>
>---
> tools/testing/vsock/vsock_test.c | 21 ++++++++++++++++-----
> 1 file changed, 16 insertions(+), 5 deletions(-)
>
>diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>index be6ce764f694..630110ee31df 100644
>--- a/tools/testing/vsock/vsock_test.c
>+++ b/tools/testing/vsock/vsock_test.c
>@@ -1937,6 +1937,7 @@ static void test_stream_transport_change_client(const struct test_opts *opts)
> 			.svm_cid = opts->peer_cid,
> 			.svm_port = opts->peer_port,
> 		};
>+		bool send_control = false;
> 		int s;
>
> 		s = socket(AF_VSOCK, SOCK_STREAM, 0);
>@@ -1957,19 +1958,29 @@ static void test_stream_transport_change_client(const struct test_opts *opts)
> 			exit(EXIT_FAILURE);
> 		}
>
>+		/* Notify the server if the connect() is successful or the
>+		 * receiver connection queue is full, so it will do accept()
>+		 * to drain it.
>+		 */
>+		if (!ret || errno == ECONNRESET)
>+			send_control = true;
>+
> 		/* Set CID to 0 cause a transport change. */
> 		sa.svm_cid = 0;
>
>-		/* Ignore return value since it can fail or not.
>-		 * If the previous connect is interrupted while the
>-		 * connection request is already sent, the second
>+		/* There is a case where this will not fail:
>+		 * if the previous connect() is interrupted while the
>+		 * connection request is already sent, this second
> 		 * connect() will wait for the response.
> 		 */
>-		connect(s, (struct sockaddr *)&sa, sizeof(sa));
>+		ret = connect(s, (struct sockaddr *)&sa, sizeof(sa));
>+		if (!ret || errno == ECONNRESET)
>+			send_control = true;
>
> 		close(s);
>
>-		control_writeulong(CONTROL_CONTINUE);
>+		if (send_control)
>+			control_writeulong(CONTROL_CONTINUE);
>
> 	} while (current_nsec() < tout);
>
>-- 2.50.0
>

LGTM!
Thanks for the fix!

Reviewed-by: Luigi Leonardi <leonardi@...hat.com>