| 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: <f7a3rlgpc36wk75grqeg6ndqmlprvilznlsesyruqfb7m5vrp7@myil7ex4f62n>
Date: Wed, 18 Dec 2024 16:31:03 +0100
From: Stefano Garzarella <sgarzare@...hat.com>
To: Hyunwoo Kim <v4bel@...ori.io>
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, imv4bel@...il.com
Subject: Re: [PATCH] vsock/virtio: Fix null-ptr-deref in vsock_stream_has_data
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:
>>On Wed, Dec 18, 2024 at 02:40:49PM +0100, Stefano Garzarella wrote:
>>>On Wed, Dec 18, 2024 at 07:25:07AM -0500, Hyunwoo Kim wrote:
>>>> When calling connect to change the CID of a vsock, the loopback
>>>> worker for the VIRTIO_VSOCK_OP_RST command is invoked.
>>>> During this process, vsock_stream_has_data() calls
>>>> vsk->transport->stream_has_data().
>>>> However, a null-ptr-deref occurs because vsk->transport was set
>>>> to NULL in vsock_deassign_transport().
>>>>
>>>> cpu0 cpu1
>>>>
>>>> socket(A)
>>>>
>>>> bind(A, VMADDR_CID_LOCAL)
>>>> vsock_bind()
>>>>
>>>> listen(A)
>>>> vsock_listen()
>>>> socket(B)
>>>>
>>>> connect(B, VMADDR_CID_LOCAL)
>>>>
>>>> connect(B, VMADDR_CID_HYPERVISOR)
>>>> vsock_connect(B)
>>>> lock_sock(sk);
It shouldn't go on here anyway, because there's this check in
vsock_connect():
switch (sock->state) {
case SS_CONNECTED:
err = -EISCONN;
goto out;
Indeed if I try, I have this behaviour:
shell1# python3
import socket
s = socket.socket(socket.AF_VSOCK, socket.SOCK_STREAM)
s.bind((1,1234))
s.listen()
shell2# python3
import socket
s = socket.socket(socket.AF_VSOCK, socket.SOCK_STREAM)
s.connect((1, 1234))
s.connect((2, 1234))
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
OSError: [Errno 106] Transport endpoint is already connected
Where 106 is exactly EISCONN.
So, do you have a better reproducer for that?
Would be nice to add a test in tools/testing/vsock/vsock_test.c
Thanks,
Stefano
>>>> vsock_assign_transport()
>>>> virtio_transport_release()
>>>> virtio_transport_close()
>>>> virtio_transport_shutdown()
>>>> virtio_transport_send_pkt_info()
>>>> vsock_loopback_send_pkt(VIRTIO_VSOCK_OP_SHUTDOWN)
>>>> queue_work(vsock_loopback_work)
>>>> vsock_deassign_transport()
>>>> vsk->transport = NULL;
>>>> vsock_loopback_work()
>>>> virtio_transport_recv_pkt(VIRTIO_VSOCK_OP_SHUTDOWN)
>>>> virtio_transport_recv_connected()
>>>> virtio_transport_reset()
>>>> virtio_transport_send_pkt_info()
>>>> vsock_loopback_send_pkt(VIRTIO_VSOCK_OP_RST)
>>>> queue_work(vsock_loopback_work)
>>>>
>>>> vsock_loopback_work()
>>>> virtio_transport_recv_pkt(VIRTIO_VSOCK_OP_RST)
>>>> virtio_transport_recv_disconnecting()
>>>> virtio_transport_do_close()
>>>> vsock_stream_has_data()
>>>> vsk->transport->stream_has_data(vsk); // null-ptr-deref
>>>>
>>>> To resolve this issue, add a check for vsk->transport, similar to
>>>> functions like vsock_send_shutdown().
>>>>
>>>> Fixes: fe502c4a38d9 ("vsock: add 'transport' member in the struct vsock_sock")
>>>> Signed-off-by: Hyunwoo Kim <v4bel@...ori.io>
>>>> Signed-off-by: Wongi Lee <qwerty@...ori.io>
>>>> ---
>>>> net/vmw_vsock/af_vsock.c | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>>>> index 5cf8109f672a..a0c008626798 100644
>>>> --- a/net/vmw_vsock/af_vsock.c
>>>> +++ b/net/vmw_vsock/af_vsock.c
>>>> @@ -870,6 +870,9 @@ EXPORT_SYMBOL_GPL(vsock_create_connected);
>>>>
>>>> s64 vsock_stream_has_data(struct vsock_sock *vsk)
>>>> {
>>>> + if (!vsk->transport)
>>>> + return 0;
>>>> +
>>>
>>>I understand that this alleviates the problem, but IMO it is not the right
>>>solution. We should understand why we're still processing the packet in the
>>>context of this socket if it's no longer assigned to the right transport.
>>
>>Got it. I agree with you.
>>
>>>
>>>Maybe we can try to improve virtio_transport_recv_pkt() and check if the
>>>vsk->transport is what we expect, I mean something like this (untested):
>>>
>>>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>>>index 9acc13ab3f82..18b91149a62e 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) {
>>> (void)virtio_transport_reset_no_sock(t, skb);
>>> release_sock(sk);
>>> sock_put(sk);
>>>
>>>BTW I'm not sure it is the best solution, we have to check that we do not
>>>introduce strange cases, but IMHO we have to solve the problem earlier in
>>>virtio_transport_recv_pkt().
>>
>>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);
>
>>
>>And separately, I think applying the vsock_stream_has_data patch would help
>>prevent potential issues that could arise when vsock_stream_has_data is
>>called somewhere.
>
>Not sure, with that check, we wouldn't have seen this problem we had,
>so either add an error, but mute it like this I don't think is a good
>idea, also because the same function is used in a hot path, so an
>extra check could affect performance (not much honestly in this case,
>but adding it anywhere could).
>
>Thanks,
>Stefano
>
>>
>>>
>>>Thanks,
>>>Stefano
>>>
>>>> return vsk->transport->stream_has_data(vsk);
>>>> }
>>>> EXPORT_SYMBOL_GPL(vsock_stream_has_data);
>>>> --
>>>> 2.34.1
>>>>
>>>
>>
Powered by blists - more mailing lists