| 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: <s2k74f6zvjm7uexqfyej6txvoqgf6lkaa47igo2eh4pq55d4n2@wnrrcr6aa6lk>
Date: Wed, 18 Dec 2024 15:40:40 +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 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);
>> > 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