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: <fjy4jaww6xualdudevfuyoavnrbu45cg4d7erv4rttde363xfc@nahglijbl2eg>
Date: Mon, 10 Mar 2025 15:52:40 +0100
From: Stefano Garzarella <sgarzare@...hat.com>
To: Michal Luczaj <mhal@...x.co>
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 Fri, Mar 07, 2025 at 05:01:11PM +0100, Michal Luczaj wrote:
>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.

Sure, this is fine, thanks for that!

Stefano

>
>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;
>}
>