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