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);
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.
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().
Thanks,
Stefano
return vsk->transport->stream_has_data(vsk);
}
EXPORT_SYMBOL_GPL(vsock_stream_has_data);
--
2.34.1
