Skip to content

fix(vmm): an update publishes a port the node forbids, and a named GPU is any PCI address - #1286

Open
kvinwang wants to merge 2 commits into
nextfrom
fix/operator-boundary
Open

kvinwang wants to merge 2 commits into
nextfrom
fix/operator-boundary

Conversation

@kvinwang

Copy link
Copy Markdown
Collaborator

Problem

Two places in the VMM let an operator-credentialed caller reach an effect
through a second entry point that does not run the checks the first one does.

1. update_vm applies port mappings without the enable flag or the allowed
range (GHSA-hqm3-qmh8-5xx2).

create_manifest_from_vm_config refuses a mapping on a node that has port
mapping turned off, and refuses one outside cvm.port_mapping.range:

if !(request.ports.is_empty() || pm_cfg.enabled) {
    bail!("Port mapping is disabled");
}if !pm_cfg.is_allowed(&p.protocol, from) {
    bail!("Port mapping is not allowed for {}:{}", p.protocol, from);
}

update_vm builds its map from the request and never looks at
cvm.port_mapping at all, so CreateVm with no ports followed by
UpdateVm{update_ports: true, ports: […]} publishes any host port on a node
whose whole port policy is enabled = false. The VMM's default config ships
[auth] enabled = false, so on an unhardened node this needs no credential at
all.

The same divergence has a quieter half. Deployment lets an omitted
host_address fall back to cvm.port_mapping.address; the update path parsed
the empty string, so the one request shape every deployment may use was
rejected on update with "Invalid host address".

2. attach_mode = "listed" attaches any PCI address, bypassing the node's
GPU allowlist.

resolve_gpus has two arms for one effect. "all" discovers devices through
GpuConfig::list_devices, which honours cvm.gpu.listing, cvm.gpu.include
and cvm.gpu.exclude, and the ListGpus RPC the web UI fills its picker from
reads the same function. "listed" copies slot out of the request and
consults none of them; try_allocate_gpus passes it through unchanged and
configure_gpus emits -device vfio-pci,host={slot},bus=… from it.

So a slot an operator explicitly put in cvm.gpu.exclude is attachable, a
device that is not a GPU at all is attachable, and the string is never shown to
be a PCI address. pci_numa_node charset-checks it but only on the
manifest.hugepages branch; gpu_reset::is_pci_slot exists in the same crate
and is never applied to request input. QEMU's -device parser treats , as an
option separator and = as a key/value delimiter — resolve_volume_source
guards exactly that for volume paths, with a comment saying why, and this path
has no such guard.

Fix

Port mappings. One port_map_from_proto(ports, pm_cfg, held), called from
both paths.

held is the mappings the VM already carries, and they are exempt. This is not
a weakening; it is the compat discipline this file already uses.
vmm/ui/src/composables/useVmManager.ts sets update_ports = true
unconditionally on every update and sends the VM's current port list back.
Without the exemption, narrowing cvm.port_mapping.range — or turning port
mapping off — would make every other field of an affected VM unsendable through
the UI, over a port nobody touched. held_networking_config already spells out
the identical argument for networking; this follows it. Deployment passes an
empty held, so nothing is exempt there.

GPUs. resolve_gpus_with_config now resolves the node's published slots
through the same list_devices the "all" arm and ListGpus use, and
ensure_gpus_are_published refuses a requested slot that is not among them.

Does this reject input that previously worked?

Yes, in two places, and both are the point:

  • A newly added port mapping the node's cvm.port_mapping does not allow.
    Mappings a VM already holds keep working. In the other direction it now
    accepts input that previously failed: an omitted host_address on update.
  • A deployment naming a GPU slot the node does not publish — excluded by
    cvm.gpu.exclude, outside a non-empty cvm.gpu.include, or with a product
    ID missing from cvm.gpu.listing. That set is exactly what ListGpus shows,
    so the web UI's picker could never have produced a slot outside it;
    vmm-cli --gpu could. An operator whose listing is incomplete for cards
    already in use will have to complete it.

The GPU check also costs one lspci on the "listed" deployment path, which
the "all" path already pays in the same function.

How this was verified

port_map_from_proto did not exist on next, so the reproduction was staged:
the function was first introduced as a verbatim lift of update_vm's
existing conversion
, wired into update_vm, and the new tests run against
it. The failing run below is therefore next's own logic, under test. The GPU
guard was staged the same way, as Ok(()) — the shape it has on next.

