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
> Signed-off-by: Wongi Lee
> ---
> 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 yo
Re: [PATCH] vsock/virtio: Fix null-ptr-deref in vsock_stream_has_data
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
> > Signed-off-by: Wongi Lee
> > ---
> > 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
> intro
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 "", line 1, in
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
Signed-off-by: Wongi Lee
---
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 virti
Re: [PATCH] vsock/virtio: Fix null-ptr-deref in vsock_stream_has_data
On 12/18/24 16:51, Hyunwoo Kim wrote:
> On Wed, Dec 18, 2024 at 04:31:03PM +0100, Stefano Garzarella wrote:
>> 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:
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);
Hi, I got curious about this race, my 2 cents:
Your patch seems to fix the reported issue, but there's also a variant (as
in: transport going null unexpectedly) involving BPF:
/*
$ gcc vsock-transport.c && sudo ./a.out
BUG: kernel NULL pointer dereference, address: 00a0
#PF: supervisor read access in kernel mode
#PF: error_code(0x) - not-present page
PGD 12faf8067 P4D 12faf8067 PUD 113670067 PMD 0
Oops: Oops: [#1] PREEMPT SMP NOPTI
CPU: 15 UID: 0 PID: 1198 Comm: a.out Not tainted 6.13.0-rc2+
RIP: 0010:vsock_connectible_has_data+0x1f/0x40
Call Trace:
vsock_bpf_recvmsg+0xca/0x5e0
sock_recvmsg+0xb9/0xc0
__sys_recvfrom+0xb3/0x130
__x64_sys_recvfrom+0x20/0x30
do_syscall_64+0x93/0x180
entry_SYSCALL_64_after_hwframe+0x76/0x7e
*/
#include
#include
#include
#include
#include
#include
#include
#include
static void die(const char *msg)
{
perror(msg);
exit(-1);
}
static int create_sockmap(void)
{
union bpf_attr attr = {
.map_type = BPF_MAP_TYPE_SOCKMAP,
.key_size = sizeof(int),
.value_size = sizeof(int),
.max_entries = 1
};
int map;
map = syscall(SYS_bpf, BPF_MAP_CREATE, &attr, sizeof(attr));
if (map < 0)
die("create_sockmap");
return map;
}
static void map_update_elem(int fd, int key, int value)
{
union bpf_attr attr = {
.map_fd = fd,
.key = (uint64_t)&key,
.value = (uint64_t)&value,
.flags = BPF_ANY
};
if (syscall(SYS_bpf, BPF_MAP_UPDATE_ELEM, &attr, sizeof(attr)))
die("map_update_elem");
}
int main(void)
{
struct sockaddr_vm addr = {
.svm_family = AF_VSOCK,
.svm_port = VMADDR_PORT_ANY,
.svm_cid = VMADDR_CID_LOCAL
};
socklen_t alen = sizeof(addr);
int map, s;
map = create_sockmap();
s = socket(AF_VSOCK, SOCK_SEQPACKET, 0);
if (s < 0)
die("socket");
if (!connect(s, (struct sockaddr *)&addr, alen))
die("connect #1");
perror("ok, connect #1 failed; transport set");
map_update_elem(map, 0, s);
addr.svm_cid = 42;
if (!connect(s, (struct sockaddr *)&addr, alen))
die("connect #2");
perror("ok, connect #2 failed; transport unset");
recv(s, NULL, 0, 0);
return 0;
}
Re: [PATCH] vsock/virtio: Fix null-ptr-deref in vsock_stream_has_data
On Thu, Dec 19, 2024 at 01:25:34AM +0100, Michal Luczaj wrote:
> On 12/18/24 16:51, Hyunwoo Kim wrote:
> > On Wed, Dec 18, 2024 at 04:31:03PM +0100, Stefano Garzarella wrote:
> >> 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:
> 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);
>
> Hi, I got curious about this race, my 2 cents:
>
> Your patch seems to fix the reported issue, but there's also a variant (as
> in: transport going null unexpectedly) involving BPF:
Yes. It seems that calling connect() twice causes the transport to become
NULL, leading to null-ptr-deref in any flow that tries to access that
transport.
And that null-ptr-deref occurs because, unlike __vsock_stream_recvmsg,
vsock_bpf_recvmsg does not check vsock->transport:
```
int
__vsock_connectible_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
int flags)
{
...
lock_sock(sk);
transport = vsk->transport;
if (!transport || sk->sk_state != TCP_ESTABLISHED) {
/* Recvmsg is supposed to return 0 if a peer performs an
* orderly shutdown. Differentiate between that case and when a
* peer has not connected or a local shutdown occurred with the
* SOCK_DONE flag.
*/
if (sock_flag(sk, SOCK_DONE))
err = 0;
else
err = -ENOTCONN;
goto out;
}
```
>
> /*
> $ gcc vsock-transport.c && sudo ./a.out
>
> BUG: kernel NULL pointer dereference, address: 00a0
> #PF: supervisor read access in kernel mode
> #PF: error_code(0x) - not-present page
> PGD 12faf8067 P4D 12faf8067 PUD 113670067 PMD 0
> Oops: Oops: [#1] PREEMPT SMP NOPTI
> CPU: 15 UID: 0 PID: 1198 Comm: a.out Not tainted 6.13.0-rc2+
> RIP: 0010:vsock_connectible_has_data+0x1f/0x40
> Call Trace:
> vsock_bpf_recvmsg+0xca/0x5e0
> sock_recvmsg+0xb9/0xc0
> __sys_recvfrom+0xb3/0x130
> __x64_sys_recvfrom+0x20/0x30
> do_syscall_64+0x93/0x180
> entry_SYSCALL_64_after_hwframe+0x76/0x7e
> */
>
> #include
> #include
> #include
> #include
> #include
> #include
> #include
> #include
>
> static void die(const char *msg)
> {
> perror(msg);
> exit(-1);
> }
>
> static int create_sockmap(void)
> {
> union bpf_attr attr = {
> .map_type = BPF_MAP_TYPE_SOCKMAP,
> .key_size = sizeof(int),
> .value_size = sizeof(int),
> .max_entries = 1
> };
> int map;
>
> map = syscall(SYS_bpf, BPF_MAP_CREATE, &attr, sizeof(attr));
> if (map < 0)
> die("create_sockmap");
>
> return map;
> }
>
> static void map_update_elem(int fd, int key, int value)
> {
> union bpf_attr attr = {
> .map_fd = fd,
> .key = (uint64_t)&key,
> .value = (uint64_t)&value,
> .flags = BPF_ANY
> };
>
> if (syscall(SYS_bpf, BPF_MAP_UPDATE_ELEM, &attr, sizeof(attr)))
> die("map_update_elem");
> }
>
> int main(void)
> {
> struct sockaddr_vm addr = {
> .svm_family = AF_VSOCK,
> .svm_port = VMADDR_PORT_ANY,
> .svm_cid = VMADDR_CID_LOCAL
> };
> socklen_t alen = sizeof(addr);
> int map, s;
>
> map = create_sockmap();
>
> s = socket(AF_VSOCK, SOCK_SEQPACKET, 0);
> if (s < 0)
> die("socket");
>
> if (!connect(s, (struct sockaddr *)&addr, alen))
> die("connect #1");
> perror("ok, connect #1 failed; transport set");
>
> map_update_elem(map, 0, s);
>
> addr.svm_cid = 42;
> if (!connect(s, (struct sockaddr *)&addr, alen))
> die("connect #2");
> perror("ok, connect #2 failed;
Re: [PATCH] vsock/virtio: Fix null-ptr-deref in vsock_stream_has_data
On Wed, Dec 18, 2024 at 04:31:03PM +0100, Stefano Garzarella wrote:
> 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;
By using a kill signal, set sock->state to SS_UNCONNECTED and sk->sk_state to
TCP_CLOSING before the second connect() is called, causing the worker to
execute without the SOCK_DONE flag being set.
>
>
> 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 "", line 1, in
> OSError: [Errno 106] Transport endpoint is already connected
>
>
> Where 106 is exactly EISCONN.
> So, do you have a better reproducer for that?
The full scenario is as follows:
```
cpu0
cpu1
socket(A)
bind(A, {cid:
VMADDR_CID_LOCAL, port: 1024})
vsock_bind()
listen(A)
vsock_listen()
socket(B)
connect(B, {cid: VMADDR_CID_LOCAL, port: 1024})
vsock_connect()
lock_sock(sk);
virtio_transport_connect()
virtio_transport_connect()
virtio_transport_send_pkt_info()
vsock_loopback_send_pkt(VIRTIO_VSOCK_OP_REQUEST)
queue_work(vsock_loopback_work)
sk->sk_state = TCP_SYN_SENT;
release_sock(sk);
vsock_loopback_work()
virtio_transport_recv_pkt(VIRTIO_VSOCK_OP_REQUEST)
sk =
vsock_find_bound_socket(&dst);
virtio_transport_recv_listen(sk, skb)
child =
vsock_create_connected(sk);
vsock_assign_transport()
vvs =
kzalloc(sizeof(*vvs), GFP_KERNEL);
vsock_insert_connected(vchild);
list_add(&vsk->connected_table, list);
virtio_transport_send_response(vchild, skb);
virtio_transport_send_pkt_info()
vsock_loopback_send_pkt(VIRTIO_VSOCK_OP_RESPONSE)
queue_work(vsock_loopback_work)
vsock_loopback_work()
virtio_transport_recv_pkt(VIRTIO_VSOCK_OP_RESPONSE)
Re: [PATCH] vsock/virtio: Fix null-ptr-deref in vsock_stream_has_data
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
Signed-off-by: Wongi Lee
---
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
[PATCH] vsock/virtio: Fix null-ptr-deref in vsock_stream_has_data
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
Signed-off-by: Wongi Lee
---
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;
+
return vsk->transport->stream_has_data(vsk);
}
EXPORT_SYMBOL_GPL(vsock_stream_has_data);
--
2.34.1
