On 11/2/26 20:43, Bobby Eshleman wrote: > On Mon, Nov 02, 2026 at 02:21:27AM +0100, Michal Luczaj wrote: >> On 10/30/26 21:30, Bobby Eshleman wrote: >>>>>> Yeah its all kind of a pain. The problem is that we need to hold >>>>>> sock_lock to set sk_state and sk_err, but can't acquire it under the >>>>>> table lock. Instead, we grab the socket ref sock_hold under the table >>>>>> lock to guarantee it stays alive between releasing the table lock and >>>>>> grabbing the socket lock. >>>>>> >>>>>> I was really tempted to not do this lock_sock dance though. The race >>>>>> condition I'm avoiding here is that if this reset races with an RST >>>>>> receive, then the socket could end up with TCP_CLOSING + sk_err >>>>>> ECONNRESET... but I'm not sure this actually propagates up into a real >>>>>> problem for any potential user. >>>> >>>> Right, vsock_connect() is fragile. Two things to be cautious about: >>>> 1. Don't let it reset the already connected socket. Because that would >>>> allow user to "re-connect()" and make mess in connected_table. >>>> 2. Don't let it re-assign transport of an already connected socket. Because >>>> that breaks the contract in few places. >>>> >>>> State 1 is not an issue: vsock_g2h_reset_unreachable() calls >>>> __vsock_remove_connected(). >>>> >>>> State 2 may be problematic: if vsock_g2h_reset_unreachable() sets TCP_CLOSE >>>> when connect() is scheduled out, then after the wake-up sock may get reset >>>> to SS_UNCONNECTED. Then user re-connect()s and hits the transport >>>> re-assignment logic. I'm worried that, as we re-assign, someone may be >>>> holding a reference to such "connected" (even if not in connected_table >>>> anymore) vsock. >>> >>> I see path you're looking at... after vsock_connect() gets scheduled >>> back in we set TCP_CLOSE/SS_UNCONNECTED if sock_error returns non-zero. >>> Could you elaborate on your last concern there about someone holding a >>> reference? I'm not sure I fully understood. >> >> Ah, sorry, let me explain. This case reminded me of the vsock's TCP_CLOSING >> bug[1]: reporter mentioned vsk->trans getting freed when skb was still in >> flight. I did not reproduce the exact scenario, but it did sound plausible >> and I thought it applies here: virtio_transport_destruct() frees vsk->trans >> the moment vsock_loopback_work() invokes virtio_transport_consume_skb_sent(). >> >> Now, to get there let's start with vsock_connect() setting SS_UNCONNECTED >> (on a connected) socket after vsock_g2h_reset_unreachable(). Then user is >> able to re-assign the transport, which involves >> virtio_transport_destruct(). But since we're dealing with a connected >> socket, (before the reset) some send() might have triggered >> vsock_loopback_work(). Does this make sense to you? > > Ah you're right. I think that your approach of making vsock_connect() > more robust is probably the right way to go... > > I think this is the new race... > > 1) vsock_connect(cid=VMADDR_CID_HOST) > -> enqueue pkt > -> sleep > > 2) rx worker receives OP_RESPONSE > -> sets TCP_ESTABLISHED/SS_CONNECTED > > (vsock_connect() still asleep) > > 2) sendmsg() > -> sees SS_CONNECTED > -> enqueues skb > > 3) netns reset sets TCP_CLOSE and sk_err=ECONNRESET > > 4) vsock_connect() > -> scheduled back in > -> sees sk_err != 0 > -> sets SS_UNCONNECTED > > 5) vsock_connect(CID=VMADDR_CID_LOCAL) call #2 > -> sees SS_UNCONNECTED > -> frees old vsk->trans > > 6) sendmsg's skb tx completes > -> tx worker dereferences vsk->trans for the UAF
Yes, that's exactly what I've meant. > If connect() checks for SS_CONNECTING before setting SS_UNCONNECTED, > then in step 4 sock->state stays as SS_CONNECTING and the second > connect() call will hit -EISCONN instead of freeing vsk->trans. > > Seems to work for a racing vsock_shutdown() too, where sock->state would > become SS_DISCONNECTING, and the second connect() would hit -EINVAL > instead of freeing the vsk->trans. > > I guess the rule would be that vsock_connect() sets SS_CONNECTING before > sleeping and only changes it to SS_UNCONNECTED after waking if and only > if it is still SS_CONNECTING. > > WDYT? I think it's a great idea. Does seem to solve quite a lot. thanks, Michal