---- an_update_cannot_publish_a_port_deployment_is_refused stdout ----
called `Result::unwrap_err()` on an `Ok` value:
  [PortMapping { address: 0.0.0.0, protocol: Tcp, from: 2222, to: 2222, nic_index: None }]

---- an_update_cannot_publish_a_port_outside_the_allowed_range stdout ----
called `Result::unwrap_err()` on an `Ok` value:
  [PortMapping { address: 0.0.0.0, protocol: Tcp, from: 22, to: 22, nic_index: None }]

---- an_update_keeps_a_mapping_the_vm_already_holds stdout ----
called `Result::unwrap_err()` on an `Ok` value:
  [PortMapping { address: 0.0.0.0, protocol: Tcp, from: 2223, to: 2223, nic_index: None }]

---- an_omitted_host_address_falls_back_to_the_node_default_on_both_paths stdout ----
called `Result::unwrap()` on an `Err` value: Invalid host address
Caused by: invalid IP address syntax

test result: FAILED. 43 passed; 4 failed; 0 ignored; 0 measured; 169 filtered out
---- a_named_gpu_must_be_one_the_node_publishes stdout ----
called `Result::unwrap_err()` on an `Ok` value: ()

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 216 filtered out

Gates, run from dstack/. No simulator, no network, no TDX host, and no
DSTACK_SIMULATOR_ENDPOINT — the VMM crate's tests are hermetic. dstack-vmm
has no lib target, so -p dstack-vmm --lib errors; use --bins.

cargo test -p dstack-vmm                                                    # 214 passed; 0 failed; 3 ignored
cargo clippy -p dstack-vmm --bins -- -D warnings --allow unused_variables   # clean
cargo clippy --workspace --lib -- -D warnings --allow unused_variables      # clean
cargo fmt --all -- --check                                                  # clean

--all-targets does not pass on next either and was not used.

Merging with open PRs

Test-merged against the open PRs touching these files:

PR result
#1271 clean
#1272 clean
#1282 one conflict in dstack/vmm/src/main_service.rs

The #1282 conflict is textual, not semantic: both sides edit the first lines of
create_manifest_from_vm_config. #1282 inserts its three zero-resource guards;
this branch replaces the inline port-map block with a call to
port_map_from_proto. Take both — keep port_map_from_proto and its
delegating call, and fold #1282's three bail!s in after validate_label:

    validate_label(&request.name)?;
    // The same three a resize refuses. Without this a VM is created with a
    // `-m 0` and a `0G` data disk, and only the launch says so.
    if request.vcpu == 0 {
        bail!("vcpu must be greater than zero");
    }
    if request.memory == 0 {
        bail!("memory must be greater than zero");
    }
    if request.disk_size == 0 {
        bail!("disk_size must be greater than zero");
    }

    // A deployment holds nothing yet, so every mapping it asks for is new.
    let port_map = port_map_from_proto(&request.ports, &cvm_config.port_mapping, &[])?;

Verified: with that resolution,
cargo test -p dstack-vmm --bin dstack-vmm main_service:: gives
49 passed; 0 failed, including #1282's own
a_deployment_rejects_the_same_zero_resources_a_resize_does and all five tests
added here.

Not fixed here

Found in the same pass and deliberately left, with reasons, in
.agent/THREAT-operator-registry.md:

  • cvm.max_allocable_vcpu and cvm.max_allocable_memory_in_mb are described
    in the proto as "Capacity caps enforced by the scheduler", are reported over
    GetMeta, and are compared against nothing anywhere in the tree. Enforcing
    them rejects input that works today on any node whose running VMs exceed the
    shipped defaults, so it wants its own migration story.
  • An image's self-declared metadata.json version decides, at
    app/qemu.rs:327-334, whether the TD gets an mrconfigid at all, while
    mr_config_version decides the sibling question from entirely different
    inputs. Below 0.5.2 the app identity silently leaves the quote, and the
    guest accepts an all-zero value as "unset".
  • update_vm writes the compose file, encrypted env and user config before
    the GPU, port, networking and NIC checks that can still reject the update, so
    a refused update leaves a new compose hash against an old manifest.
  • apply_resource_updates still accepts vcpu = Some(0) from update_vm,
    which resize_vm refuses through validate_resize_request, and which fix(vmm): a registry digest string aborts the whole VMM, and a guest names an unbounded event #1282
    closes only for deployment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant