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@xxxxxxxxx>
Signed-off-by: Wongi Lee <qwerty@xxxxxxxxx>
---
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
