Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] vsock: unprotected socket state modification in vsock_g2h_reset_unreachable() --
commit 22f9613466fe3520fb04ad3d32779638f9afe30e Author: Bobby Eshleman <[email protected]> vsock: add a netlink command to assign the g2h device to a netns This commit introduces a vsock generic netlink family and a command to assign the guest-to-host (g2h) device to a specific network namespace. It aims to allow isolation of a guest's vsock traffic to a namespace by requiring CAP_NET_ADMIN in the initial user namespace. Connections made before the move from an unreachable namespace are reset. Link: https://lore.kernel.org/all/20200427142518.uwssa6dtasrp3bfc@steredhat/ > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c > --- a/net/vmw_vsock/af_vsock.c > +++ b/net/vmw_vsock/af_vsock.c [ ... ] > +static void vsock_g2h_reset_unreachable(const struct vsock_transport *t) > +{ > + struct vsock_sock *vsk, *tmp; > + LIST_HEAD(reset_list); > + struct sock *sk; > + int i; > + > + /* The calling context must hold vsock_register_mutex, which serializes > + * concurrent netns assignments' use of vsk->pending_reset. > + */ > + lockdep_assert_held(&vsock_register_mutex); > + > + spin_lock_bh(&vsock_table_lock); > + > + for (i = 0; i < ARRAY_SIZE(vsock_connected_table); i++) { > + list_for_each_entry(vsk, &vsock_connected_table[i], > + connected_table) { > + sk = sk_vsock(vsk); > + > + if (vsk->transport != t || > + sk->sk_state == TCP_CLOSE || > + vsock_g2h_reachable_sk(vsk)) > + continue; > + > + sk->sk_state = TCP_CLOSE; > + sk->sk_err = ECONNRESET; > + sk_error_report(sk); [Severity: High] Does this code modify the socket state and error locklessly? Modifying sk->sk_state and sk->sk_err without holding the socket lock (lock_sock()) can introduce data races and TOCTOU vulnerabilities. Concurrent operations such as sendmsg() or recvmsg() that hold lock_sock() expect the socket state to remain stable. If the state is unexpectedly changed to TCP_CLOSE underneath them, it can lead to inconsistent state transitions or invalid packets being queued to the transport. A privileged user with CAP_NET_ADMIN in the initial user namespace could trigger this via the VSOCK_CMD_DEV_NETNS_SET netlink command. Could we acquire lock_sock() for the socket before updating these fields? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
