Re: [PATCH net-next v2] vsock/test: Add test for null ptr deref when transport changes

[Stefano Garzarella]

On Wed, Mar 26, 2025 at 04:14:20PM +0100, Luigi Leonardi wrote:
> 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 :)

If this works (as I also initially thought), we should check the result 
of the first connect() in the client code. It can succeed or fail with 
-EINTR, in other cases we should report an error because it is not 
expected.

And we should check also the second connect(), it should always fail, 
right?

For this I think you need another sync point to be sure the server is 
listening before try to connect the first time:

client:
    // pthread_create, etc.

    control_expectln("LISTENING");

    do {
        ...
    } while();

    control_writeln("DONE");

server:
    int s = vsock_stream_listen(VMADDR_CID_ANY, opts->peer_port);
    control_writeln("LISTENING");
    control_expectln("DONE");
    close(s);

Thanks,
Stefano