From 87490d321bd2422fd79ffd981914efc9e8b7de24 Mon Sep 17 00:00:00 2001 From: Aaqa Ishtyaq Date: Fri, 17 Jul 2026 15:57:29 +0530 Subject: [PATCH] feat: allow-list jailed vfio devices Extend the strict v1 manifest with an optional VFIO-only device list. Accept only the control node and canonical numeric IOMMU-group paths, require exact in-jail destinations, and reject duplicates, aliases, and mounts that overlap the reserved device tree. Resolve each selected host node as a real character device before the pivot, then recreate its major/minor identity inside the private jail. This avoids bind-mounting host /dev or changing host device ownership. CPU-only manifests remain compatible because the new list defaults empty. Document the boundary and dedicated hardware-runner coverage, and bump the binary package version to 0.1.10. Linux amd64 container tests pass with 13 unit tests, strict Clippy, and an optimized release build. Signed-off-by: Aaqa Ishtyaq --- ARCHITECTURE.md | 15 +++-- Cargo.lock | 2 +- Cargo.toml | 2 +- README.md | 14 +++- docs/testing.md | 4 ++ src/linux/jail.rs | 60 ++++++++++++++++- src/linux/mod.rs | 3 +- src/manifest.rs | 166 ++++++++++++++++++++++++++++++++++++++++++++++ 8 files changed, 253 insertions(+), 13 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 07ec5a9..796ee15 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -17,8 +17,9 @@ JSON manifest -> manifest validation -> Linux launch workflow its pure invariants. - `src/linux/mod.rs` owns operation order. It has no low-level mount, cgroup, or credential details. -- `src/linux/jail.rs` stages the root, applies declared bind mounts, performs - `pivot_root`, and creates only the KVM/TUN device nodes required by CH. +- `src/linux/jail.rs` stages the root, applies declared bind mounts, resolves + allow-listed VFIO character identities, performs `pivot_root`, and creates + only the KVM/TUN/entropy and declared VFIO nodes required by CH. - `src/linux/cgroup.rs` owns cgroup-v2 discovery, controller delegation through `cgroup.subtree_control`, limit writes, and process attachment. - `src/linux/process.rs` owns namespaces, resource limits, descriptor and @@ -38,8 +39,10 @@ Adopted or strengthened here: - strict versioned manifest rather than caller-provided arbitrary arguments; - bounded machine IDs, non-root target identity, path validation, and forced CH seccomp; -- mount and PID namespaces, `pivot_root`, netns join, KVM/TUN nodes, rlimits, - cgroup-v2 limits, and descriptor/environment sanitization; +- mount and PID namespaces, `pivot_root`, netns join, KVM/TUN/entropy nodes, + rlimits, cgroup-v2 limits, and descriptor/environment sanitization; +- exact per-machine VFIO control/IOMMU-group allow-lists whose character + identities are recreated inside the jail instead of exposing host `/dev`; - cgroup-v2 controller availability checks and recursive delegation before a leaf cgroup is configured; and - `close_range` with an `ENOSYS` fallback, rather than an unconditional @@ -52,8 +55,8 @@ Intentional differences: - the API socket is created by CH inside the jail, not passed as a listener FD or bind mounted from the host; - Firecracker-specific userfaultfd support is not exposed; and -- `/dev/urandom` is not created because CH uses host randomness through the - kernel, not a jailed device path. +- VFIO groups are supplied by the host allocator; this launcher validates the + boundary but does not discover devices or decide assignment policy. ## Remaining hardening work diff --git a/Cargo.lock b/Cargo.lock index 7463eaf..8b34022 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -112,7 +112,7 @@ checksum = "c8d4a3bb8b1e0c1050499d1815f5ab16d04f0959b233085fb31653fbfc9d98f9" [[package]] name = "cloud-hypervisor-jailer" -version = "0.1.6" +version = "0.1.10" dependencies = [ "anyhow", "clap", diff --git a/Cargo.toml b/Cargo.toml index 4e369ec..e556de7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "cloud-hypervisor-jailer" -version = "0.1.6" +version = "0.1.10" edition = "2024" rust-version = "1.85" description = "Cloud Hypervisor sandbox launcher for Depot" diff --git a/README.md b/README.md index 1827444..7c0d8b1 100644 --- a/README.md +++ b/README.md @@ -22,7 +22,9 @@ On Linux, `launch` requires root and then: - mounts only declared non-symlink sources; - joins a pre-created network namespace when requested; - configures the declared cgroup-v2 values and resource limits; -- creates jailed KVM and TUN device nodes; +- creates jailed KVM, TUN, and entropy device nodes; +- recreates only explicitly declared canonical VFIO control/group character + devices, without bind-mounting host `/dev` or changing host device ownership; - creates a PID namespace when requested; - clears inherited environment and non-standard file descriptors; - drops the complete capability bounding set, sets `no_new_privs`, and changes @@ -33,7 +35,7 @@ On Linux, `launch` requires root and then: flowchart LR O["Host orchestrator"] -->|"versioned JSON manifest"| V["validate"] V -->|"pure checks"| L["launch as root"] - L --> J["jail\nmount namespace • bind mounts • pivot_root • KVM/TUN"] + L --> J["jail\nmount namespace • bind mounts • pivot_root • KVM/TUN/VFIO"] L --> C["cgroup v2\ncontroller delegation • limits • lease"] L --> P["process\nnetns/PID ns • rlimits • FD/env cleanup • UID/GID"] J --> CH["Cloud Hypervisor\n--seccomp true"] @@ -94,6 +96,14 @@ in [`src/manifest.rs`](src/manifest.rs) for the current canonical contract. Trea paths and arguments as host-orchestrator-controlled inputs; this is not a safe interface for tenant-provided configuration. +VFIO passthrough is opt-in per manifest. `devices` may contain only the exact +control path `/dev/vfio/vfio` and canonical numeric IOMMU-group paths such as +`/dev/vfio/42`; each destination must be the identical sandbox-relative path. +Group entries require the control device. The jailer verifies every source is +a real character device before pivoting and recreates its major/minor identity +inside the private jail. Arbitrary host devices, symlinks, path aliases, +destination remapping, duplicate nodes, and mounts over `dev/` are rejected. + ## Status The launcher is tested for manifest validation and compiled/tested on native diff --git a/docs/testing.md b/docs/testing.md index 7c271c8..60103ae 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -26,6 +26,10 @@ release used by the consuming orchestrator: for reconciler retry. 5. Create/restore a CH snapshot through the host orchestrator; verify the jailer does not broaden storage or device visibility. +6. On a dedicated VFIO host, pass one complete IOMMU group and verify the VMM + sees only `/dev/vfio/vfio` plus that numeric group. Also verify arbitrary + devices, a second unassigned group, symlinks, and mounts over `/dev` fail + before Cloud Hypervisor execs. The workflow is deliberately manual until a hardened dedicated runner exists. Never run it on a shared developer host or a runner that also contains tenant diff --git a/src/linux/jail.rs b/src/linux/jail.rs index f9408b1..323a124 100644 --- a/src/linux/jail.rs +++ b/src/linux/jail.rs @@ -3,12 +3,14 @@ use std::env; use std::ffi::CString; use std::fs::{self, OpenOptions}; +use std::os::unix::fs::FileTypeExt; +use std::os::unix::fs::MetadataExt; use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; use std::path::Path; use anyhow::{Context, Result, bail}; -use crate::manifest::{Manifest, Mount}; +use crate::manifest::{Device, Manifest, Mount}; use super::util::{c_path, syscall_ok}; @@ -49,6 +51,50 @@ pub(super) fn mount_resources(manifest: &Manifest) -> Result<()> { Ok(()) } +#[derive(Debug)] +pub(super) struct ResolvedDevice { + destination: std::path::PathBuf, + major: u32, + minor: u32, +} + +/// Resolve the device identity while the host `/dev` tree is still visible. +/// Manifest validation has already constrained every path to canonical VFIO +/// names; this second check proves each selected entry is a real character +/// device rather than trusting a regular file at an allowed-looking path. +pub(super) fn resolve_devices(manifest: &Manifest) -> Result> { + manifest + .devices + .iter() + .map(resolve_device) + .collect::>>() +} + +fn resolve_device(device: &Device) -> Result { + let metadata = fs::symlink_metadata(&device.source) + .with_context(|| format!("stat device source {}", device.source.display()))?; + if metadata.file_type().is_symlink() || !metadata.file_type().is_char_device() { + bail!( + "device source is not a character device: {}", + device.source.display() + ); + } + let canonical = fs::canonicalize(&device.source) + .with_context(|| format!("canonicalize device source {}", device.source.display()))?; + if canonical != device.source { + bail!( + "device source changed during resolution: {}", + device.source.display() + ); + } + let device_id = metadata.rdev(); + Ok(ResolvedDevice { + destination: device.destination.0.clone(), + major: libc::major(device_id) as u32, + minor: libc::minor(device_id) as u32, + }) +} + pub(super) fn pivot_into_jail(root: &Path) -> Result<()> { mount_call(Some(root), root, libc::MS_BIND | libc::MS_REC)?; env::set_current_dir(root).context("enter jail root")?; @@ -66,14 +112,24 @@ pub(super) fn pivot_into_jail(root: &Path) -> Result<()> { syscall_ok(unsafe { libc::rmdir(old_root.as_ptr()) }).context("remove old root") } -pub(super) fn create_device_nodes(uid: u32, gid: u32) -> Result<()> { +pub(super) fn create_device_nodes(uid: u32, gid: u32, devices: &[ResolvedDevice]) -> Result<()> { fs::create_dir_all("/dev/net").context("create jailed dev directory")?; + if !devices.is_empty() { + fs::create_dir_all("/dev/vfio").context("create jailed VFIO directory")?; + } create_character_device(Path::new("/dev/kvm"), 10, 232)?; create_character_device(Path::new("/dev/net/tun"), 10, 200)?; // Cloud Hypervisor's default virtio-rng device reads from /dev/urandom. // Expose only this non-blocking entropy device; guest workloads never // receive the host /dev filesystem. create_character_device(Path::new("/dev/urandom"), 1, 9)?; + for device in devices { + let destination = Path::new("/").join(&device.destination); + create_character_device(&destination, device.major, device.minor) + .with_context(|| format!("create jailed device {}", destination.display()))?; + chown_path(&destination, uid, gid) + .with_context(|| format!("chown jailed device {}", destination.display()))?; + } for path in [ Path::new("/"), Path::new("/dev/kvm"), diff --git a/src/linux/mod.rs b/src/linux/mod.rs index ec14b55..0d95319 100644 --- a/src/linux/mod.rs +++ b/src/linux/mod.rs @@ -20,6 +20,7 @@ pub(crate) fn launch(manifest: &Manifest) -> Result<()> { .map(File::open) .transpose() .context("open network namespace")?; + let devices = jail::resolve_devices(manifest)?; jail::prepare_root(manifest)?; jail::enter_mount_namespace()?; @@ -31,7 +32,7 @@ pub(crate) fn launch(manifest: &Manifest) -> Result<()> { // effective capabilities needed for pivot_root and device setup. process::drop_capability_bounding_set()?; jail::pivot_into_jail(&manifest.root)?; - jail::create_device_nodes(manifest.uid, manifest.gid)?; + jail::create_device_nodes(manifest.uid, manifest.gid, &devices)?; if let Some(netns) = netns { process::join_network_namespace(netns.as_raw_fd())?; } diff --git a/src/manifest.rs b/src/manifest.rs index 0a7541a..dce5860 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -33,6 +33,8 @@ pub(crate) struct Manifest { pub(crate) resource_limits: ResourceLimits, pub(crate) api_socket: SandboxPath, pub(crate) mounts: Vec, + #[serde(default)] + pub(crate) devices: Vec, } #[derive(Debug, Default, Deserialize)] @@ -76,6 +78,18 @@ pub(crate) struct Mount { pub(crate) read_only: bool, } +/// A host device that may be recreated inside the jail. +/// +/// The v1 contract deliberately limits this to VFIO. The launcher resolves +/// the character-device identity from the trusted host path before pivoting; +/// it never bind-mounts the host `/dev` tree or changes host device ownership. +#[derive(Debug, Deserialize)] +#[serde(deny_unknown_fields)] +pub(crate) struct Device { + pub(crate) source: PathBuf, + pub(crate) destination: SandboxPath, +} + #[derive(Debug, Deserialize)] #[serde(transparent)] pub(crate) struct SandboxPath(pub(crate) PathBuf); @@ -102,6 +116,18 @@ pub(crate) enum ManifestError { SourceNotAbsolute(String), #[error("duplicate sandbox destination {0}")] DuplicateDestination(String), + #[error("mount destination {0} overlaps the jailer's reserved device tree")] + ReservedMountDestination(String), + #[error("device source {0} is not an allow-listed VFIO path")] + InvalidDeviceSource(String), + #[error("VFIO device destination must match its source: {0}")] + InvalidDeviceDestination(String), + #[error("duplicate VFIO device {0}")] + DuplicateDevice(String), + #[error("VFIO group devices require the /dev/vfio/vfio control device")] + MissingVFIOControlDevice, + #[error("a jail may contain at most 65 VFIO devices")] + TooManyDevices, #[error("resource limit no_file must be between 3 and 1048576")] InvalidNoFileLimit, #[error("cgroup property {0} is not an allow-listed cgroup v2 file")] @@ -191,14 +217,80 @@ impl Manifest { } validate_sandbox_path(&mount.destination)?; let destination = mount.destination.0.display().to_string(); + if mount.destination.0.starts_with("dev") { + return Err(ManifestError::ReservedMountDestination(destination)); + } if !destinations.insert(destination.clone()) { return Err(ManifestError::DuplicateDestination(destination)); } } + if self.devices.len() > 65 { + return Err(ManifestError::TooManyDevices); + } + let mut devices = HashSet::new(); + let mut has_control = false; + let mut has_group = false; + for device in &self.devices { + let source = validate_vfio_source(&device.source)?; + let expected_destination = source + .strip_prefix("/") + .expect("validated VFIO source is absolute"); + validate_sandbox_path(&device.destination)?; + if device.destination.0 != expected_destination { + return Err(ManifestError::InvalidDeviceDestination( + device.destination.0.display().to_string(), + )); + } + if !devices.insert(source.to_owned()) { + return Err(ManifestError::DuplicateDevice(source.display().to_string())); + } + if !destinations.insert(device.destination.0.display().to_string()) { + return Err(ManifestError::DuplicateDestination( + device.destination.0.display().to_string(), + )); + } + if source == std::path::Path::new("/dev/vfio/vfio") { + has_control = true; + } else { + has_group = true; + } + } + if has_group && !has_control { + return Err(ManifestError::MissingVFIOControlDevice); + } Ok(()) } } +fn validate_vfio_source(path: &std::path::Path) -> Result<&std::path::Path, ManifestError> { + if !is_clean_absolute_host_path(path) { + return Err(ManifestError::InvalidDeviceSource( + path.display().to_string(), + )); + } + if path == std::path::Path::new("/dev/vfio/vfio") { + return Ok(path); + } + let Some(group) = path + .strip_prefix("/dev/vfio") + .ok() + .and_then(|relative| relative.to_str()) + else { + return Err(ManifestError::InvalidDeviceSource( + path.display().to_string(), + )); + }; + if group.is_empty() + || !group.bytes().all(|byte| byte.is_ascii_digit()) + || (group.len() > 1 && group.starts_with('0')) + { + return Err(ManifestError::InvalidDeviceSource( + path.display().to_string(), + )); + } + Ok(path) +} + fn is_clean_absolute_host_path(path: &std::path::Path) -> bool { path.is_absolute() && !path.components().any(|component| { @@ -251,6 +343,7 @@ pub(crate) mod tests { resource_limits: ResourceLimits::default(), api_socket: SandboxPath("run/ch.sock".into()), mounts: vec![], + devices: vec![], } } @@ -315,4 +408,77 @@ pub(crate) mod tests { Err(ManifestError::PrivilegedIdentity) )); } + + #[test] + fn accepts_exact_vfio_control_and_group_devices() { + let mut manifest = valid_manifest(); + manifest.devices = vec![ + Device { + source: "/dev/vfio/vfio".into(), + destination: SandboxPath("dev/vfio/vfio".into()), + }, + Device { + source: "/dev/vfio/42".into(), + destination: SandboxPath("dev/vfio/42".into()), + }, + ]; + manifest.validate().unwrap(); + } + + #[test] + fn rejects_arbitrary_devices_and_vfio_path_aliases() { + for source in [ + "/dev/null", + "/dev/vfio/../../mem", + "/dev/vfio/01", + "/dev/vfio/1/2", + ] { + let mut manifest = valid_manifest(); + manifest.devices = vec![Device { + source: source.into(), + destination: SandboxPath("dev/vfio/1".into()), + }]; + assert!(matches!( + manifest.validate(), + Err(ManifestError::InvalidDeviceSource(_)) + )); + } + } + + #[test] + fn rejects_destination_remapping_and_missing_control_device() { + let mut manifest = valid_manifest(); + manifest.devices = vec![Device { + source: "/dev/vfio/42".into(), + destination: SandboxPath("dev/vfio/41".into()), + }]; + assert!(matches!( + manifest.validate(), + Err(ManifestError::InvalidDeviceDestination(_)) + )); + + let mut manifest = valid_manifest(); + manifest.devices = vec![Device { + source: "/dev/vfio/42".into(), + destination: SandboxPath("dev/vfio/42".into()), + }]; + assert!(matches!( + manifest.validate(), + Err(ManifestError::MissingVFIOControlDevice) + )); + } + + #[test] + fn rejects_mounts_over_the_reserved_device_tree() { + let mut manifest = valid_manifest(); + manifest.mounts = vec![Mount { + source: "/var/lib/machined/device-shadow".into(), + destination: SandboxPath("dev/vfio".into()), + read_only: false, + }]; + assert!(matches!( + manifest.validate(), + Err(ManifestError::ReservedMountDestination(_)) + )); + } }