vmbus_channel: GPADL ID collision across a device's subchannels silently overwrites the map entry; later teardown panics on remove().unwrap() #4716

Summary

In vmbus_channel, GPADLs are keyed per-offer on the server side, but all subchannels of one device share a single GpadlMap. GpadlMap::add is a bare insert (vm/vmbus/vmbus_channel/src/gpadl.rs), so if a guest establishes the same GPADL ID on both the primary channel and a subchannel of the same device, the second registration silently overwrites the first entry — no error, no refcount distinction. When the overwritten registration is later torn down, the close path hits remove().unwrap() on an entry that is no longer there and panics, killing that device's channel task.

Affected code (tested at openvmm 1c093df8)

• vm/vmbus/vmbus_channel/src/gpadl.rs — GpadlMap::add (bare insert, around lines 136-147) and the teardown path's remove().unwrap() (around line 155).

Reproduction

Verified with an in-crate test against the unmodified tree (then reverted), plus a positive control:

1. Register GPADL ID X on a device's primary channel, then register the same ID X on one of its subchannels → the map silently keeps only the second entry.
2. Tear down in an order that removes the overwritten entry → panic at the remove().unwrap() in gpadl.rs.
3. Positive control: with distinct IDs, teardown correctly defers until the last GpadlMap view is dropped — i.e. the refcounted revoke handshake itself works; the defect is specifically the silent overwrite on collision.

Guest reachability: multi-channel devices (e.g. storvsp/netvsp with subchannels) let a guest driver issue GPADL establishments per channel, so the colliding sequence is guest-reachable on real device configurations.

Impact

Guest-triggerable panic in a device's channel task (denial of service for that device/VM), and, before the panic, a window where two channels believe they own the same GPADL mapping while the map tracks only one of them — a state inconsistency in the GPA mapping bookkeeping that is worth eliminating on correctness grounds alone.

Suggested fix

Make GpadlMap::add reject (or explicitly refcount) a duplicate ID within the shared map instead of overwriting, and replace the teardown remove().unwrap() with a handled error so a bookkeeping mismatch can never panic.

Found during a local security audit; happy to provide the test case if useful.