Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 0 · Low: 0 (1)
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] Torn 64-bit lockless access to `vsock->cached_peer_access` on
32-bit builds defeats the access-control invariant the series claims…
Pre-existing issues:
- [Low] Stale justification comments in both lockless callers of
`vmci_transport_allow_dgram()`.
--
vsock/vmci: enforce per-netns mode on the datagram receive path
This adds a vsock_net_mode_global() check at the top of
vmci_transport_allow_dgram() so the receive path refuses datagrams when the
socket's netns is not in global mode, matching the send hook
vmci_transport_dgram_allow(). The check is placed before the
VMADDR_CID_HYPERVISOR short-circuit so hypervisor datagrams are gated too.
> diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
> index 31728e26a770..d6791459e2fd 100644
> --- a/net/vmw_vsock/vmci_transport.c
> +++ b/net/vmw_vsock/vmci_transport.c
> @@ -533,6 +533,14 @@ static bool vmci_transport_allow_dgram(struct vsock_sock
> *vsock, u32 peer_cid)
> {
> u64 access;
>
> + /* Enforce the per-netns mode first, symmetrically with the send hook
> + * vmci_transport_dgram_allow(): a socket in a non-global (local) netns
> + * must not receive datagrams it could never send (this also covers the
> + * hypervisor CID).
> + */
> + if (!vsock_net_mode_global(vsock))
> + return false;
> +
> if (VMADDR_CID_HYPERVISOR == peer_cid)
> return true;
>
[Severity: High]
While looking at the rest of vmci_transport_allow_dgram(), can the packed
cached_peer_access word tear on 32-bit builds?
The helper reads and writes the decision as one u64:
net/vmw_vsock/vmci_transport.c:vmci_transport_allow_dgram() {
access = READ_ONCE(vsock->cached_peer_access);
if ((access & VMCI_DGRAM_ACCESS_VALID) &&
(u32)(access >> VMCI_DGRAM_ACCESS_CID_SHIFT) == peer_cid)
return !!(access & VMCI_DGRAM_ACCESS_ALLOW);
...
WRITE_ONCE(vsock->cached_peer_access, access);
return !!(access & VMCI_DGRAM_ACCESS_ALLOW);
}
and the comment above it, plus the field comment in include/net/af_vsock.h,
state that using a single word means the race between the receive tasklet
and the send path "can never return a stale allow for a restricted peer".
include/asm-generic/rwonce.h says the opposite for a 64-bit access on a
32-bit architecture:
* Yes, this permits 64-bit accesses on 32-bit architectures. These will
* actually be atomic in some cases (namely Armv7 + LPAE), but for others we
* rely on the access being split into 2x32-bit accesses ... and a strong
* prevailing wind.
drivers/misc/vmw_vmci/Kconfig allows this on 32-bit x86:
depends on (X86 || ARM64) && !CPU_BIG_ENDIAN && PCI
The reader runs with no socket lock held, from
vmci_transport_recv_dgram_cb() and vmci_transport_recv_stream_cb(), while
the writer can run concurrently from vmci_transport_dgram_enqueue() under
lock_sock(), which does not exclude bottom halves.
So on i386, with the cache holding {cid=B, VALID, ALLOW clear} for a
RESTRICTED peer B, can this happen?
CPU0 (sendmsg, trusted peer A) in vmci_transport_allow_dgram()
WRITE_ONCE(vsock->cached_peer_access, access);
-> low word stored first: VALID|ALLOW
-> high word still holds B
CPU1 (receive tasklet, datagram from restricted peer B)
access = READ_ONCE(vsock->cached_peer_access);
-> low = VALID|ALLOW, high = B
(u32)(access >> VMCI_DGRAM_ACCESS_CID_SHIFT) == peer_cid /* matches */
return !!(access & VMCI_DGRAM_ACCESS_ALLOW); /* true */
That would accept a datagram from a VMCI_PRIVILEGE_FLAG_RESTRICTED context
on a non-trusted socket, and the mirrored tear (new high word, old low word)
would drop legitimate datagrams.
Would an atomic64_t accessed with atomic64_read()/atomic64_set(), a
seqcount, or plain serialization be needed here instead of u64
READ_ONCE()/WRITE_ONCE()?
[Severity: Low]
This isn't an issue introduced by this patch, but since
vmci_transport_allow_dgram() is being touched: both lockless callers still
carry a justification that no longer matches the helper.
vmci_transport_recv_dgram_cb() and vmci_transport_recv_stream_cb() both say:
* NOTE: We access the socket struct without holding the lock here.
* This is ok because the field we are interested is never modified
* outside of the create and destruct socket functions.
*/
vsk = vsock_sk(sk);
if (!vmci_transport_allow_dgram(vsk, dg->src.context))
return VMCI_ERROR_NO_ACCESS;
but the helper writes socket state on the ordinary receive and send paths:
WRITE_ONCE(vsock->cached_peer_access, access);
and the field comment in include/net/af_vsock.h documents that both the
receive tasklet and the lock_sock() send path update it.
Could the NOTE be reworded to name the fields that really are immutable
(trusted, owner) and mention that cached_peer_access has its own
READ_ONCE()/WRITE_ONCE() protocol?
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260919123208.29032-1-bartlomiej.dmitruk%40isec.pl
