On Wed, Dec 18, 2024 at 08:37:38PM -0500, Hyunwoo Kim wrote:
On Thu, Dec 19, 2024 at 01:25:34AM +0100, Michal Luczaj wrote:
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:
I think it is a different problem, to be solved in vsock_bpf.c
With something like this (untested):
diff --git a/net/vmw_vsock/vsock_bpf.c b/net/vmw_vsock/vsock_bpf.c
index 4aa6e74ec295..8c2322dc2af7 100644
--- a/net/vmw_vsock/vsock_bpf.c
+++ b/net/vmw_vsock/vsock_bpf.c
@@ -25,10 +25,8 @@ static struct proto vsock_bpf_prot;
static bool vsock_has_data(struct sock *sk, struct sk_psock *psock)
{
struct vsock_sock *vsk = vsock_sk(sk);
- s64 ret;
- ret = vsock_connectible_has_data(vsk);
- if (ret > 0)
+ if (vsk->transport && vsock_connectible_has_data(vsk) > 0)
return true;
return vsock_sk_has_data(sk, psock);
Note: we should check better this, because sometime we call it without
lock_sock, also thi
Yes. It seems that calling connect() twice causes the transport to become
NULL, leading to null-ptr-deref in any flow that tries to access that
transport.
We already expect vsk->transport to be null in several parts, but in
some we assume it is called when this is valid. So we should check
better what we do when we deassign a transport.
And that null-ptr-deref occurs because, unlike __vsock_stream_recvmsg,
vsock_bpf_recvmsg does not check vsock->transport:
Right.
So, thanks for the report, I'll try to see if I can make a patch with
everything before tomorrow, because then I'm gone for 2 weeks.
Otherwise we'll see as soon as I get back or if you have time in the
meantime, any solution is welcome.
I think the best thing though is to better understand how to handle
deassign, rather than checking everywhere that it's not null, also
because in some cases (like the one in virtio-vsock), it's also
important that the transport is the same.
Thanks,
Stefano
```
int
__vsock_connectible_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
int flags)
{
...
lock_sock(sk);
transport = vsk->transport;
if (!transport || sk->sk_state != TCP_ESTABLISHED) {
/* Recvmsg is supposed to return 0 if a peer performs an
* orderly shutdown. Differentiate between that case and when a
* peer has not connected or a local shutdown occurred with the
* SOCK_DONE flag.
*/
if (sock_flag(sk, SOCK_DONE))
err = 0;
else
err = -ENOTCONN;
goto out;
}
```
/*
$ 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;
}
