| 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
| ||
|
Message-ID: <fezrztdzj5bz54ys6qialz4w3bjqqxmhx74t2tnklbif6ns5dn@mtcjqnqbx6n4>
Date: Thu, 19 Dec 2024 09:19:44 +0100
From: Stefano Garzarella <sgarzare@...hat.com>
To: Hyunwoo Kim <v4bel@...ori.io>, 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>,
Jason Wang <jasowang@...hat.com>, "Michael S. Tsirkin" <mst@...hat.com>,
virtualization@...ts.linux.dev, netdev@...r.kernel.org, qwerty@...ori.io
Subject: Re: [PATCH] vsock/virtio: Fix null-ptr-deref in vsock_stream_has_data
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;
>> }
>>
>
Powered by blists - more mailing lists