Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_vmapplies port mappings without the enable flag or the allowedrange (GHSA-hqm3-qmh8-5xx2).
create_manifest_from_vm_configrefuses a mapping on a node that has portmapping turned off, and refuses one outside
cvm.port_mapping.range:update_vmbuilds its map from the request and never looks atcvm.port_mappingat all, soCreateVmwith no ports followed byUpdateVm{update_ports: true, ports: […]}publishes any host port on a nodewhose whole port policy is
enabled = false. The VMM's default config ships[auth] enabled = false, so on an unhardened node this needs no credential atall.
The same divergence has a quieter half. Deployment lets an omitted
host_addressfall back tocvm.port_mapping.address; the update path parsedthe 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'sGPU allowlist.
resolve_gpushas two arms for one effect."all"discovers devices throughGpuConfig::list_devices, which honourscvm.gpu.listing,cvm.gpu.includeand
cvm.gpu.exclude, and theListGpusRPC the web UI fills its picker fromreads the same function.
"listed"copiesslotout of the request andconsults none of them;
try_allocate_gpuspasses it through unchanged andconfigure_gpusemits-device vfio-pci,host={slot},bus=…from it.So a slot an operator explicitly put in
cvm.gpu.excludeis attachable, adevice that is not a GPU at all is attachable, and the string is never shown to
be a PCI address.
pci_numa_nodecharset-checks it but only on themanifest.hugepagesbranch;gpu_reset::is_pci_slotexists in the same crateand is never applied to request input. QEMU's
-deviceparser treats,as anoption separator and
=as a key/value delimiter —resolve_volume_sourceguards 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 fromboth paths.
heldis the mappings the VM already carries, and they are exempt. This is nota weakening; it is the compat discipline this file already uses.
vmm/ui/src/composables/useVmManager.tssetsupdate_ports = trueunconditionally on every update and sends the VM's current port list back.
Without the exemption, narrowing
cvm.port_mapping.range— or turning portmapping off — would make every other field of an affected VM unsendable through
the UI, over a port nobody touched.
held_networking_configalready spells outthe identical argument for networking; this follows it. Deployment passes an
empty
held, so nothing is exempt there.GPUs.
resolve_gpus_with_confignow resolves the node's published slotsthrough the same
list_devicesthe"all"arm andListGpususe, andensure_gpus_are_publishedrefuses a requested slot that is not among them.Does this reject input that previously worked?
Yes, in two places, and both are the point:
cvm.port_mappingdoes not allow.Mappings a VM already holds keep working. In the other direction it now
accepts input that previously failed: an omitted
host_addresson update.cvm.gpu.exclude, outside a non-emptycvm.gpu.include, or with a productID missing from
cvm.gpu.listing. That set is exactly whatListGpusshows,so the web UI's picker could never have produced a slot outside it;
vmm-cli --gpucould. An operator whoselistingis incomplete for cardsalready in use will have to complete it.
The GPU check also costs one
lspcion the"listed"deployment path, whichthe
"all"path already pays in the same function.How this was verified
port_map_from_protodid not exist onnext, so the reproduction was staged:the function was first introduced as a verbatim lift of
update_vm'sexisting conversion, wired into
update_vm, and the new tests run againstit. The failing run below is therefore
next's own logic, under test. The GPUguard was staged the same way, as
Ok(())— the shape it has onnext.Gates, run from
dstack/. No simulator, no network, no TDX host, and noDSTACK_SIMULATOR_ENDPOINT— the VMM crate's tests are hermetic.dstack-vmmhas no lib target, so
-p dstack-vmm --liberrors; use--bins.--all-targetsdoes not pass onnexteither and was not used.Merging with open PRs
Test-merged against the open PRs touching these files:
dstack/vmm/src/main_service.rsThe #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 — keepport_map_from_protoand itsdelegating call, and fold #1282's three
bail!s in aftervalidate_label:Verified: with that resolution,
cargo test -p dstack-vmm --bin dstack-vmm main_service::gives49 passed; 0 failed, including #1282's owna_deployment_rejects_the_same_zero_resources_a_resize_doesand all five testsadded here.
Not fixed here
Found in the same pass and deliberately left, with reasons, in
.agent/THREAT-operator-registry.md:cvm.max_allocable_vcpuandcvm.max_allocable_memory_in_mbare describedin the proto as "Capacity caps enforced by the scheduler", are reported over
GetMeta, and are compared against nothing anywhere in the tree. Enforcingthem rejects input that works today on any node whose running VMs exceed the
shipped defaults, so it wants its own migration story.
metadata.jsonversiondecides, atapp/qemu.rs:327-334, whether the TD gets anmrconfigidat all, whilemr_config_versiondecides the sibling question from entirely differentinputs. Below
0.5.2the app identity silently leaves the quote, and theguest accepts an all-zero value as "unset".
update_vmwrites the compose file, encrypted env and user config beforethe 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_updatesstill acceptsvcpu = Some(0)fromupdate_vm,which
resize_vmrefuses throughvalidate_resize_request, and which fix(vmm): a registry digest string aborts the whole VMM, and a guest names an unbounded event #1282closes only for deployment.