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 <[email protected]> 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/[email protected]?part=1
