linux-kernel - Re: [PATCH net-next v2] vsock/test: Add 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: <ghik6xpa5oxhb5lc4ztmqlwm3tkv5qbkj63h5mfqs33vursd5y@6jttd2lwwo7h>
Date: Wed, 26 Mar 2025 16:14:20 +0100
From: Luigi Leonardi <leonardi@...hat.com>
To: Michal Luczaj <mhal@...x.co>
Cc: Stefano Garzarella <sgarzare@...hat.com>, 
	virtualization@...ts.linux.dev, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Hyunwoo Kim <v4bel@...ori.io>
Subject: Re: [PATCH net-next v2] vsock/test: Add test for null ptr deref when
 transport changes

Hi Michal,

On Wed, Mar 19, 2025 at 01:27:35AM +0100, Michal Luczaj wrote:
>On 3/14/25 10:27, Luigi Leonardi wrote:
>> Add a new test to ensure that when the transport changes a null pointer
>> dereference does not occur[1].
>>
>> Note that this test does not fail, but it may hang on the client side if
>> it triggers a kernel oops.
>>
>> This works by creating a socket, trying to connect to a server, and then
>> executing a second connect operation on the same socket but to a
>> different CID (0). This triggers a transport change. If the connect
>> operation is interrupted by a signal, this could cause a null-ptr-deref.
>
>Just to be clear: that's the splat, right?
>
>Oops: general protection fault, probably for non-canonical address 0xdffffc000000000c: 0000 [#1] PREEMPT SMP KASAN NOPTI
>KASAN: null-ptr-deref in range [0x0000000000000060-0x0000000000000067]
>CPU: 2 UID: 0 PID: 463 Comm: kworker/2:3 Not tainted
>Workqueue: vsock-loopback vsock_loopback_work
>RIP: 0010:vsock_stream_has_data+0x44/0x70
>Call Trace:
> virtio_transport_do_close+0x68/0x1a0
> virtio_transport_recv_pkt+0x1045/0x2ae4
> vsock_loopback_work+0x27d/0x3f0
> process_one_work+0x846/0x1420
> worker_thread+0x5b3/0xf80
> kthread+0x35a/0x700
> ret_from_fork+0x2d/0x70
> ret_from_fork_asm+0x1a/0x30
>

Yep! I'll add it to the commit message in v3.
>> ...
>> +static void test_stream_transport_change_client(const struct test_opts *opts)
>> +{
>> +	__sighandler_t old_handler;
>> +	pid_t pid = getpid();
>> +	pthread_t thread_id;
>> +	time_t tout;
>> +
>> +	old_handler = signal(SIGUSR1, test_transport_change_signal_handler);
>> +	if (old_handler == SIG_ERR) {
>> +		perror("signal");
>> +		exit(EXIT_FAILURE);
>> +	}
>> +
>> +	if (pthread_create(&thread_id, NULL, test_stream_transport_change_thread, &pid)) {
>> +		perror("pthread_create");
>
>Does pthread_create() set errno on failure?
It does not, very good catch!
>
>> +		exit(EXIT_FAILURE);
>> +	}
>> +
>> +	tout = current_nsec() + TIMEOUT * NSEC_PER_SEC;
>
>Isn't 10 seconds a bit excessive? I see the oops pretty much immediately.
Yeah it's probably excessive. I used because it's the default timeout 
value.
>
>> +	do {
>> +		struct sockaddr_vm sa = {
>> +			.svm_family = AF_VSOCK,
>> +			.svm_cid = opts->peer_cid,
>> +			.svm_port = opts->peer_port,
>> +		};
>> +		int s;
>> +
>> +		s = socket(AF_VSOCK, SOCK_STREAM, 0);
>> +		if (s < 0) {
>> +			perror("socket");
>> +			exit(EXIT_FAILURE);
>> +		}
>> +
>> +		connect(s, (struct sockaddr *)&sa, sizeof(sa));
>> +
>> +		/* Set CID to 0 cause a transport change. */
>> +		sa.svm_cid = 0;
>> +		connect(s, (struct sockaddr *)&sa, sizeof(sa));
>> +
>> +		close(s);
>> +	} while (current_nsec() < tout);
>> +
>> +	if (pthread_cancel(thread_id)) {
>> +		perror("pthread_cancel");
>
>And errno here.
>
>> +		exit(EXIT_FAILURE);
>> +	}
>> +
>> +	/* Wait for the thread to terminate */
>> +	if (pthread_join(thread_id, NULL)) {
>> +		perror("pthread_join");
>
>And here.
>Aaand I've realized I've made exactly the same mistake elsewhere :)
>
>> ...
>> +static void test_stream_transport_change_server(const struct test_opts *opts)
>> +{
>> +	time_t tout = current_nsec() + TIMEOUT * NSEC_PER_SEC;
>> +
>> +	do {
>> +		int s = vsock_stream_listen(VMADDR_CID_ANY, opts->peer_port);
>> +
>> +		close(s);
>> +	} while (current_nsec() < tout);
>> +}
>
>I'm not certain you need to re-create the listener or measure the time
>here. What about something like
>
>	int s = vsock_stream_listen(VMADDR_CID_ANY, opts->peer_port);
>	control_expectln("DONE");
>	close(s);
>
Just tried and it triggers the oops :)

>Thanks,
>Michal
>

Thanks for the review!

Luigi