Re: [PATCH v2 1/2] vsock/vmci: make the cached_peer dgram decision race-safe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] vsock/vmci: data tearing on 32-bit allows restricted peer bypass
--

commit 06ed034dbe2daae869975406fc5bbc586f43bfe2
Author: Bartłomiej Dmitruk <bartlomiej.dmitruk@isec.pl>

vsock/vmci: make the cached_peer dgram decision race-safe

vmci_transport_allow_dgram() cached its result in vsock->cached_peer and
vsock->cached_peer_allow_dgram with an unsynchronized check-then-set. The
function runs both in the lockless receive tasklet
(vmci_transport_recv_dgram_cb(), no socket lock) and in the lock_sock() send
path; lock_sock() does not exclude bottom halves, so the two contexts race on
those fields and can return a stale 'allow' for a VMCI_PRIVILEGE_FLAG_RESTRICTED
peer. It is also a plain data race. The in-code comment claiming the fields
are never modified outside create/destruct is contradicted by the send path.

Keep the O(1) cache -- it avoids an O(N) vmci_ctx_get() lookup on every
datagram in the bottom-half receive path -- but pack the peer CID and the
decision into a single word accessed with READ_ONCE()/WRITE_ONCE(). A race
then only forces a recompute and can never return a stale allow.

> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
> index 5549298c1ec6b..9d59b8fe28532 100644
> --- a/include/net/af_vsock.h
> +++ b/include/net/af_vsock.h
[ ... ]
> @@ -39,10 +39,13 @@ struct vsock_sock {
>  	 * modified outsided of socket create or destruct.
>  	 */
>  	bool trusted;
> -	bool cached_peer_allow_dgram;	/* Dgram communication allowed to
> -					 * cached peer?
> -					 */
> -	u32 cached_peer;  /* Context ID of last dgram destination check. */
> +	/* Cached dgram access decision for the last peer, packed as
> +	 * (cid << 32) | VALID | ALLOW and accessed via READ_ONCE()/
> +	 * WRITE_ONCE() so the lockless receive tasklet and the
> +	 * lock_sock() send path cannot race to a stale decision.
> +	 * See vmci_transport_allow_dgram().
> +	 */
> +	u64 cached_peer_access;

[Severity: High]
Can using a 64-bit field here lead to data tearing on 32-bit architectures?

Generic READ_ONCE() and WRITE_ONCE() macros cannot safely read or write
a 64-bit value atomically on 32-bit systems.

>  	const struct cred *owner;
>  	/* Rest are SOCK_STREAM only. */
>  	long connect_timeout;
> diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
> index 1f186e8f83640..31728e26a7705 100644
> --- a/net/vmw_vsock/vmci_transport.c
> +++ b/net/vmw_vsock/vmci_transport.c
[ ... ]
> @@ -524,23 +524,38 @@ static bool vmci_transport_is_trusted(struct vsock_sock *vsock, u32 peer_cid)
>   * only if it is trusted as described in vmci_transport_is_trusted.
>   */
>  
> +/* Packing for vsk->cached_peer_access. */
> +#define VMCI_DGRAM_ACCESS_VALID		BIT_ULL(0)
> +#define VMCI_DGRAM_ACCESS_ALLOW		BIT_ULL(1)
> +#define VMCI_DGRAM_ACCESS_CID_SHIFT	32
> +
>  static bool vmci_transport_allow_dgram(struct vsock_sock *vsock, u32 peer_cid)
>  {
> +	u64 access;
> +
>  	if (VMADDR_CID_HYPERVISOR == peer_cid)
>  		return true;
>  
> -	if (vsock->cached_peer != peer_cid) {
> -		vsock->cached_peer = peer_cid;
> -		if (!vmci_transport_is_trusted(vsock, peer_cid) &&
> -		    (vmci_context_get_priv_flags(peer_cid) &
> -		     VMCI_PRIVILEGE_FLAG_RESTRICTED)) {
> -			vsock->cached_peer_allow_dgram = false;
> -		} else {
> -			vsock->cached_peer_allow_dgram = true;
> -		}
> -	}
> -
> -	return vsock->cached_peer_allow_dgram;
> +	/* Cache the trusted/restricted decision for the last peer to avoid the
> +	 * O(N) vmci_ctx_get() lookup on every datagram.  Read/update it through
> +	 * a single word so a race between the lockless receive tasklet and the
> +	 * lock_sock() send path only forces a recompute -- it can never return a
> +	 * stale allow for a restricted peer.
> +	 */
> +	access = READ_ONCE(vsock->cached_peer_access);

[Severity: High]
Might this read expose partially updated cache states to the lockless receive
tasklet on 32-bit architectures?

Because READ_ONCE() on a 64-bit value can be split into two 32-bit reads,
the tasklet could read a torn state containing the new upper 32 bits (the 
restricted peer's CID) and the old lower 32 bits (with the ALLOW flag from 
a trusted peer).

If the tasklet matches the new CID and sees the ALLOW flag, could it 
incorrectly authorize access?

> +	if ((access & VMCI_DGRAM_ACCESS_VALID) &&
> +	    (u32)(access >> VMCI_DGRAM_ACCESS_CID_SHIFT) == peer_cid)
> +		return !!(access & VMCI_DGRAM_ACCESS_ALLOW);
> +
> +	access = VMCI_DGRAM_ACCESS_VALID |
> +		 ((u64)peer_cid << VMCI_DGRAM_ACCESS_CID_SHIFT);
> +	if (vmci_transport_is_trusted(vsock, peer_cid) ||
> +	    !(vmci_context_get_priv_flags(peer_cid) &
> +	      VMCI_PRIVILEGE_FLAG_RESTRICTED))
> +		access |= VMCI_DGRAM_ACCESS_ALLOW;
> +
> +	WRITE_ONCE(vsock->cached_peer_access, access);

[Severity: High]
Is it possible for this write to compile into two 32-bit stores on 32-bit
systems?

If a datagram is sent from a restricted peer while the host socket concurrently
communicates with a trusted peer, the lock_sock() send path updates this field
using WRITE_ONCE().

If these stores interleave with concurrent readers from the lockless receive
tasklet, it could leave the cache in a torn state.

Could this race allow a restricted VM to bypass datagram access controls and
communicate with a host socket, circumventing VM isolation?

> +	return !!(access & VMCI_DGRAM_ACCESS_ALLOW);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260919123208.29032-1-bartlomiej.dmitruk@isec.pl?part=1