From a8dc36aca3f69e1e6d7bf6d59121cf2f71fc25b2 Mon Sep 17 00:00:00 2001 From: Leechael Yim Date: Wed, 12 Aug 2026 13:06:14 +0800 Subject: [PATCH 01/13] feat(vmm): generate QEMU netdev from a pre-opened tap chardev An external net daemon can create a macvtap interface for a VM and expose it as a character device such as /dev/tap7498. QEMU cannot open that node itself under the VMM's launch model, but it can use one that is already open: `-netdev tap,id=netN,fd=M`. This adds the manifest side of that path. Design notes: - `Networking.open_file` is a per-NIC option, not a host default. `cvm.networking.open_file` is rejected during config validation and `resolve_networking` overwrites rather than merges it, because the value names one specific device: inheriting it would attach every NIC of every VM to the same tap. - It pairs with `mode = "custom"` and is mutually exclusive with `netdev`. The netdev string is generated rather than operator-supplied because the descriptor number is decided by the process manager, not by the manifest. - Descriptor numbering follows the LISTEN_FDS convention systemd uses for `OpenFile=`: entries are handed over in declaration order starting at fd 3. `open_files` collects the paths in NIC order and `open_file_fd` derives a NIC's number from how many earlier NICs also asked for one, so both sides agree without a runtime handshake. - `validate_open_file` is deliberately narrow. `:` separates the fields of a systemd `OpenFile=` property and `%` starts a specifier expansion, so either character would change what the property means instead of naming a device. - ProcessConfig carries the paths to the process manager. The field is skipped when empty, so existing Supervisor records and requests keep serializing byte-identically. Two combinations are rejected instead of silently launching a VM whose netdev points at an unrelated descriptor: - `cvm.user`, which prefixes QEMU with sudo; sudo closes every descriptor above stderr before exec. - swtpm, which puts vm-launcher between the process manager and QEMU. vm-launcher would inherit the descriptors and leak them into swtpm, and nothing keeps their numbers stable across its own file operations. --- dstack/supervisor/client/src/main.rs | 1 + dstack/supervisor/src/process.rs | 9 +++ dstack/vmm/src/app.rs | 2 + dstack/vmm/src/app/network.rs | 105 ++++++++++++++++++++++++++- dstack/vmm/src/app/qemu.rs | 86 ++++++++++++++++++++-- dstack/vmm/src/config.rs | 54 ++++++++++++++ dstack/vmm/src/main_service.rs | 3 + 7 files changed, 252 insertions(+), 8 deletions(-) diff --git a/dstack/supervisor/client/src/main.rs b/dstack/supervisor/client/src/main.rs index 16d8c61a5..d6ed9177e 100644 --- a/dstack/supervisor/client/src/main.rs +++ b/dstack/supervisor/client/src/main.rs @@ -74,6 +74,7 @@ async fn main() -> Result<()> { pidfile: String::new(), cid: None, note: String::new(), + open_files: Vec::new(), }; print_json(&client.deploy(&config).await?)?; } diff --git a/dstack/supervisor/src/process.rs b/dstack/supervisor/src/process.rs index 94e0c61e5..42cce1b10 100644 --- a/dstack/supervisor/src/process.rs +++ b/dstack/supervisor/src/process.rs @@ -46,6 +46,15 @@ pub struct ProcessConfig { pub cid: Option, #[serde(default)] pub note: String, + /// Files the process manager opens before exec and passes to the process + /// as inherited file descriptors, in declaration order starting at fd 3. + /// + /// Only the VMM's systemd backend implements this. Supervisor rejects a + /// config that sets it rather than starting a process without the file + /// descriptors it asked for. Skipped when empty so existing records and + /// requests keep serializing byte-identically. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub open_files: Vec, } #[derive(Debug, Clone, Serialize, Deserialize)] diff --git a/dstack/vmm/src/app.rs b/dstack/vmm/src/app.rs index bc88f0d1e..4c4abf55d 100644 --- a/dstack/vmm/src/app.rs +++ b/dstack/vmm/src/app.rs @@ -2166,6 +2166,7 @@ mod tests { dhcp_start: String::new(), restrict: false, netdev: String::new(), + open_file: String::new(), }]; workdir.put_manifest(&manifest)?; @@ -2428,6 +2429,7 @@ mod tests { dhcp_start: String::new(), restrict: false, netdev: String::new(), + open_file: String::new(), }]; let user_manifest = test_manifest(2048); let image = test_tdx_image(true); diff --git a/dstack/vmm/src/app/network.rs b/dstack/vmm/src/app/network.rs index 87925d9a6..72cdb9c17 100644 --- a/dstack/vmm/src/app/network.rs +++ b/dstack/vmm/src/app/network.rs @@ -10,7 +10,9 @@ use anyhow::{bail, Result}; use sha2::{Digest, Sha256}; use super::Manifest; -use crate::config::{CvmConfig, Networking, NetworkingMode}; +use crate::config::{ + validate_open_file, CvmConfig, Networking, NetworkingMode, SD_LISTEN_FDS_START, +}; pub(crate) fn resolve_networking(networking: &Networking, cfg: &CvmConfig) -> Networking { let mut resolved = cfg.networking.clone(); @@ -31,6 +33,9 @@ pub(crate) fn resolve_networking(networking: &Networking, cfg: &CvmConfig) -> Ne if !networking.netdev.is_empty() { resolved.netdev = networking.netdev.clone(); } + // Not merged from the host defaults: a pre-opened chardev names one + // device and belongs to exactly one NIC. + resolved.open_file = networking.open_file.clone(); resolved } @@ -47,6 +52,17 @@ pub(crate) fn resolved_networks(manifest: &Manifest, cfg: &CvmConfig) -> Vec Result<()> { + if !networking.open_file.is_empty() { + validate_open_file("networking.open_file", &networking.open_file)?; + if networking.mode != NetworkingMode::Custom { + bail!("networking.open_file requires mode = \"custom\""); + } + if !networking.netdev.is_empty() { + // The netdev string is generated from the inherited fd number, + // which only the process manager knows. + bail!("networking.open_file and networking.netdev are mutually exclusive"); + } + } if networking.mode != NetworkingMode::Bridge { return Ok(()); } @@ -69,6 +85,32 @@ pub(crate) fn validate_resolved_networks(networks: &[Networking]) -> Result<()> Ok(()) } +/// Chardev paths the process manager must open before exec, in NIC order. +/// +/// The order is the contract: systemd hands the files to the service in +/// declaration order, so entry `i` of this list arrives as fd +/// `SD_LISTEN_FDS_START + i`. +pub(crate) fn open_files(networks: &[Networking]) -> Vec { + networks + .iter() + .filter(|networking| !networking.open_file.is_empty()) + .map(|networking| networking.open_file.clone()) + .collect() +} + +/// File descriptor the NIC at `index` receives, or `None` if it does not use a +/// pre-opened chardev. +pub(crate) fn open_file_fd(networks: &[Networking], index: usize) -> Option { + if networks.get(index)?.open_file.is_empty() { + return None; + } + let preceding = networks[..index] + .iter() + .filter(|networking| !networking.open_file.is_empty()) + .count(); + Some(SD_LISTEN_FDS_START + preceding as u32) +} + /// Derives a deterministic, locally administered unicast MAC address. /// /// Index zero preserves the legacy single-NIC derivation. Later interfaces @@ -95,7 +137,66 @@ pub(crate) fn mac_address_for_vm_index(vm_id: &str, prefix: &[u8], index: usize) #[cfg(test)] mod tests { - use super::mac_address_for_vm_index; + use super::{ + mac_address_for_vm_index, open_file_fd, open_files, validate_resolved_network, Networking, + NetworkingMode, + }; + + fn open_file_network(path: &str) -> Networking { + Networking { + mode: NetworkingMode::Custom, + bridge: String::new(), + mac_prefix: String::new(), + net: String::new(), + dhcp_start: String::new(), + restrict: false, + netdev: String::new(), + open_file: path.into(), + } + } + + #[test] + fn open_file_descriptors_are_numbered_in_nic_order() { + let mut networks = vec![ + open_file_network(""), + open_file_network("/dev/tap10"), + open_file_network(""), + open_file_network("/dev/tap11"), + ]; + networks[0].mode = NetworkingMode::User; + networks[2].mode = NetworkingMode::Bridge; + + assert_eq!(open_files(&networks), ["/dev/tap10", "/dev/tap11"]); + assert_eq!(open_file_fd(&networks, 0), None); + assert_eq!(open_file_fd(&networks, 1), Some(3)); + assert_eq!(open_file_fd(&networks, 2), None); + assert_eq!(open_file_fd(&networks, 3), Some(4)); + assert_eq!(open_file_fd(&networks, 4), None); + } + + #[test] + fn open_file_networks_are_validated() { + validate_resolved_network(&open_file_network("/dev/tap7498")).unwrap(); + + for path in [ + "dev/tap7498", + "/dev/tap 7498", + "/dev/tap7498:foo", + "/dev/tap7498,vhost=on", + "/dev/%i/tap7498", + ] { + validate_resolved_network(&open_file_network(path)).unwrap_err(); + } + + let mut wrong_mode = open_file_network("/dev/tap7498"); + wrong_mode.mode = NetworkingMode::Bridge; + wrong_mode.bridge = "br0".into(); + validate_resolved_network(&wrong_mode).unwrap_err(); + + let mut with_netdev = open_file_network("/dev/tap7498"); + with_netdev.netdev = "tap,id=net0,fd=3".into(); + validate_resolved_network(&with_netdev).unwrap_err(); + } #[test] fn primary_mac_keeps_legacy_derivation_and_later_nics_are_distinct() { diff --git a/dstack/vmm/src/app/qemu.rs b/dstack/vmm/src/app/qemu.rs index 67115c0fe..a3ace3ce5 100644 --- a/dstack/vmm/src/app/qemu.rs +++ b/dstack/vmm/src/app/qemu.rs @@ -9,7 +9,10 @@ use super::{ hugepage_numa_nodes, image::Image, mr_config::{snp_host_data, tdx_mr_config_id}, - network::{mac_address_for_vm_index, resolved_networks, validate_resolved_networks}, + network::{ + mac_address_for_vm_index, open_file_fd, open_files, resolved_networks, + validate_resolved_networks, + }, pci_numa_node, round_up, GpuConfig, VmWorkDir, }; use crate::{ @@ -354,6 +357,14 @@ impl VmConfig { let Some(socket) = prepared.swtpm_socket.as_deref() else { return Ok(vec![process]); }; + if !process.open_files.is_empty() { + // The swtpm path puts vm-launcher between the process manager and + // QEMU. vm-launcher would inherit the descriptors and leak them + // into swtpm as well, and nothing keeps their numbers stable + // across the launcher's own file operations, so QEMU could be + // handed an unrelated fd. Reject instead of guessing. + bail!("networking.open_file is not supported for VMs that use swtpm"); + } let swtpm_path = prepared .swtpm_path .as_ref() @@ -414,6 +425,9 @@ impl VmConfig { pidfile: process.pidfile, cid: process.cid, note: process.note, + // Rejected above: file descriptor passing does not survive the + // vm-launcher indirection. + open_files: Vec::new(), }; Ok(vec![launcher]) } @@ -628,12 +642,18 @@ impl QemuCommandBuilder<'_> { } } NetworkingMode::Custom => { - if !networking.netdev.contains(&format!("id={net_id}")) { - bail!( - "custom networking netdev must contain id={net_id} for interface index {index}" - ); + if let Some(fd) = open_file_fd(&self.prepared.networks, index) { + // The chardev is opened by the process manager, so the + // fd number is the only handle QEMU gets. + format!("tap,id={net_id},fd={fd}") + } else { + if !networking.netdev.contains(&format!("id={net_id}")) { + bail!( + "custom networking netdev must contain id={net_id} for interface index {index}" + ); + } + networking.netdev.clone() } - networking.netdev.clone() } }; command.arg("-netdev").arg(netdev); @@ -781,6 +801,7 @@ impl QemuCommandBuilder<'_> { fn process_config(&self, command: Command) -> Result { let workdir = &self.prepared.workdir; + let open_files = open_files(&self.prepared.networks); let mut arguments = vec![self.cfg.qemu_path.to_string_lossy().to_string()]; arguments.extend( command @@ -791,6 +812,13 @@ impl QemuCommandBuilder<'_> { arguments.splice(0..0, ["taskset", "-c", cpus].into_iter().map(String::from)); } if !self.cfg.user.is_empty() { + if !open_files.is_empty() { + // sudo closes every descriptor above stderr before exec, so + // QEMU would be told to use an fd that no longer exists. + bail!( + "networking.open_file requires cvm.user to be empty: sudo closes inherited file descriptors" + ); + } arguments.splice( 0..0, ["sudo", "-u", &self.cfg.user].into_iter().map(String::from), @@ -819,6 +847,7 @@ impl QemuCommandBuilder<'_> { pidfile: workdir.pid_file().to_string_lossy().to_string(), cid: Some(self.vm.cid), note, + open_files, }) } } @@ -1265,5 +1294,50 @@ mod tests { .args .windows(2) .any(|args| args == ["-tpmdev", "emulator,id=tpm0,chardev=chrtpm"])); + + // Pre-opened chardevs. The first NIC keeps user networking, so the + // two NICs that ask for a chardev take the first two descriptors + // systemd hands over. + prepared.swtpm_socket = None; + prepared.networks.push(config.cvm.networking.clone()); + prepared.networks[0].mode = NetworkingMode::User; + for (index, path) in [(1, "/dev/tap7498"), (2, "/dev/tap7499")] { + let networking = &mut prepared.networks[index]; + networking.mode = NetworkingMode::Custom; + networking.bridge = String::new(); + networking.netdev = String::new(); + networking.open_file = path.into(); + } + let process = QemuCommandBuilder { + vm: &vm, + cfg: &config.cvm, + gpus: &GpuConfig::default(), + prepared: &prepared, + } + .build() + .unwrap(); + assert!(process + .args + .windows(2) + .any(|args| args == ["-netdev", "tap,id=net1,fd=3"])); + assert!(process + .args + .windows(2) + .any(|args| args == ["-netdev", "tap,id=net2,fd=4"])); + assert_eq!(process.open_files, ["/dev/tap7498", "/dev/tap7499"]); + + // sudo closes inherited descriptors, so the combination is rejected + // instead of launching QEMU against an fd that no longer exists. + let mut sudo_config = config.clone(); + sudo_config.cvm.user = "qemu".into(); + let error = QemuCommandBuilder { + vm: &vm, + cfg: &sudo_config.cvm, + gpus: &GpuConfig::default(), + prepared: &prepared, + } + .build() + .unwrap_err(); + assert!(error.to_string().contains("cvm.user"), "{error:#}"); } } diff --git a/dstack/vmm/src/config.rs b/dstack/vmm/src/config.rs index f3ca78d3b..b9fe3cacd 100644 --- a/dstack/vmm/src/config.rs +++ b/dstack/vmm/src/config.rs @@ -798,6 +798,13 @@ fn validate_networking(networking: &Networking) -> Result<()> { "cvm.networking.mac_prefix must contain 1 to 3 two-digit hexadecimal bytes" ); } + // A pre-opened chardev names one specific host device, so it belongs to a + // single VM NIC. Inheriting it as a host-wide default would attach every + // VM to the same tap device. + anyhow::ensure!( + networking.open_file.is_empty(), + "cvm.networking.open_file must be set per VM NIC, not as a host-wide default" + ); match networking.mode { NetworkingMode::Bridge => anyhow::ensure!( !networking.bridge.trim().is_empty(), @@ -812,6 +819,29 @@ fn validate_networking(networking: &Networking) -> Result<()> { Ok(()) } +/// First file descriptor systemd hands to a service, per the LISTEN_FDS +/// convention shared by socket activation and `OpenFile=`. +pub const SD_LISTEN_FDS_START: u32 = 3; + +/// Validates a `open_file` path before it reaches a systemd unit property. +/// +/// systemd parses `OpenFile=` as `path:fdname:options` and expands `%` +/// specifiers, so those characters would change the meaning of the property +/// rather than name a device. The check is deliberately conservative: the only +/// intended values are host device nodes such as `/dev/tap7498`. +pub fn validate_open_file(name: &str, path: &str) -> Result<()> { + anyhow::ensure!( + path.starts_with('/'), + "{name} must be an absolute path: {path}" + ); + anyhow::ensure!( + path.bytes() + .all(|byte| byte.is_ascii_graphic() && !matches!(byte, b':' | b',' | b'%' | b'\\')), + "{name} must not contain whitespace or any of ':' ',' '%' '\\': {path}" + ); + Ok(()) +} + #[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize, Serialize)] #[serde(rename_all = "lowercase")] pub enum NetworkingMode { @@ -849,6 +879,19 @@ pub struct Networking { // ── Custom fields ────────────────────────────────────────────── #[serde(default)] pub netdev: String, + + // ── Pre-opened chardev ───────────────────────────────────────── + /// Absolute path to an already existing tap character device, e.g. + /// `/dev/tap7498` for a macvtap interface created by an external net + /// daemon. The process manager opens it before exec and QEMU inherits it + /// as a file descriptor, so the netdev becomes `tap,id=netN,fd=M`. + /// + /// Only the systemd process manager can pass file descriptors, so this is + /// rejected on every other launch path instead of being silently dropped: + /// QEMU would otherwise open an unrelated fd and attach the guest to the + /// wrong network. + #[serde(default)] + pub open_file: String, } impl Networking { @@ -1157,6 +1200,17 @@ mod tests { assert_eq!(parse("auto"), ProcessManagerBackend::Auto); } + #[test] + fn host_wide_open_file_is_rejected() { + let mut config = default_config(); + config.cvm.networking.open_file = "/dev/tap7498".into(); + assert!(config + .validate() + .unwrap_err() + .to_string() + .contains("open_file")); + } + #[test] fn config_validation_rejects_invalid_static_invariants() { let mut config = default_config(); diff --git a/dstack/vmm/src/main_service.rs b/dstack/vmm/src/main_service.rs index ef52eed4c..d8daedfe0 100644 --- a/dstack/vmm/src/main_service.rs +++ b/dstack/vmm/src/main_service.rs @@ -370,6 +370,9 @@ fn networking_from_proto(proto: &rpc::NetworkingConfig) -> Result Date: Wed, 12 Aug 2026 13:06:26 +0800 Subject: [PATCH 02/13] feat(vmm): open VM chardevs from the systemd transient unit systemd 253+ can open files for a service before exec and pass them as inherited descriptors, which is exactly what a NIC with `open_file` needs. Each path becomes an `OpenFile=` property on the transient unit, in the order the VMM collected them across the VM's NICs. The properties carry no fdname and no `graceful` option on purpose: a missing or unopenable device must fail the unit start. Skipping it would shift every later descriptor down by one and hand QEMU somebody else's file. The paths are re-validated here as well, because a process record can outlive the manifest that produced it. Building the systemd-run argument list moved into `run_args` so the rendered properties can be asserted without a live systemd. Backends that cannot pass descriptors reject the process before anything is spawned rather than launching QEMU against a descriptor that was never opened. --- dstack/vmm/src/process_manager.rs | 159 ++++++++++++++++++++++++------ 1 file changed, 128 insertions(+), 31 deletions(-) diff --git a/dstack/vmm/src/process_manager.rs b/dstack/vmm/src/process_manager.rs index e102dd2ee..f30450384 100644 --- a/dstack/vmm/src/process_manager.rs +++ b/dstack/vmm/src/process_manager.rs @@ -15,6 +15,8 @@ use tokio::process::Command; use tokio::sync::RwLock; use tracing::warn; +use crate::config::validate_open_file; + #[derive(Clone)] pub enum ProcessManager { Supervisor(SupervisorClient), @@ -62,7 +64,10 @@ impl ProcessManager { pub async fn deploy(&self, config: &ProcessConfig) -> Result<()> { match self { - Self::Supervisor(client) => client.deploy(config).await, + Self::Supervisor(client) => { + ensure_no_open_files(config)?; + client.deploy(config).await + } Self::Systemd(manager) => manager.deploy(config).await, Self::Auto(manager) => manager.deploy(config).await, } @@ -101,6 +106,19 @@ impl ProcessManager { } } +/// Rejects a process that needs pre-opened file descriptors on a backend that +/// cannot pass them. Launching it anyway would leave QEMU pointing at whatever +/// the fd number happens to be, so this fails before anything is spawned. +fn ensure_no_open_files(config: &ProcessConfig) -> Result<()> { + if !config.open_files.is_empty() { + bail!( + "process {} requires pre-opened files, which only the systemd process manager supports; set cvm.pm = \"systemd\" or \"auto\"", + config.id + ); + } + Ok(()) +} + pub struct AutoProcessManager { systemd: Arc, supervisor: Option, @@ -355,47 +373,63 @@ impl SystemdProcessManager { Ok(output) } - async fn launch(&self, config: &ProcessConfig) -> Result<()> { - let unit = self.unit(&config.id); - // Failed transient units remain loaded until reset and otherwise - // prevent automatic restart from reusing the unit name. - let mut reset = Command::new("systemctl"); - reset.arg("reset-failed").arg(&unit); - let _ = reset.output().await; - let mut command = Command::new("systemd-run"); - command - .arg("--quiet") - .arg("--unit") - .arg(&unit) - .arg("--service-type=exec") - .arg("--property=KillMode=mixed") - .arg("--property=KillSignal=SIGTERM") - .arg("--property=SendSIGKILL=yes") - .arg(format!("--property=TimeoutStopSec={}", self.stop_timeout)) - .arg("--property=ExitType=cgroup") - .arg("--property=Restart=no") - .arg(format!("--description=dstack VM process {}", config.id)); + fn run_args(&self, config: &ProcessConfig, unit: &str) -> Result> { + let mut args = vec![ + "--quiet".into(), + "--unit".into(), + unit.to_string(), + "--service-type=exec".into(), + "--property=KillMode=mixed".into(), + "--property=KillSignal=SIGTERM".into(), + "--property=SendSIGKILL=yes".into(), + format!("--property=TimeoutStopSec={}", self.stop_timeout), + "--property=ExitType=cgroup".into(), + "--property=Restart=no".into(), + format!("--description=dstack VM process {}", config.id), + ]; if !config.cwd.is_empty() { - command.arg(format!("--working-directory={}", config.cwd)); + args.push(format!("--working-directory={}", config.cwd)); } if config.stdout.is_empty() { - command.arg("--property=StandardOutput=null"); + args.push("--property=StandardOutput=null".into()); } else { - command.arg(format!( + args.push(format!( "--property=StandardOutput=append:{}", config.stdout )); } if config.stderr.is_empty() { - command.arg("--property=StandardError=null"); + args.push("--property=StandardError=null".into()); } else { - command.arg(format!("--property=StandardError=append:{}", config.stderr)); + args.push(format!("--property=StandardError=append:{}", config.stderr)); } for (key, value) in &config.env { - command.arg(format!("--setenv={key}={value}")); + args.push(format!("--setenv={key}={value}")); + } + // systemd opens these before exec and passes them in declaration + // order starting at fd 3, which is what the QEMU netdev arguments + // reference. No fdname and no `graceful` option: a missing device must + // fail the unit instead of shifting every later descriptor by one. + for path in &config.open_files { + validate_open_file("open_files entry", path)?; + args.push(format!("--property=OpenFile={path}")); } - command.arg("--").arg(&config.command).args(&config.args); + args.push("--".into()); + args.push(config.command.clone()); + args.extend(config.args.iter().cloned()); + Ok(args) + } + + async fn launch(&self, config: &ProcessConfig) -> Result<()> { + let unit = self.unit(&config.id); + // Failed transient units remain loaded until reset and otherwise + // prevent automatic restart from reusing the unit name. + let mut reset = Command::new("systemctl"); + reset.arg("reset-failed").arg(&unit); + let _ = reset.output().await; + let mut command = Command::new("systemd-run"); + command.args(self.run_args(config, &unit)?); Self::command(command, "systemd-run").await?; if !config.pidfile.is_empty() { @@ -544,6 +578,30 @@ mod tests { #[test] fn unit_names_are_stable_and_do_not_embed_process_ids() { + let (_dir, manager) = test_manager(); + assert_eq!(manager.unit("vm/one"), manager.unit("vm/one")); + assert_ne!(manager.unit("vm/one"), manager.unit("vm-two")); + assert!(!manager.unit("vm/one").contains("vm/one")); + } + + fn test_config(open_files: &[&str]) -> ProcessConfig { + ProcessConfig { + id: "vm/one".into(), + name: "vm".into(), + command: "/usr/bin/qemu".into(), + args: vec!["-netdev".into(), "tap,id=net0,fd=3".into()], + env: HashMap::new(), + cwd: String::new(), + stdout: String::new(), + stderr: String::new(), + pidfile: String::new(), + cid: None, + note: String::new(), + open_files: open_files.iter().map(|path| path.to_string()).collect(), + } + } + + fn test_manager() -> (tempfile::TempDir, SystemdProcessManager) { let dir = tempfile::tempdir().unwrap(); let manager = SystemdProcessManager::new( dir.path().to_path_buf(), @@ -551,9 +609,48 @@ mod tests { "infinity".into(), ) .unwrap(); - assert_eq!(manager.unit("vm/one"), manager.unit("vm/one")); - assert_ne!(manager.unit("vm/one"), manager.unit("vm-two")); - assert!(!manager.unit("vm/one").contains("vm/one")); + (dir, manager) + } + + #[test] + fn renders_open_files_as_ordered_unit_properties() { + let (_dir, manager) = test_manager(); + let args = manager + .run_args( + &test_config(&["/dev/tap7498", "/dev/tap7499"]), + "unit.service", + ) + .unwrap(); + let properties = args + .iter() + .take_while(|arg| *arg != "--") + .filter_map(|arg| arg.strip_prefix("--property=OpenFile=")) + .collect::>(); + assert_eq!(properties, ["/dev/tap7498", "/dev/tap7499"]); + assert_eq!(args.last().unwrap(), "tap,id=net0,fd=3"); + + assert!(!manager + .run_args(&test_config(&[]), "unit.service") + .unwrap() + .iter() + .any(|arg| arg.contains("OpenFile"))); + } + + #[test] + fn rejects_open_files_that_would_change_the_unit_property() { + let (_dir, manager) = test_manager(); + for path in ["relative/tap", "/dev/tap:0", "/dev/%i/tap"] { + manager + .run_args(&test_config(&[path]), "unit.service") + .unwrap_err(); + } + } + + #[test] + fn supervisor_backend_rejects_open_files() { + ensure_no_open_files(&test_config(&[])).unwrap(); + let error = ensure_no_open_files(&test_config(&["/dev/tap7498"])).unwrap_err(); + assert!(error.to_string().contains("systemd"), "{error:#}"); } #[test] From 23b5bf3f1b4176561d9571a31146edce9eb1d963 Mon Sep 17 00:00:00 2001 From: Leechael Yim Date: Wed, 12 Aug 2026 13:06:26 +0800 Subject: [PATCH 03/13] fix: reject pre-opened chardevs where descriptors cannot be passed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Supervisor spawns processes without pre-opened descriptors and one-shot mode execs QEMU directly, so both would start a VM whose netdev refers to a descriptor that does not exist — or worse, to an unrelated file that happens to occupy the number. Supervisor rejects the deploy at its own API rather than relying on the VMM to filter, since it is a general-purpose process runner with other callers. One-shot mirrors the existing libvirt-filtering guard and still allows --dry-run, which only prints the command. --- dstack/supervisor/src/supervisor.rs | 6 ++++++ dstack/vmm/src/one_shot.rs | 10 ++++++++++ 2 files changed, 16 insertions(+) diff --git a/dstack/supervisor/src/supervisor.rs b/dstack/supervisor/src/supervisor.rs index 378013c05..a73c0acd0 100644 --- a/dstack/supervisor/src/supervisor.rs +++ b/dstack/supervisor/src/supervisor.rs @@ -59,6 +59,12 @@ impl Supervisor { if id.is_empty() { return Err(anyhow::anyhow!("Process ID is empty")); } + if !config.open_files.is_empty() { + // Supervisor spawns processes without pre-opened file descriptors, + // so honoring the rest of the config would start a process that is + // missing the files it depends on. + bail!("open_files is not supported by supervisor"); + } if self .info(&id) .is_some_and(|info| info.state.status.is_running()) diff --git a/dstack/vmm/src/one_shot.rs b/dstack/vmm/src/one_shot.rs index c71bee58f..46ef7e733 100644 --- a/dstack/vmm/src/one_shot.rs +++ b/dstack/vmm/src/one_shot.rs @@ -290,6 +290,16 @@ Compose file content (first 200 chars): ); } + if !dry_run + && resolved_networks(&manifest, &config.cvm) + .iter() + .any(|network| !network.open_file.is_empty()) + { + anyhow::bail!( + "one-shot execution cannot pass pre-opened file descriptors to QEMU; run the VMM server with cvm.pm = \"systemd\" or use --dry-run" + ); + } + let process_configs = vm_builder_config .config_qemu(&workdir_path, &config.cvm, &gpus) .context("Failed to build QEMU configuration")?; From 739c99db9a13885ea456b7e9897ffda35c023d43 Mon Sep 17 00:00:00 2001 From: Leechael Yim Date: Wed, 12 Aug 2026 19:55:41 +0800 Subject: [PATCH 04/13] feat(vmm): drop VM privileges through the systemd unit instead of sudo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Production must not launch VMs through sudo. The systemd backend can do the privilege drop itself, so when `cvm.user` is set and the effective process manager is systemd (`cvm.pm = "systemd"` or `"auto"`, whose new deploys always land on systemd), QEMU is exec'd directly and the transient unit carries `--property=User=`. Supervisor has no such mechanism and keeps the sudo prefix unchanged. `taskset` wrapping is untouched in both cases. This makes `open_file` plus `cvm.user` the normal production combination, so the rejection added for it is now scoped to the supervisor backend, where sudo's closefrom behaviour still destroys the descriptors before QEMU starts. OpenFile= vs the privilege drop: systemd.exec(5) states "The file or socket is opened by the service manager and the file descriptor is passed to the service", and the drop to `User=` happens in the forked child just before exec. The chardev is therefore opened with the manager's privileges, which is what makes this useful: a root-owned /dev/tapN does not have to be chowned to the QEMU user. This is the documented contract rather than an observed one — worth a `systemd-run --property=OpenFile= --property=User=` smoke test on the node before relying on it in production. Mechanics: - ProcessConfig carries `user`, skipped when empty so existing Supervisor records and requests keep serializing byte-identically. Supervisor rejects a non-empty value at its own API instead of running a VM as root that asked to be confined. - The value is validated against a POSIX-user-name charset before it becomes a unit property, so it cannot introduce `%` specifier expansion or extra property syntax. `cvm.user` is checked at config load as well. - One-shot mode rejects a non-empty user: it creates no unit and the command has no sudo prefix, so it would run QEMU with the VMM's own privileges. - The swtpm path passes the user through to the vm-launcher unit, so vm-launcher and the swtpm and QEMU children it spawns all run unprivileged. Under Supervisor only QEMU dropped privileges, so this is a behaviour change for TPM-backed VMs that needs a run on a real node. --- dstack/supervisor/client/src/main.rs | 1 + dstack/supervisor/src/process.rs | 8 +++ dstack/supervisor/src/supervisor.rs | 6 +++ dstack/vmm/src/app/qemu.rs | 74 +++++++++++++++++++++----- dstack/vmm/src/config.rs | 38 ++++++++++++++ dstack/vmm/src/one_shot.rs | 9 ++++ dstack/vmm/src/process_manager.rs | 78 ++++++++++++++++++++++------ 7 files changed, 186 insertions(+), 28 deletions(-) diff --git a/dstack/supervisor/client/src/main.rs b/dstack/supervisor/client/src/main.rs index d6ed9177e..b076a8ddb 100644 --- a/dstack/supervisor/client/src/main.rs +++ b/dstack/supervisor/client/src/main.rs @@ -74,6 +74,7 @@ async fn main() -> Result<()> { pidfile: String::new(), cid: None, note: String::new(), + user: String::new(), open_files: Vec::new(), }; print_json(&client.deploy(&config).await?)?; diff --git a/dstack/supervisor/src/process.rs b/dstack/supervisor/src/process.rs index 42cce1b10..595c583d9 100644 --- a/dstack/supervisor/src/process.rs +++ b/dstack/supervisor/src/process.rs @@ -46,6 +46,14 @@ pub struct ProcessConfig { pub cid: Option, #[serde(default)] pub note: String, + /// User the process manager runs the process as. + /// + /// Only the VMM's systemd backend implements this, by dropping privileges + /// in the transient unit. Supervisor rejects a config that sets it rather + /// than running the process with its own privileges. Skipped when empty so + /// existing records and requests keep serializing byte-identically. + #[serde(default, skip_serializing_if = "String::is_empty")] + pub user: String, /// Files the process manager opens before exec and passes to the process /// as inherited file descriptors, in declaration order starting at fd 3. /// diff --git a/dstack/supervisor/src/supervisor.rs b/dstack/supervisor/src/supervisor.rs index a73c0acd0..18d7c528d 100644 --- a/dstack/supervisor/src/supervisor.rs +++ b/dstack/supervisor/src/supervisor.rs @@ -59,6 +59,12 @@ impl Supervisor { if id.is_empty() { return Err(anyhow::anyhow!("Process ID is empty")); } + if !config.user.is_empty() { + // Supervisor runs processes with its own privileges. Starting the + // process anyway would run a VM as root that asked to be confined + // to an unprivileged user. + bail!("user is not supported by supervisor"); + } if !config.open_files.is_empty() { // Supervisor spawns processes without pre-opened file descriptors, // so honoring the rest of the config would start a process that is diff --git a/dstack/vmm/src/app/qemu.rs b/dstack/vmm/src/app/qemu.rs index a3ace3ce5..0a66d29dc 100644 --- a/dstack/vmm/src/app/qemu.rs +++ b/dstack/vmm/src/app/qemu.rs @@ -19,6 +19,7 @@ use crate::{ app::Manifest, config::{ CvmConfig, CvmPlatform, NetworkFilterMode, Networking, NetworkingMode, ProcessAnnotation, + ProcessManagerBackend, }, netd::{tap_name, InterfaceIdentity}, vm_launcher::{ChildCommand, LaunchSpec}, @@ -425,6 +426,9 @@ impl VmConfig { pidfile: process.pidfile, cid: process.cid, note: process.note, + // The launcher unit owns the privilege drop, so vm-launcher and + // the swtpm and QEMU children it spawns all run as this user. + user: process.user, // Rejected above: file descriptor passing does not survive the // vm-launcher indirection. open_files: Vec::new(), @@ -811,18 +815,26 @@ impl QemuCommandBuilder<'_> { if let Some(cpus) = &self.prepared.numa_cpus { arguments.splice(0..0, ["taskset", "-c", cpus].into_iter().map(String::from)); } + // The systemd backend drops privileges in the unit itself, so QEMU is + // exec'd directly. Supervisor has no such mechanism and keeps the sudo + // prefix, which is also why it cannot pass file descriptors: sudo + // closes every descriptor above stderr before exec, so QEMU would be + // told to use an fd that no longer exists. + let mut user = String::new(); if !self.cfg.user.is_empty() { - if !open_files.is_empty() { - // sudo closes every descriptor above stderr before exec, so - // QEMU would be told to use an fd that no longer exists. - bail!( - "networking.open_file requires cvm.user to be empty: sudo closes inherited file descriptors" + if self.cfg.pm == ProcessManagerBackend::Supervisor { + if !open_files.is_empty() { + bail!( + "networking.open_file requires cvm.pm = \"systemd\" or \"auto\" when cvm.user is set: sudo closes inherited file descriptors" + ); + } + arguments.splice( + 0..0, + ["sudo", "-u", &self.cfg.user].into_iter().map(String::from), ); + } else { + user = self.cfg.user.clone(); } - arguments.splice( - 0..0, - ["sudo", "-u", &self.cfg.user].into_iter().map(String::from), - ); } let command = arguments.remove(0); @@ -847,6 +859,7 @@ impl QemuCommandBuilder<'_> { pidfile: workdir.pid_file().to_string_lossy().to_string(), cid: Some(self.vm.cid), note, + user, open_files, }) } @@ -1036,7 +1049,8 @@ mod tests { use crate::app::image::{Image, ImageInfo}; use crate::app::{needs_swtpm, GpuConfig, Manifest, PortMapping, VmVolume, VmWorkDir}; use crate::config::{ - Config, CvmPlatform, NetworkFilterMode, NetworkingMode, Protocol, DEFAULT_CONFIG, + Config, CvmPlatform, NetworkFilterMode, NetworkingMode, ProcessManagerBackend, Protocol, + DEFAULT_CONFIG, }; use crate::netd::{tap_name, InterfaceIdentity}; use dstack_types::{KeyProviderKind, TeeVariant}; @@ -1326,8 +1340,25 @@ mod tests { .any(|args| args == ["-netdev", "tap,id=net2,fd=4"])); assert_eq!(process.open_files, ["/dev/tap7498", "/dev/tap7499"]); - // sudo closes inherited descriptors, so the combination is rejected - // instead of launching QEMU against an fd that no longer exists. + // systemd drops privileges in the unit, so QEMU is exec'd directly and + // keeps the descriptors it was handed. + let mut systemd_config = config.clone(); + systemd_config.cvm.pm = ProcessManagerBackend::Systemd; + systemd_config.cvm.user = "qemu".into(); + let process = QemuCommandBuilder { + vm: &vm, + cfg: &systemd_config.cvm, + gpus: &GpuConfig::default(), + prepared: &prepared, + } + .build() + .unwrap(); + assert_eq!(process.user, "qemu"); + assert_eq!(process.command, "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/not-installed/qemu-system-x86_64"); + assert!(!process.args.iter().any(|arg| arg == "sudo")); + + // Supervisor has no privilege-drop mechanism and falls back to sudo, + // which closes the descriptors before QEMU starts. let mut sudo_config = config.clone(); sudo_config.cvm.user = "qemu".into(); let error = QemuCommandBuilder { @@ -1338,6 +1369,23 @@ mod tests { } .build() .unwrap_err(); - assert!(error.to_string().contains("cvm.user"), "{error:#}"); + assert!(error.to_string().contains("cvm.pm"), "{error:#}"); + + // Without a pre-opened chardev, Supervisor keeps the sudo prefix. + for networking in &mut prepared.networks { + networking.open_file = String::new(); + networking.mode = NetworkingMode::User; + } + let process = QemuCommandBuilder { + vm: &vm, + cfg: &sudo_config.cvm, + gpus: &GpuConfig::default(), + prepared: &prepared, + } + .build() + .unwrap(); + assert_eq!(process.command, "sudo"); + assert_eq!(&process.args[..2], ["-u", "qemu"]); + assert!(process.user.is_empty()); } } diff --git a/dstack/vmm/src/config.rs b/dstack/vmm/src/config.rs index b9fe3cacd..9d22b4040 100644 --- a/dstack/vmm/src/config.rs +++ b/dstack/vmm/src/config.rs @@ -727,6 +727,9 @@ impl Config { } validate_networking(&self.cvm.networking)?; + if !self.cvm.user.is_empty() { + validate_unit_user("cvm.user", &self.cvm.user)?; + } if self.cvm.pm != ProcessManagerBackend::Systemd { anyhow::ensure!( !self.supervisor.sock.trim().is_empty(), @@ -829,6 +832,25 @@ pub const SD_LISTEN_FDS_START: u32 = 3; /// specifiers, so those characters would change the meaning of the property /// rather than name a device. The check is deliberately conservative: the only /// intended values are host device nodes such as `/dev/tap7498`. +/// Validates a user name before it reaches a systemd unit property. +/// +/// `User=` takes a user name or a numeric UID. The charset is kept to what a +/// POSIX user name can contain so the value cannot introduce a `%` specifier +/// expansion or extra property syntax. +pub fn validate_unit_user(name: &str, user: &str) -> Result<()> { + anyhow::ensure!(!user.is_empty(), "{name} must not be empty"); + anyhow::ensure!( + !user.starts_with('-'), + "{name} must not start with '-': {user}" + ); + anyhow::ensure!( + user.bytes() + .all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'_' | b'-' | b'.')), + "{name} must contain only alphanumerics, '_', '-' and '.': {user}" + ); + Ok(()) +} + pub fn validate_open_file(name: &str, path: &str) -> Result<()> { anyhow::ensure!( path.starts_with('/'), @@ -1211,6 +1233,22 @@ mod tests { .contains("open_file")); } + #[test] + fn unit_user_names_are_validated() { + validate_unit_user("cvm.user", "qemu-1.user_x").unwrap(); + for user in ["", "-qemu", "qemu:0", "qemu user", "%i", "qemu$"] { + validate_unit_user("cvm.user", user).unwrap_err(); + } + + let mut config = default_config(); + config.cvm.user = "qemu:0".into(); + assert!(config + .validate() + .unwrap_err() + .to_string() + .contains("cvm.user")); + } + #[test] fn config_validation_rejects_invalid_static_invariants() { let mut config = default_config(); diff --git a/dstack/vmm/src/one_shot.rs b/dstack/vmm/src/one_shot.rs index 46ef7e733..deb15f31d 100644 --- a/dstack/vmm/src/one_shot.rs +++ b/dstack/vmm/src/one_shot.rs @@ -322,6 +322,15 @@ Compose file content (first 200 chars): println!("# QEMU Command:"); println!("{}", full_command.join(" ")); + if !dry_run && !process_config.user.is_empty() { + // Privileges are dropped by the systemd unit, which one-shot mode does + // not create, and the command carries no sudo prefix either. Running it + // here would start QEMU with the VMM's own privileges. + anyhow::bail!( + "one-shot execution cannot drop privileges to cvm.user with cvm.pm = \"systemd\" or \"auto\"; use --dry-run or cvm.pm = \"supervisor\"" + ); + } + if dry_run { println!("# Dry run mode - QEMU command not executed"); println!( diff --git a/dstack/vmm/src/process_manager.rs b/dstack/vmm/src/process_manager.rs index f30450384..5b6e2b752 100644 --- a/dstack/vmm/src/process_manager.rs +++ b/dstack/vmm/src/process_manager.rs @@ -15,7 +15,7 @@ use tokio::process::Command; use tokio::sync::RwLock; use tracing::warn; -use crate::config::validate_open_file; +use crate::config::{validate_open_file, validate_unit_user}; #[derive(Clone)] pub enum ProcessManager { @@ -65,7 +65,7 @@ impl ProcessManager { pub async fn deploy(&self, config: &ProcessConfig) -> Result<()> { match self { Self::Supervisor(client) => { - ensure_no_open_files(config)?; + ensure_supervisor_supported(config)?; client.deploy(config).await } Self::Systemd(manager) => manager.deploy(config).await, @@ -106,15 +106,23 @@ impl ProcessManager { } } -/// Rejects a process that needs pre-opened file descriptors on a backend that -/// cannot pass them. Launching it anyway would leave QEMU pointing at whatever -/// the fd number happens to be, so this fails before anything is spawned. -fn ensure_no_open_files(config: &ProcessConfig) -> Result<()> { - if !config.open_files.is_empty() { - bail!( - "process {} requires pre-opened files, which only the systemd process manager supports; set cvm.pm = \"systemd\" or \"auto\"", - config.id - ); +/// Rejects a process asking for something Supervisor cannot provide. +/// +/// Supervisor spawns processes with its own privileges and without pre-opened +/// file descriptors. Launching anyway would run a VM as root that asked to be +/// confined, or leave QEMU pointing at whatever the fd number happens to be, +/// so this fails before anything is spawned. +fn ensure_supervisor_supported(config: &ProcessConfig) -> Result<()> { + for (what, unsupported) in [ + ("pre-opened files", !config.open_files.is_empty()), + ("a dedicated user", !config.user.is_empty()), + ] { + if unsupported { + bail!( + "process {} requires {what}, which only the systemd process manager supports; set cvm.pm = \"systemd\" or \"auto\"", + config.id + ); + } } Ok(()) } @@ -407,10 +415,20 @@ impl SystemdProcessManager { for (key, value) in &config.env { args.push(format!("--setenv={key}={value}")); } + if !config.user.is_empty() { + validate_unit_user("user", &config.user)?; + args.push(format!("--property=User={}", config.user)); + } // systemd opens these before exec and passes them in declaration // order starting at fd 3, which is what the QEMU netdev arguments // reference. No fdname and no `graceful` option: a missing device must // fail the unit instead of shifting every later descriptor by one. + // + // systemd.exec(5): "The file or socket is opened by the service + // manager and the file descriptor is passed to the service." The open + // therefore happens with the manager's privileges, before the `User=` + // drop that lands just before exec, so a root-owned chardev such as + // /dev/tapN does not have to be chowned to the QEMU user. for path in &config.open_files { validate_open_file("open_files entry", path)?; args.push(format!("--property=OpenFile={path}")); @@ -585,6 +603,10 @@ mod tests { } fn test_config(open_files: &[&str]) -> ProcessConfig { + test_config_as("", open_files) + } + + fn test_config_as(user: &str, open_files: &[&str]) -> ProcessConfig { ProcessConfig { id: "vm/one".into(), name: "vm".into(), @@ -597,6 +619,7 @@ mod tests { pidfile: String::new(), cid: None, note: String::new(), + user: user.into(), open_files: open_files.iter().map(|path| path.to_string()).collect(), } } @@ -647,10 +670,35 @@ mod tests { } #[test] - fn supervisor_backend_rejects_open_files() { - ensure_no_open_files(&test_config(&[])).unwrap(); - let error = ensure_no_open_files(&test_config(&["/dev/tap7498"])).unwrap_err(); - assert!(error.to_string().contains("systemd"), "{error:#}"); + fn renders_the_privilege_drop_as_a_unit_property() { + let (_dir, manager) = test_manager(); + let args = manager + .run_args(&test_config_as("qemu", &["/dev/tap7498"]), "unit.service") + .unwrap(); + assert!(args.iter().any(|arg| arg == "--property=User=qemu")); + // The privilege drop replaces the sudo prefix rather than joining it. + assert!(!args.iter().any(|arg| arg == "sudo")); + + assert!(!manager + .run_args(&test_config(&[]), "unit.service") + .unwrap() + .iter() + .any(|arg| arg.contains("User="))); + + for user in ["qemu:0", "qemu user", "%i", "-qemu"] { + manager + .run_args(&test_config_as(user, &[]), "unit.service") + .unwrap_err(); + } + } + + #[test] + fn supervisor_backend_rejects_what_it_cannot_provide() { + ensure_supervisor_supported(&test_config(&[])).unwrap(); + for config in [test_config(&["/dev/tap7498"]), test_config_as("qemu", &[])] { + let error = ensure_supervisor_supported(&config).unwrap_err(); + assert!(error.to_string().contains("systemd"), "{error:#}"); + } } #[test] From 8c9e0051ff0cb695003d4f46ea9cd8d04704353c Mon Sep 17 00:00:00 2001 From: Leechael Yim Date: Thu, 13 Aug 2026 00:10:47 +0800 Subject: [PATCH 05/13] fix(supervisor): default ProcessConfig builder fields for user and open_files --- dstack/supervisor/src/process.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dstack/supervisor/src/process.rs b/dstack/supervisor/src/process.rs index 595c583d9..6286d0087 100644 --- a/dstack/supervisor/src/process.rs +++ b/dstack/supervisor/src/process.rs @@ -53,6 +53,7 @@ pub struct ProcessConfig { /// than running the process with its own privileges. Skipped when empty so /// existing records and requests keep serializing byte-identically. #[serde(default, skip_serializing_if = "String::is_empty")] + #[builder(default)] pub user: String, /// Files the process manager opens before exec and passes to the process /// as inherited file descriptors, in declaration order starting at fd 3. @@ -62,6 +63,7 @@ pub struct ProcessConfig { /// descriptors it asked for. Skipped when empty so existing records and /// requests keep serializing byte-identically. #[serde(default, skip_serializing_if = "Vec::is_empty")] + #[builder(default)] pub open_files: Vec, } From 2d16a9c813ea8cc9f5d591a8aa0d7a22ef95d52b Mon Sep 17 00:00:00 2001 From: Leechael Yim Date: Thu, 13 Aug 2026 00:10:48 +0800 Subject: [PATCH 06/13] fix(vmm): accept numeric cvm.user for sudo and systemd --- dstack/vmm/src/app.rs | 8 +- dstack/vmm/src/app/qemu.rs | 55 +++++++++-- dstack/vmm/src/config.rs | 146 +++++++++++++++++++++++------- dstack/vmm/src/process_manager.rs | 18 +++- 4 files changed, 175 insertions(+), 52 deletions(-) diff --git a/dstack/vmm/src/app.rs b/dstack/vmm/src/app.rs index 4c4abf55d..f57cf0683 100644 --- a/dstack/vmm/src/app.rs +++ b/dstack/vmm/src/app.rs @@ -22,7 +22,7 @@ use dstack_vmm_rpc::{ use fs_err as fs; use guest_api::client::DefaultClient as GuestClient; use id_pool::IdPool; -use nix::unistd::{Uid, User}; +use nix::unistd::Uid; use or_panic::ResultOrPanic; use ra_rpc::client::RaClient; use serde::{Deserialize, Serialize}; @@ -521,11 +521,7 @@ impl App { let qemu_uid = if self.config.cvm.user.is_empty() { Uid::effective().as_raw() } else { - User::from_name(&self.config.cvm.user) - .context("failed to resolve QEMU user")? - .with_context(|| format!("QEMU user {} does not exist", self.config.cvm.user))? - .uid - .as_raw() + qemu::resolve_cvm_user(&self.config.cvm.user)?.uid.as_raw() }; let mut prepared = Vec::new(); for (nic_index, network) in networks.iter().enumerate() { diff --git a/dstack/vmm/src/app/qemu.rs b/dstack/vmm/src/app/qemu.rs index 0a66d29dc..2f2a825fb 100644 --- a/dstack/vmm/src/app/qemu.rs +++ b/dstack/vmm/src/app/qemu.rs @@ -18,8 +18,8 @@ use super::{ use crate::{ app::Manifest, config::{ - CvmConfig, CvmPlatform, NetworkFilterMode, Networking, NetworkingMode, ProcessAnnotation, - ProcessManagerBackend, + parse_unit_user, CvmConfig, CvmPlatform, NetworkFilterMode, Networking, NetworkingMode, + ProcessAnnotation, ProcessManagerBackend, UnitUser, }, netd::{tap_name, InterfaceIdentity}, vm_launcher::{ChildCommand, LaunchSpec}, @@ -28,7 +28,7 @@ use anyhow::{bail, Context, Result}; use bon::Builder; use dstack_types::shared_filenames::HOST_SHARED_DISK_LABEL; use fs_err as fs; -use nix::unistd::User; +use nix::unistd::{Uid, User}; use serde::Serialize; use std::collections::HashMap; use std::os::unix::fs::PermissionsExt; @@ -312,6 +312,17 @@ fn prepare_data_disk(vm: &VmConfig, workdir: &VmWorkDir, cfg: &CvmConfig) -> Res Ok(()) } +pub(crate) fn resolve_cvm_user(user: &str) -> Result { + match parse_unit_user("cvm.user", user)? { + UnitUser::Name(name) => User::from_name(&name) + .context("failed to resolve QEMU user")? + .with_context(|| format!("QEMU user {name} does not exist")), + UnitUser::Uid(uid) => User::from_uid(Uid::from_raw(uid)) + .context("failed to resolve QEMU user")? + .with_context(|| format!("QEMU user uid {uid} does not exist")), + } +} + fn prepare_shared_dir(workdir: &VmWorkDir) -> Result<()> { let shared_dir = workdir.shared_dir(); if !shared_dir.exists() { @@ -373,9 +384,7 @@ impl VmConfig { let (socket_uid, socket_gid) = if cfg.user.is_empty() { (unsafe { libc::geteuid() }, unsafe { libc::getegid() }) } else { - let user = User::from_name(&cfg.user) - .context("failed to resolve QEMU user")? - .with_context(|| format!("QEMU user {} does not exist", cfg.user))?; + let user = resolve_cvm_user(&cfg.user)?; (user.uid.as_raw(), user.gid.as_raw()) }; @@ -822,18 +831,21 @@ impl QemuCommandBuilder<'_> { // told to use an fd that no longer exists. let mut user = String::new(); if !self.cfg.user.is_empty() { + let unit_user = parse_unit_user("cvm.user", &self.cfg.user)?; if self.cfg.pm == ProcessManagerBackend::Supervisor { if !open_files.is_empty() { bail!( "networking.open_file requires cvm.pm = \"systemd\" or \"auto\" when cvm.user is set: sudo closes inherited file descriptors" ); } + let sudo_user = unit_user.sudo_value(); arguments.splice( 0..0, - ["sudo", "-u", &self.cfg.user].into_iter().map(String::from), + ["sudo", "-u", &sudo_user].into_iter().map(String::from), ); } else { - user = self.cfg.user.clone(); + // systemd User= takes a bare name or decimal UID, not sudo's #UID. + user = unit_user.systemd_value(); } } @@ -1387,5 +1399,32 @@ mod tests { assert_eq!(process.command, "sudo"); assert_eq!(&process.args[..2], ["-u", "qemu"]); assert!(process.user.is_empty()); + + // Numeric UIDs keep sudo's #UID form and systemd's bare digits. + sudo_config.cvm.user = "#1000".into(); + let process = QemuCommandBuilder { + vm: &vm, + cfg: &sudo_config.cvm, + gpus: &GpuConfig::default(), + prepared: &prepared, + } + .build() + .unwrap(); + assert_eq!(process.command, "sudo"); + assert_eq!(&process.args[..2], ["-u", "#1000"]); + + let mut uid_config = config.clone(); + uid_config.cvm.pm = ProcessManagerBackend::Systemd; + uid_config.cvm.user = "#1000".into(); + let process = QemuCommandBuilder { + vm: &vm, + cfg: &uid_config.cvm, + gpus: &GpuConfig::default(), + prepared: &prepared, + } + .build() + .unwrap(); + assert_eq!(process.user, "1000"); + assert!(!process.args.iter().any(|arg| arg == "sudo")); } } diff --git a/dstack/vmm/src/config.rs b/dstack/vmm/src/config.rs index 9d22b4040..f20ab3358 100644 --- a/dstack/vmm/src/config.rs +++ b/dstack/vmm/src/config.rs @@ -333,7 +333,10 @@ pub struct CvmConfig { pub qmp_socket: bool, /// GPU configuration pub gpu: GpuConfig, - /// Use sudo to run the VM + /// User the VM process runs as. Empty keeps the VMM's own privileges. + /// Supervisor prefixes QEMU with `sudo -u`; systemd sets `User=` on the + /// transient unit. Accepts a POSIX user name, a numeric UID, or sudo's + /// `#UID` form. pub user: String, /// Auto restart configuration @@ -824,43 +827,91 @@ fn validate_networking(networking: &Networking) -> Result<()> { /// First file descriptor systemd hands to a service, per the LISTEN_FDS /// convention shared by socket activation and `OpenFile=`. -pub const SD_LISTEN_FDS_START: u32 = 3; +pub(crate) const SD_LISTEN_FDS_START: u32 = 3; + +/// A `cvm.user` value after syntax checks, before looking the account up. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum UnitUser { + /// POSIX user name for `User=` / `sudo -u`. + Name(String), + /// Numeric UID. systemd takes the bare digits; sudo needs `#UID`. + Uid(u32), +} + +impl UnitUser { + /// Value for a systemd `User=` property. + pub(crate) fn systemd_value(&self) -> String { + match self { + Self::Name(name) => name.clone(), + Self::Uid(uid) => uid.to_string(), + } + } + + /// Value for `sudo -u`. + pub(crate) fn sudo_value(&self) -> String { + match self { + Self::Name(name) => name.clone(), + Self::Uid(uid) => format!("#{uid}"), + } + } +} -/// Validates a `open_file` path before it reaches a systemd unit property. +/// Parses a user name or numeric UID before it reaches sudo or a unit property. +/// +/// Accepts a POSIX user name, a bare decimal UID (systemd `User=`), or sudo's +/// `#UID` form. The charset for names excludes `%` and property separators so +/// the value cannot expand as a systemd specifier or inject extra syntax. +pub(crate) fn parse_unit_user(name: &str, user: &str) -> Result { + if user.is_empty() { + bail!("{name} must not be empty"); + } + if let Some(digits) = user.strip_prefix('#') { + if digits.is_empty() || !digits.bytes().all(|byte| byte.is_ascii_digit()) { + bail!("{name} must be '#' when it starts with '#': {user}"); + } + let uid = digits + .parse::() + .with_context(|| format!("{name} contains an out-of-range uid: {user}"))?; + return Ok(UnitUser::Uid(uid)); + } + if user.bytes().all(|byte| byte.is_ascii_digit()) { + let uid = user + .parse::() + .with_context(|| format!("{name} contains an out-of-range uid: {user}"))?; + return Ok(UnitUser::Uid(uid)); + } + if user.starts_with('-') { + bail!("{name} must not start with '-': {user}"); + } + if !user + .bytes() + .all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'_' | b'-' | b'.')) + { + bail!("{name} must contain only alphanumerics, '_', '-' and '.': {user}"); + } + Ok(UnitUser::Name(user.to_string())) +} + +pub(crate) fn validate_unit_user(name: &str, user: &str) -> Result<()> { + parse_unit_user(name, user).map(|_| ()) +} + +/// Validates an `open_file` path before it reaches a systemd unit property. /// /// systemd parses `OpenFile=` as `path:fdname:options` and expands `%` /// specifiers, so those characters would change the meaning of the property /// rather than name a device. The check is deliberately conservative: the only /// intended values are host device nodes such as `/dev/tap7498`. -/// Validates a user name before it reaches a systemd unit property. -/// -/// `User=` takes a user name or a numeric UID. The charset is kept to what a -/// POSIX user name can contain so the value cannot introduce a `%` specifier -/// expansion or extra property syntax. -pub fn validate_unit_user(name: &str, user: &str) -> Result<()> { - anyhow::ensure!(!user.is_empty(), "{name} must not be empty"); - anyhow::ensure!( - !user.starts_with('-'), - "{name} must not start with '-': {user}" - ); - anyhow::ensure!( - user.bytes() - .all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'_' | b'-' | b'.')), - "{name} must contain only alphanumerics, '_', '-' and '.': {user}" - ); - Ok(()) -} - -pub fn validate_open_file(name: &str, path: &str) -> Result<()> { - anyhow::ensure!( - path.starts_with('/'), - "{name} must be an absolute path: {path}" - ); - anyhow::ensure!( - path.bytes() - .all(|byte| byte.is_ascii_graphic() && !matches!(byte, b':' | b',' | b'%' | b'\\')), - "{name} must not contain whitespace or any of ':' ',' '%' '\\': {path}" - ); +pub(crate) fn validate_open_file(name: &str, path: &str) -> Result<()> { + if !path.starts_with('/') { + bail!("{name} must be an absolute path: {path}"); + } + if !path + .bytes() + .all(|byte| byte.is_ascii_graphic() && !matches!(byte, b':' | b',' | b'%' | b'\\')) + { + bail!("{name} must not contain whitespace or any of ':' ',' '%' '\\': {path}"); + } Ok(()) } @@ -1236,7 +1287,34 @@ mod tests { #[test] fn unit_user_names_are_validated() { validate_unit_user("cvm.user", "qemu-1.user_x").unwrap(); - for user in ["", "-qemu", "qemu:0", "qemu user", "%i", "qemu$"] { + assert_eq!( + parse_unit_user("cvm.user", "#1000").unwrap(), + UnitUser::Uid(1000) + ); + assert_eq!( + parse_unit_user("cvm.user", "1000").unwrap(), + UnitUser::Uid(1000) + ); + assert_eq!( + parse_unit_user("cvm.user", "#1000").unwrap().sudo_value(), + "#1000" + ); + assert_eq!( + parse_unit_user("cvm.user", "#1000") + .unwrap() + .systemd_value(), + "1000" + ); + for user in [ + "", + "#", + "#-1", + "-qemu", + "qemu:0", + "qemu user", + "%i", + "qemu$", + ] { validate_unit_user("cvm.user", user).unwrap_err(); } @@ -1247,6 +1325,8 @@ mod tests { .unwrap_err() .to_string() .contains("cvm.user")); + config.cvm.user = "#1000".into(); + config.validate().unwrap(); } #[test] diff --git a/dstack/vmm/src/process_manager.rs b/dstack/vmm/src/process_manager.rs index 5b6e2b752..7ffb7941e 100644 --- a/dstack/vmm/src/process_manager.rs +++ b/dstack/vmm/src/process_manager.rs @@ -15,7 +15,7 @@ use tokio::process::Command; use tokio::sync::RwLock; use tracing::warn; -use crate::config::{validate_open_file, validate_unit_user}; +use crate::config::{parse_unit_user, validate_open_file}; #[derive(Clone)] pub enum ProcessManager { @@ -416,15 +416,18 @@ impl SystemdProcessManager { args.push(format!("--setenv={key}={value}")); } if !config.user.is_empty() { - validate_unit_user("user", &config.user)?; - args.push(format!("--property=User={}", config.user)); + // ProcessConfig.user is already normalized to a systemd User= + // value (name or bare UID). Re-parse to reject anything that + // would still inject property syntax. + let user = parse_unit_user("user", &config.user)?; + args.push(format!("--property=User={}", user.systemd_value())); } // systemd opens these before exec and passes them in declaration // order starting at fd 3, which is what the QEMU netdev arguments // reference. No fdname and no `graceful` option: a missing device must // fail the unit instead of shifting every later descriptor by one. // - // systemd.exec(5): "The file or socket is opened by the service + // systemd.service(5): "The file or socket is opened by the service // manager and the file descriptor is passed to the service." The open // therefore happens with the manager's privileges, before the `User=` // drop that lands just before exec, so a root-owned chardev such as @@ -685,7 +688,12 @@ mod tests { .iter() .any(|arg| arg.contains("User="))); - for user in ["qemu:0", "qemu user", "%i", "-qemu"] { + let args = manager + .run_args(&test_config_as("1000", &[]), "unit.service") + .unwrap(); + assert!(args.iter().any(|arg| arg == "--property=User=1000")); + + for user in ["qemu:0", "qemu user", "%i", "-qemu", "#"] { manager .run_args(&test_config_as(user, &[]), "unit.service") .unwrap_err(); From f66f609ef0734e6763e241a83ffb4d65e11de230 Mon Sep 17 00:00:00 2001 From: Leechael Yim Date: Thu, 13 Aug 2026 00:10:48 +0800 Subject: [PATCH 07/13] fix(vmm): chown swtpm state under systemd User= --- dstack/vmm/Cargo.toml | 2 +- dstack/vmm/src/app/qemu.rs | 32 +++++++++++++++++++++++++++++++- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/dstack/vmm/Cargo.toml b/dstack/vmm/Cargo.toml index 941802b05..da08ca72f 100644 --- a/dstack/vmm/Cargo.toml +++ b/dstack/vmm/Cargo.toml @@ -25,7 +25,7 @@ sha2.workspace = true hex.workspace = true fs-err.workspace = true getrandom = { workspace = true, features = ["std"] } -nix = { workspace = true, features = ["user"] } +nix = { workspace = true, features = ["fs", "user"] } dirs.workspace = true which.workspace = true clap = { workspace = true, features = ["derive", "string"] } diff --git a/dstack/vmm/src/app/qemu.rs b/dstack/vmm/src/app/qemu.rs index 2f2a825fb..9161f178d 100644 --- a/dstack/vmm/src/app/qemu.rs +++ b/dstack/vmm/src/app/qemu.rs @@ -28,7 +28,7 @@ use anyhow::{bail, Context, Result}; use bon::Builder; use dstack_types::shared_filenames::HOST_SHARED_DISK_LABEL; use fs_err as fs; -use nix::unistd::{Uid, User}; +use nix::unistd::{chown, Uid, User}; use serde::Serialize; use std::collections::HashMap; use std::os::unix::fs::PermissionsExt; @@ -246,6 +246,14 @@ impl PreparedQemuLaunch { .context("tpm key provider requested but swtpm is not installed")?; let state_dir = workdir.swtpm_state_dir(); fs::create_dir_all(&state_dir).context("failed to create swtpm state directory")?; + // systemd drops privileges for the whole launcher unit, including + // swtpm. Hand the state directory over so socket creation and TPM + // state updates are not denied on a root-owned path. Existing + // files from earlier root-owned boots are included. + if !cfg.user.is_empty() && cfg.pm != ProcessManagerBackend::Supervisor { + let user = resolve_cvm_user(&cfg.user)?; + chown_tree_to_user(&state_dir, &user)?; + } let socket = workdir.swtpm_socket(); if socket.exists() { fs::remove_file(&socket).context("failed to remove stale swtpm socket")?; @@ -323,6 +331,28 @@ pub(crate) fn resolve_cvm_user(user: &str) -> Result { } } +/// Makes `path` and its contents writable by the unprivileged VM user. +/// +/// Under systemd the transient unit drops privileges before exec, so paths the +/// VMM created as root must be handed over before launch. Supervisor keeps +/// root for the launcher/swtpm path and only sudo's QEMU, so it does not need +/// this. +fn chown_tree_to_user(path: &Path, user: &User) -> Result<()> { + chown(path, Some(user.uid), Some(user.gid)) + .with_context(|| format!("failed to chown {}", path.display()))?; + if !path.is_dir() { + return Ok(()); + } + for entry in fs::read_dir(path) + .with_context(|| format!("failed to read directory {}", path.display()))? + { + let entry = + entry.with_context(|| format!("failed to read entry under {}", path.display()))?; + chown_tree_to_user(&entry.path(), user)?; + } + Ok(()) +} + fn prepare_shared_dir(workdir: &VmWorkDir) -> Result<()> { let shared_dir = workdir.shared_dir(); if !shared_dir.exists() { From bf9f80fb5927138cb385df748ea4452b6c838089 Mon Sep 17 00:00:00 2001 From: Leechael Yim Date: Thu, 13 Aug 2026 00:10:48 +0800 Subject: [PATCH 08/13] fix(vmm): stop open_file from inheriting host custom netdev --- dstack/vmm/src/app/network.rs | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/dstack/vmm/src/app/network.rs b/dstack/vmm/src/app/network.rs index 72cdb9c17..b8aee06cf 100644 --- a/dstack/vmm/src/app/network.rs +++ b/dstack/vmm/src/app/network.rs @@ -30,12 +30,15 @@ pub(crate) fn resolve_networking(networking: &Networking, cfg: &CvmConfig) -> Ne if !networking.dhcp_start.is_empty() { resolved.dhcp_start = networking.dhcp_start.clone(); } - if !networking.netdev.is_empty() { - resolved.netdev = networking.netdev.clone(); - } // Not merged from the host defaults: a pre-opened chardev names one - // device and belongs to exactly one NIC. + // device and belongs to exactly one NIC. When set, it also owns the + // netdev string (generated later from the fd number), so even an empty + // NIC netdev must replace a host-wide custom default. resolved.open_file = networking.open_file.clone(); + let replace_netdev = !networking.open_file.is_empty() || !networking.netdev.is_empty(); + if replace_netdev { + resolved.netdev = networking.netdev.clone(); + } resolved } @@ -198,6 +201,24 @@ mod tests { validate_resolved_network(&with_netdev).unwrap_err(); } + #[test] + fn open_file_does_not_inherit_host_custom_netdev() { + use rocket::figment::{providers::Format, providers::Toml, Figment}; + + let mut cfg: crate::config::Config = + Figment::from(Toml::string(crate::config::DEFAULT_CONFIG)) + .extract() + .unwrap(); + cfg.cvm.networking.mode = NetworkingMode::Custom; + cfg.cvm.networking.netdev = "tap,id=net0,ifname=legacy,script=no".into(); + + let nic = open_file_network("/dev/tap7498"); + let resolved = super::resolve_networking(&nic, &cfg.cvm); + assert_eq!(resolved.open_file, "/dev/tap7498"); + assert!(resolved.netdev.is_empty()); + validate_resolved_network(&resolved).unwrap(); + } + #[test] fn primary_mac_keeps_legacy_derivation_and_later_nics_are_distinct() { assert_eq!( From fd1b8b3c7c336f67d678a1a497298164ed60b276 Mon Sep 17 00:00:00 2001 From: Leechael Yim Date: Thu, 13 Aug 2026 00:10:48 +0800 Subject: [PATCH 09/13] fix(vmm): omit empty open_file from networking json --- dstack/vmm/src/config.rs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/dstack/vmm/src/config.rs b/dstack/vmm/src/config.rs index f20ab3358..63a95068a 100644 --- a/dstack/vmm/src/config.rs +++ b/dstack/vmm/src/config.rs @@ -963,7 +963,7 @@ pub struct Networking { /// rejected on every other launch path instead of being silently dropped: /// QEMU would otherwise open an unrelated fd and attach the guest to the /// wrong network. - #[serde(default)] + #[serde(default, skip_serializing_if = "String::is_empty")] pub open_file: String, } @@ -1329,6 +1329,22 @@ mod tests { config.validate().unwrap(); } + #[test] + fn empty_open_file_is_omitted_from_json() { + let networking = Networking { + mode: NetworkingMode::User, + bridge: String::new(), + mac_prefix: String::new(), + net: String::new(), + dhcp_start: String::new(), + restrict: false, + netdev: String::new(), + open_file: String::new(), + }; + let value = serde_json::to_value(&networking).unwrap(); + assert!(value.get("open_file").is_none()); + } + #[test] fn config_validation_rejects_invalid_static_invariants() { let mut config = default_config(); From c2d970a345fd188430e4e11232d9c7e409929ee2 Mon Sep 17 00:00:00 2001 From: Leechael Yim Date: Thu, 13 Aug 2026 00:10:48 +0800 Subject: [PATCH 10/13] fix(vmm): clarify one-shot dry-run limits for open_file and user --- dstack/vmm/src/one_shot.rs | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/dstack/vmm/src/one_shot.rs b/dstack/vmm/src/one_shot.rs index deb15f31d..09b13896a 100644 --- a/dstack/vmm/src/one_shot.rs +++ b/dstack/vmm/src/one_shot.rs @@ -290,13 +290,12 @@ Compose file content (first 200 chars): ); } - if !dry_run - && resolved_networks(&manifest, &config.cvm) - .iter() - .any(|network| !network.open_file.is_empty()) - { + let needs_open_files = resolved_networks(&manifest, &config.cvm) + .iter() + .any(|network| !network.open_file.is_empty()); + if !dry_run && needs_open_files { anyhow::bail!( - "one-shot execution cannot pass pre-opened file descriptors to QEMU; run the VMM server with cvm.pm = \"systemd\" or use --dry-run" + "one-shot execution cannot pass pre-opened file descriptors to QEMU; run the VMM server with cvm.pm = \"systemd\"" ); } @@ -322,21 +321,35 @@ Compose file content (first 200 chars): println!("# QEMU Command:"); println!("{}", full_command.join(" ")); - if !dry_run && !process_config.user.is_empty() { + let needs_systemd_user = !process_config.user.is_empty(); + if !dry_run && needs_systemd_user { // Privileges are dropped by the systemd unit, which one-shot mode does // not create, and the command carries no sudo prefix either. Running it // here would start QEMU with the VMM's own privileges. anyhow::bail!( - "one-shot execution cannot drop privileges to cvm.user with cvm.pm = \"systemd\" or \"auto\"; use --dry-run or cvm.pm = \"supervisor\"" + "one-shot execution cannot drop privileges to cvm.user with cvm.pm = \"systemd\" or \"auto\"; use cvm.pm = \"supervisor\" or run the VMM server" ); } if dry_run { println!("# Dry run mode - QEMU command not executed"); - println!( - "# To execute, run: --one-shot {} (without --dry-run)", - vm_config_path - ); + if needs_open_files { + println!( + "# This command needs pre-opened file descriptors from systemd OpenFile=; \ + run the VMM server with cvm.pm = \"systemd\" instead of removing --dry-run" + ); + } else if needs_systemd_user { + println!( + "# This command expects systemd User={}; run the VMM server with cvm.pm = \"systemd\" \ + or \"auto\", or set cvm.pm = \"supervisor\" so one-shot can use sudo", + process_config.user + ); + } else { + println!( + "# To execute, run: --one-shot {} (without --dry-run)", + vm_config_path + ); + } } else { println!("# Executing QEMU..."); From 9dfa196a0b8be36ea2677a2556b8f8f1ffa26f23 Mon Sep 17 00:00:00 2001 From: Leechael Yim Date: Thu, 13 Aug 2026 00:10:48 +0800 Subject: [PATCH 11/13] docs: document OpenFile and User on the systemd process manager --- docs/experimental-systemd-vm-processes.md | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/docs/experimental-systemd-vm-processes.md b/docs/experimental-systemd-vm-processes.md index e2a7549c1..4fd693426 100644 --- a/docs/experimental-systemd-vm-processes.md +++ b/docs/experimental-systemd-vm-processes.md @@ -57,8 +57,20 @@ KillSignal=SIGTERM SendSIGKILL=yes TimeoutStopSec= Restart=no +User= # when cvm.user is set +OpenFile=/dev/tapN # one entry per NIC with networking.open_file ``` +`User=` replaces the Supervisor `sudo -u` path. systemd opens each `OpenFile=` +path with the manager's privileges and hands the descriptors to the service in +declaration order starting at fd 3, which is what QEMU's generated +`-netdev tap,id=netN,fd=M` arguments expect. That combination needs systemd +253 or newer. + +When both are set, the chardev stays root-owned on the host and the unit still +runs QEMU unprivileged. For software-TPM VMs the whole launcher unit runs as +`cvm.user`, so the VMM chowns the swtpm state directory before start. + The existing launcher remains responsible for swtpm readiness and graceful child shutdown. systemd owns the final cgroup lifetime. A stop request is submitted asynchronously so the VMM can report a VM as stopping while QEMU is @@ -88,8 +100,10 @@ atomic property handling and event-driven state updates. ## Limitations -- The host must run systemd with support for `ExitType=cgroup` and - `StandardOutput=append:`. +- The host must run systemd 253+ with support for `OpenFile=`, + `ExitType=cgroup`, and `StandardOutput=append:`. +- `networking.open_file` is manifest-only, requires `mode = "custom"`, and is + rejected with Supervisor, one-shot execution, and swtpm-backed VMs. - The VMM must be authorized to create and stop system services. - Transient services inherit the systemd manager environment rather than the VMM environment. Variables in `ProcessConfig.env` are forwarded; unrelated From 8cd77b2af6d7208c3ac1de43941d596459ef2765 Mon Sep 17 00:00:00 2001 From: Leechael Yim Date: Thu, 13 Aug 2026 01:47:40 +0800 Subject: [PATCH 12/13] fix(vmm): stop swtpm chown from following symlinks chown_tree_to_user handed the swtpm state directory to the unprivileged cvm.user before a systemd-managed launch using chown(2) plus Path::is_dir(), both of which follow symlinks, and recursed through fs::read_dir. The state directory is owned by (and writable by) cvm.user between boots, so a symlink planted there was followed on the next launch, letting code already running as cvm.user redirect a root chown onto an arbitrary host path (CWE-59) -- an escalation along the exact path the privilege drop is meant to contain. Walk the tree without following symlinks: fchownat the top path with AT_SYMLINK_NOFOLLOW, then descend only through directories opened with O_NOFOLLOW|O_DIRECTORY, performing every chown and every openat relative to the trusted directory fd instead of by re-resolving a path. Operating relative to an already-opened fd also closes the TOCTOU where an intermediate path component is swapped for a symlink between check and use. Add tests that plant a dangling symlink and a symlinked directory in the tree and assert the walk succeeds by chowning the link itself rather than chasing the missing target; both fail against the previous implementation. Enable the nix "dir" feature for the directory-fd traversal. --- dstack/vmm/Cargo.toml | 2 +- dstack/vmm/src/app/qemu.rs | 132 +++++++++++++++++++++++++++++++++---- 2 files changed, 122 insertions(+), 12 deletions(-) diff --git a/dstack/vmm/Cargo.toml b/dstack/vmm/Cargo.toml index da08ca72f..4efbfc1b1 100644 --- a/dstack/vmm/Cargo.toml +++ b/dstack/vmm/Cargo.toml @@ -25,7 +25,7 @@ sha2.workspace = true hex.workspace = true fs-err.workspace = true getrandom = { workspace = true, features = ["std"] } -nix = { workspace = true, features = ["fs", "user"] } +nix = { workspace = true, features = ["fs", "user", "dir"] } dirs.workspace = true which.workspace = true clap = { workspace = true, features = ["derive", "string"] } diff --git a/dstack/vmm/src/app/qemu.rs b/dstack/vmm/src/app/qemu.rs index 9161f178d..7198b8796 100644 --- a/dstack/vmm/src/app/qemu.rs +++ b/dstack/vmm/src/app/qemu.rs @@ -28,9 +28,14 @@ use anyhow::{bail, Context, Result}; use bon::Builder; use dstack_types::shared_filenames::HOST_SHARED_DISK_LABEL; use fs_err as fs; -use nix::unistd::{chown, Uid, User}; +use nix::dir::Dir; +use nix::errno::Errno; +use nix::fcntl::{openat, AtFlags, OFlag}; +use nix::sys::stat::Mode; +use nix::unistd::{fchownat, Uid, User}; use serde::Serialize; use std::collections::HashMap; +use std::os::fd::{AsRawFd, RawFd}; use std::os::unix::fs::PermissionsExt; use std::{ fs::Permissions, @@ -331,24 +336,80 @@ pub(crate) fn resolve_cvm_user(user: &str) -> Result { } } -/// Makes `path` and its contents writable by the unprivileged VM user. +const DIR_OFLAGS: OFlag = OFlag::O_RDONLY + .union(OFlag::O_NOFOLLOW) + .union(OFlag::O_DIRECTORY) + .union(OFlag::O_CLOEXEC); + +/// Makes `path` and its contents owned by the unprivileged VM user. /// /// Under systemd the transient unit drops privileges before exec, so paths the /// VMM created as root must be handed over before launch. Supervisor keeps /// root for the launcher/swtpm path and only sudo's QEMU, so it does not need /// this. +/// +/// The walk never follows a symlink: every entry is chowned and every descent +/// happens relative to an `O_NOFOLLOW`-opened directory fd. The state directory +/// is writable by the unprivileged user between boots, so a symlink planted +/// there must not be able to redirect a root chown onto an arbitrary host path +/// (CWE-59). fn chown_tree_to_user(path: &Path, user: &User) -> Result<()> { - chown(path, Some(user.uid), Some(user.gid)) - .with_context(|| format!("failed to chown {}", path.display()))?; - if !path.is_dir() { - return Ok(()); - } - for entry in fs::read_dir(path) - .with_context(|| format!("failed to read directory {}", path.display()))? - { + // Chown the top path itself without following a symlink. + fchownat( + None, + path, + Some(user.uid), + Some(user.gid), + AtFlags::AT_SYMLINK_NOFOLLOW, + ) + .with_context(|| format!("failed to chown {}", path.display()))?; + + // Open the directory itself without following symlinks. A non-directory or + // a symlink has nothing to descend into and was already chowned above. + let dir_fd = match openat(None, path, DIR_OFLAGS, Mode::empty()) { + Ok(fd) => fd, + Err(Errno::ENOTDIR | Errno::ELOOP) => return Ok(()), + Err(err) => return Err(err).with_context(|| format!("failed to open {}", path.display())), + }; + chown_dir_contents(dir_fd, path, user) +} + +/// Chowns every entry reachable through `dir_fd`, taking ownership of it (the +/// fd is closed when the `Dir` drops). Every chown and descent is performed +/// relative to the trusted fd rather than by re-resolving a path, so a symlink +/// anywhere below `path` cannot redirect the walk outside the tree. +fn chown_dir_contents(dir_fd: RawFd, path: &Path, user: &User) -> Result<()> { + let mut dir = Dir::from_fd(dir_fd) + .with_context(|| format!("failed to read directory {}", path.display()))?; + let raw = dir.as_raw_fd(); + for entry in dir.iter() { let entry = entry.with_context(|| format!("failed to read entry under {}", path.display()))?; - chown_tree_to_user(&entry.path(), user)?; + let name = entry.file_name(); + let bytes = name.to_bytes(); + if bytes == b"." || bytes == b".." { + continue; + } + // Chown the entry relative to the trusted dir fd, never following a + // symlink, so a planted link cannot redirect the chown to its target. + fchownat( + Some(raw), + name, + Some(user.uid), + Some(user.gid), + AtFlags::AT_SYMLINK_NOFOLLOW, + ) + .with_context(|| format!("failed to chown entry under {}", path.display()))?; + // Descend only into a real subdirectory, opened without following + // symlinks. ELOOP means the entry is a symlink; ENOTDIR a regular file. + match openat(Some(raw), name, DIR_OFLAGS, Mode::empty()) { + Ok(child) => chown_dir_contents(child, path, user)?, + Err(Errno::ENOTDIR | Errno::ELOOP) => {} + Err(err) => { + return Err(err) + .with_context(|| format!("failed to open entry under {}", path.display())) + } + } } Ok(()) } @@ -1457,4 +1518,53 @@ mod tests { assert_eq!(process.user, "1000"); assert!(!process.args.iter().any(|arg| arg == "sudo")); } + + fn chown_test_user() -> nix::unistd::User { + // The swtpm chown must succeed against a real account. Targeting the + // current uid keeps the chown a permitted no-op whether the suite runs + // as root or unprivileged, so the tests below assert traversal shape, + // not privilege. + nix::unistd::User::from_uid(nix::unistd::Uid::current()) + .unwrap() + .expect("current uid resolves to a user") + } + + #[test] + fn chown_tree_does_not_follow_a_symlink() { + // A symlink whose target does not exist must be chowned as the link + // itself. Following it would chase the missing target and fail — the + // exact primitive that let a planted link redirect a root chown onto + // an arbitrary host path. + let dir = tempfile::tempdir().unwrap(); + std::os::unix::fs::symlink("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/nonexistent/dstack-chown-victim", dir.path().join("link")) + .unwrap(); + super::chown_tree_to_user(dir.path(), &chown_test_user()) + .expect("must chown the symlink itself, not follow it"); + } + + #[test] + fn chown_tree_does_not_descend_into_a_symlinked_dir() { + // A symlink to a directory must not be walked into: the pointed-to + // directory holds a dangling link, so descending would chase it and + // fail. + let dir = tempfile::tempdir().unwrap(); + let outside = tempfile::tempdir().unwrap(); + std::os::unix::fs::symlink("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/nonexistent/inner-victim", outside.path().join("inner")) + .unwrap(); + std::os::unix::fs::symlink(outside.path(), dir.path().join("dirlink")).unwrap(); + super::chown_tree_to_user(dir.path(), &chown_test_user()) + .expect("must not descend into a symlinked directory"); + } + + #[test] + fn chown_tree_still_walks_a_real_tree() { + // Regression guard: the hardening must keep chowning real nested + // entries rather than stop at the top directory. + let dir = tempfile::tempdir().unwrap(); + std::fs::create_dir(dir.path().join("sub")).unwrap(); + std::fs::write(dir.path().join("sub/file"), b"x").unwrap(); + std::fs::write(dir.path().join("top"), b"y").unwrap(); + super::chown_tree_to_user(dir.path(), &chown_test_user()) + .expect("must chown a normal tree"); + } } From 22a100fb3bddd1edcf8b4e9a77ad89b65e4c950a Mon Sep 17 00:00:00 2001 From: Leechael Yim Date: Thu, 13 Aug 2026 09:17:16 +0800 Subject: [PATCH 13/13] refactor(vmm): make open_file/netdev handling explicit in resolve_networking The netdev merge relied on an implicit trick: it entered a `replace_netdev` branch whenever a NIC set open_file OR netdev, then assigned `networking.netdev.clone()` -- which for a pure open_file NIC is empty, so the assignment happened to clear the inherited host netdev. The condition tested open_file while the assignment only used netdev, so the clear worked by coincidence of emptiness rather than by stated intent. Split it into two mutually exclusive branches: an open_file NIC with no netdev explicitly clears the inherited host netdev (its real netdev is generated from the fd number later); a NIC with a netdev takes that netdev; neither inherits. Behaviour is unchanged across all four open_file/netdev combinations, including open_file+netdev set together: that pair is still carried through so validate_resolved_network rejects it as mutually exclusive, rather than letting open_file silently win and drop the user's netdev. --- dstack/vmm/src/app/network.rs | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/dstack/vmm/src/app/network.rs b/dstack/vmm/src/app/network.rs index b8aee06cf..e19d4942a 100644 --- a/dstack/vmm/src/app/network.rs +++ b/dstack/vmm/src/app/network.rs @@ -30,15 +30,21 @@ pub(crate) fn resolve_networking(networking: &Networking, cfg: &CvmConfig) -> Ne if !networking.dhcp_start.is_empty() { resolved.dhcp_start = networking.dhcp_start.clone(); } - // Not merged from the host defaults: a pre-opened chardev names one - // device and belongs to exactly one NIC. When set, it also owns the - // netdev string (generated later from the fd number), so even an empty - // NIC netdev must replace a host-wide custom default. + // A pre-opened chardev names one host device and belongs to exactly one + // NIC, so it is taken from the NIC verbatim and never inherited from the + // host defaults. resolved.open_file = networking.open_file.clone(); - let replace_netdev = !networking.open_file.is_empty() || !networking.netdev.is_empty(); - if replace_netdev { + if !networking.open_file.is_empty() && networking.netdev.is_empty() { + // The chardev generates this NIC's netdev from the fd number later, so + // drop any inherited host netdev rather than leak a stale default. + resolved.netdev = String::new(); + } else if !networking.netdev.is_empty() { + // An explicit NIC netdev overrides the host default. A netdev set + // together with open_file is kept here, not dropped, so that + // validate_resolved_network rejects the conflicting pair. resolved.netdev = networking.netdev.clone(); } + // Neither set: inherit the host netdev unchanged. resolved }