netdev - Re: [PATCH net] vsock/bpf: Handle EINTR connect() racing against sockmap update

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: <032764f5-e462-4f42-bfdc-8e31b25ada27@rbox.co>
Date: Fri, 7 Mar 2025 17:01:11 +0100
From: Michal Luczaj <mhal@...x.co>
To: Stefano Garzarella <sgarzare@...hat.com>
Cc: "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
 "Michael S. Tsirkin" <mst@...hat.com>, netdev@...r.kernel.org,
 bpf@...r.kernel.org, leonardi@...hat.com
Subject: Re: [PATCH net] vsock/bpf: Handle EINTR connect() racing against
 sockmap update

On 3/7/25 15:35, Stefano Garzarella wrote:
> On Fri, Mar 07, 2025 at 10:58:55AM +0100, Michal Luczaj wrote:
>>> Signal delivered during connect() may result in a disconnect of an already
>>> TCP_ESTABLISHED socket. Problem is that such established socket might have
>>> been placed in a sockmap before the connection was closed. We end up with a
>>> SS_UNCONNECTED vsock in a sockmap. And this, combined with the ability to
>>> reassign (unconnected) vsock's transport to NULL, breaks the sockmap
>>> contract. As manifested by WARN_ON_ONCE.
>>
>> Note that Luigi is currently working on a (vsock test suit) test[1] for a
>> related bug, which could be neatly adapted to test this bug as well.
>> [1]: https://lore.kernel.org/netdev/20250306-test_vsock-v1-0-0320b5accf92@redhat.com/
> 
> Can you work with Luigi to include the changes in that series?

I was just going to wait for Luigi to finish his work (no rush, really) and
then try to parametrize it.

That is unless BPF/sockmap maintainers decide this thread's thing is a
sockmap thing and should be in tools/testing/selftests/bpf.

Below is a repro. If I'm not mistaken, it's basically what Luigi wrote,
just sprinkled with map_update_elem() and recv().

#include <stdio.h>
#include <stdint.h>
#include <stdlib.h>
#include <unistd.h>
#include <errno.h>
#include <pthread.h>
#include <sys/wait.h>
#include <sys/socket.h>
#include <sys/syscall.h>
#include <linux/bpf.h>
#include <linux/vm_sockets.h>

static void die(const char *msg)
{
	perror(msg);
	exit(-1);
}

static int sockmap_create(void)
{
	union bpf_attr attr = {
		.map_type = BPF_MAP_TYPE_SOCKMAP,
		.key_size = sizeof(int),
		.value_size = sizeof(int),
		.max_entries = 1
	};
	int map;

	map = syscall(SYS_bpf, BPF_MAP_CREATE, &attr, sizeof(attr));
	if (map < 0)
		die("map_create");

	return map;
}

static void map_update_elem(int fd, int key, int value)
{
	union bpf_attr attr = {
		.map_fd = fd,
		.key = (uint64_t)&key,
		.value = (uint64_t)&value,
		.flags = BPF_ANY
	};

	(void)syscall(SYS_bpf, BPF_MAP_UPDATE_ELEM, &attr, sizeof(attr));
}

static void sighandler(int sig)
{
	/* nop */
}

static void *racer(void *c)
{
	int map = sockmap_create();

	for (;;) {
		map_update_elem(map, 0, *(int *)c);
 		if (kill(0, SIGUSR1) < 0)
 			die("kill");
 	}
}

int main(void)
{
	struct sockaddr_vm addr = {
		.svm_family = AF_VSOCK,
		.svm_cid = VMADDR_CID_LOCAL,
		.svm_port = VMADDR_PORT_ANY
	};
	socklen_t alen = sizeof(addr);
	struct sockaddr_vm bad_addr;
	pthread_t thread;
	int s, c;

	s = socket(AF_VSOCK, SOCK_SEQPACKET, 0);
	if (s < 0)
		die("socket s");

	if (bind(s, (struct sockaddr *)&addr, alen))
		die("bind");

	if (listen(s, -1))
		die("listen");

	if (getsockname(s, (struct sockaddr *)&addr, &alen))
		die("getsockname");

	bad_addr = addr;
	bad_addr.svm_cid = 0x42424242; /* non-existing */

	if (signal(SIGUSR1, sighandler) == SIG_ERR)
		die("signal");

	if (pthread_create(&thread, 0, racer, &c))
		die("pthread_create");

	for (;;) {
		c = socket(AF_VSOCK, SOCK_SEQPACKET, 0);
		if (c < 0)
			die("socket c");

		if (!connect(c, (struct sockaddr *)&addr, alen) ||
		    errno != EINTR)
			goto outro;

		if (!connect(c, (struct sockaddr *)&bad_addr, alen) ||
		    errno != ESOCKTNOSUPPORT)
			goto outro;

		(void)recv(c, &(char){0}, 1, MSG_DONTWAIT);
outro:
		close(accept(s, NULL, NULL));
		close(c);
	}

	return 0;
}