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:
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.
And that null-ptr-deref occurs because, unlike __vsock_stream_recvmsg,
vsock_bpf_recvmsg does not check vsock->transport:
```
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;
> }
>
