Re: [PATCH net-next v2 3/6] vsock: add a netlink command to assign the g2h device to a netns

[sashiko-bot@kernel.org]

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 <bobbyeshleman@meta.com>

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/20260921-vsock-guest-ns-v2-0-693bd78fde9e@meta.com?part=3