Re: [PATCH] vsock/virtio: Fix null-ptr-deref in vsock_stream_has_data

[Michal Luczaj <mhal@xxxxxxx>]

On 12/18/24 16:51, Hyunwoo Kim wrote:
> On Wed, Dec 18, 2024 at 04:31:03PM +0100, Stefano Garzarella wrote:
>> On Wed, Dec 18, 2024 at 03:40:40PM +0100, Stefano Garzarella wrote:
>>> On Wed, Dec 18, 2024 at 09:19:08AM -0500, Hyunwoo Kim wrote:
>>>> At least for vsock_loopback.c, this change doesn’t seem to introduce any
>>>> particular issues.
>>>
>>> But was it working for you? because the check was wrong, this one should
>>> work, but still, I didn't have time to test it properly, I'll do later.
>>>
>>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>>> index 9acc13ab3f82..ddecf6e430d6 100644
>>> --- a/net/vmw_vsock/virtio_transport_common.c
>>> +++ b/net/vmw_vsock/virtio_transport_common.c
>>> @@ -1628,8 +1628,10 @@ void virtio_transport_recv_pkt(struct virtio_transport *t,
>>>        lock_sock(sk);
>>> -       /* Check if sk has been closed before lock_sock */
>>> -       if (sock_flag(sk, SOCK_DONE)) {
>>> +       /* Check if sk has been closed or assigned to another transport before
>>> +        * lock_sock
>>> +        */
>>> +       if (sock_flag(sk, SOCK_DONE) || vsk->transport != &t->transport) {
>>>                (void)virtio_transport_reset_no_sock(t, skb);
>>>                release_sock(sk);
>>>                sock_put(sk);

Hi, I got curious about this race, my 2 cents:

Your patch seems to fix the reported issue, but there's also a variant (as
in: transport going null unexpectedly) involving BPF:

/*
$ gcc vsock-transport.c && sudo ./a.out

BUG: kernel NULL pointer dereference, address: 00000000000000a0
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
PGD 12faf8067 P4D 12faf8067 PUD 113670067 PMD 0
Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI
CPU: 15 UID: 0 PID: 1198 Comm: a.out Not tainted 6.13.0-rc2+
RIP: 0010:vsock_connectible_has_data+0x1f/0x40
Call Trace:
 vsock_bpf_recvmsg+0xca/0x5e0
 sock_recvmsg+0xb9/0xc0
 __sys_recvfrom+0xb3/0x130
 __x64_sys_recvfrom+0x20/0x30
 do_syscall_64+0x93/0x180
 entry_SYSCALL_64_after_hwframe+0x76/0x7e
*/

#include <stdio.h>
#include <stdint.h>
#include <stdlib.h>
#include <unistd.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 create_sockmap(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("create_sockmap");

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

	if (syscall(SYS_bpf, BPF_MAP_UPDATE_ELEM, &attr, sizeof(attr)))
		die("map_update_elem");
}

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

	map = create_sockmap();

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

	if (!connect(s, (struct sockaddr *)&addr, alen))
		die("connect #1");
	perror("ok, connect #1 failed; transport set");

	map_update_elem(map, 0, s);

	addr.svm_cid = 42;
	if (!connect(s, (struct sockaddr *)&addr, alen))
		die("connect #2");
	perror("ok, connect #2 failed; transport unset");

	recv(s, NULL, 0, 0);
	return 0;
}