test markus pr2

Signed-off-by: Markus Rudy <mr@edgeless.systems>

Cargo.toml

@@ -159,6 +159,7 @@ netns-rs = "0.1.0"
 # Note: nix needs to stay sync'd with libs versions
 nix = "0.26.4"
 oci-spec = { version = "0.8.1", features = ["runtime"] }
+pathrs = "0.2.4"
 opentelemetry = { version = "0.17.0", features = ["rt-tokio"] }
 procfs = "0.12.0"
 prometheus = { version = "0.14.0", features = ["process"] }

src/agent/Cargo.toml

@@ -81,6 +81,7 @@ safe-path.workspace = true
 # to be modified at runtime.
 logging.workspace = true
 vsock-exporter.workspace = true
+pathrs.workspace = true
 
 # Initdata
 base64 = "0.22"

src/agent/rustjail/Cargo.toml

@@ -46,6 +46,7 @@ libseccomp = { version = "0.3.0", optional = true }
 zbus = "3.12.0"
 bit-vec = "0.8.0"
 xattr = "0.2.3"
+pathrs.workspace = true
 
 # Local dependencies
 protocols.workspace = true

src/agent/rustjail/src/mount.rs

@@ -16,8 +16,9 @@ use nix::NixPath;
 use oci::{LinuxDevice, Mount, Process, Spec};
 use oci_spec::runtime as oci;
 use std::collections::{HashMap, HashSet};
-use std::fs::{self, OpenOptions};
+use std::fs;
 use std::mem::MaybeUninit;
+use std::os::fd::AsRawFd;
 use std::os::unix;
 use std::os::unix::fs::PermissionsExt;
 use std::os::unix::io::RawFd;
@@ -47,6 +48,7 @@ pub struct Info {
 const MOUNTINFO_FORMAT: &str = "{d} {d} {d}:{d} {} {} {} {}";
 const MOUNTINFO_PATH: &str = "/proc/self/mountinfo";
 const PROC_PATH: &str = "/proc";
+const SHARED_DIRECTORY: &str = "/run/kata-containers/shared/containers";
 
 const ERR_FAILED_PARSE_MOUNTINFO: &str = "failed to parse mountinfo file";
 const ERR_FAILED_PARSE_MOUNTINFO_FINAL_FIELDS: &str =
@@ -233,10 +235,11 @@ pub fn init_rootfs(
 
         // From https://github.com/opencontainers/runtime-spec/blob/main/config.md#mounts
         // type (string, OPTIONAL) The type of the filesystem to be mounted.
-        // bind may be only specified in the oci spec options -> flags update r#type
+        // For bind mounts, this can be empty or any string whatsoever. For consistency, we set it
+        // to 'bind'.
         let m = &{
             let mut mbind = m.clone();
-            if is_none_mount_type(mbind.typ()) && flags & MsFlags::MS_BIND == MsFlags::MS_BIND {
+            if flags.contains(MsFlags::MS_BIND) {
                 mbind.set_typ(Some("bind".to_string()));
             }
             mbind
@@ -397,13 +400,6 @@ fn mount_cgroups_v2(cfd_log: RawFd, m: &Mount, rootfs: &str, flags: MsFlags) ->
     Ok(())
 }
 
-fn is_none_mount_type(typ: &Option<String>) -> bool {
-    match typ {
-        Some(t) => t == "none",
-        None => true,
-    }
-}
-
 fn mount_cgroups(
     cfd_log: RawFd,
     m: &Mount,
@@ -763,48 +759,20 @@ fn mount_from(
     let mut d = String::from(data);
     let mount_dest = m.destination().display().to_string();
     let mount_typ = m.typ().as_ref().unwrap();
+
+    if mount_typ == "bind" {
+        // Bind mounts need special treatment for security, handle them separately.
+        return bind_mount_from(m, rootfs, flags)
+            .inspect_err(|e| log_child!(cfd_log, "bind_mount_from failed: {:?}", e));
+    }
+
     let dest = scoped_join(rootfs, mount_dest)?
         .to_str()
         .ok_or_else(|| anyhow::anyhow!("Failed to convert path to string"))?
         .to_string();
 
     let mount_source = m.source().as_ref().unwrap().display().to_string();
-    let src = if mount_typ == "bind" {
-        let src = fs::canonicalize(&mount_source)?;
-        let dir = if src.is_dir() {
-            Path::new(&dest)
-        } else {
-            Path::new(&dest).parent().unwrap()
-        };
-
-        fs::create_dir_all(dir).inspect_err(|e| {
-            log_child!(
-                cfd_log,
-                "create dir {}: {}",
-                dir.to_str().unwrap(),
-                e.to_string()
-            )
-        })?;
-
-        // make sure file exists so we can bind over it
-        if !src.is_dir() {
-            let _ = OpenOptions::new()
-                .create(true)
-                .truncate(true)
-                .write(true)
-                .open(&dest)
-                .map_err(|e| {
-                    log_child!(
-                        cfd_log,
-                        "open/create dest error. {}: {:?}",
-                        dest.as_str(),
-                        e
-                    );
-                    e
-                })?;
-        }
-        src.to_str().unwrap().to_string()
-    } else {
+    let src = {
         let _ = fs::create_dir_all(&dest);
         if mount_typ == "cgroup2" {
             "cgroup2".to_string()
@@ -864,25 +832,126 @@ fn mount_from(
     if !label.is_empty() && selinux::is_enabled()? && use_xattr {
         xattr::set(dest.as_str(), "security.selinux", label.as_bytes())?;
     }
+    Ok(())
+}
 
-    if flags.contains(MsFlags::MS_BIND)
-        && flags.intersects(
-            !(MsFlags::MS_REC
-                | MsFlags::MS_REMOUNT
-                | MsFlags::MS_BIND
-                | MsFlags::MS_PRIVATE
-                | MsFlags::MS_SHARED
-                | MsFlags::MS_SLAVE),
+fn bind_mount_from(m: &Mount, rootfs: &str, flags: MsFlags) -> Result<()> {
+    let mount_source_fd = {
+        let shared_dir = PathBuf::from(SHARED_DIRECTORY);
+        let unsafe_mount_source = m
+            .source()
+            .as_ref()
+            .ok_or_else(|| anyhow::anyhow!("Missing source for bind mount"))?;
+        // Policy checks ensured earlier that shared mount sources start with the `sfprefix`.
+        // Therefore, it's safe to derive the root for scoping the mount source from that prefix.
+        let (root_path, inner_path) = match unsafe_mount_source.strip_prefix(&shared_dir) {
+            Ok(inner) => (shared_dir, inner), // needs scoping
+            Err(_) => (PathBuf::from("/"), unsafe_mount_source.as_path()), // does not need scoping, i.e. scoped to root
+        };
+        let root = pathrs::Root::open(&root_path)
+            .context(format!("opening mount_source_root {:?} failed", root_path))?;
+        root.open_subpath(inner_path, pathrs::flags::OpenFlags::O_PATH)
+            .context(format!(
+                "opening {:?} in {:?} failed",
+                inner_path, root_path
+            ))?
+    };
+
+    let container_root = pathrs::Root::open(rootfs)
+        .context(format!("opening mount_source_root {:?} failed", rootfs))?;
+
+    // .metadata queries attrs with statx + AT_EMPTY_PATH, i.e. directly from the opened fd.
+    let meta = mount_source_fd.metadata().context("statx failed")?;
+    let mount_destination_fd = if meta.is_dir() {
+        container_root
+            .mkdir_all(m.destination(), &std::fs::Permissions::from_mode(0o755))
+            .context(format!(
+                "mkdir_all for {:?} in {} failed",
+                m.destination(),
+                rootfs
+            ))?
+            .reopen(pathrs::flags::OpenFlags::O_DIRECTORY)?
+    } else if meta.is_symlink() {
+        anyhow::bail!("won't bind mount from symlink {:?}: {:?}", m.destination(), meta)
+    } else {
+        // All other bind mounts (files, devices, sockets) should target a file.
+        if let Some(parent) = m.destination().parent() {
+            let _ = container_root
+                .mkdir_all(parent, &std::fs::Permissions::from_mode(0o755))
+                .context(format!("mkdir_all for {:?} in {} failed", parent, rootfs))?;
+        }
+        container_root
+            .create_file(
+                m.destination(),
+                pathrs::flags::OpenFlags::O_TRUNC,
+                &std::fs::Permissions::from_mode(0o755),
+            )
+            .context(format!(
+                "create_file for {:?} in {} failed",
+                m.destination(),
+                rootfs
+            ))?
+    };
+
+    let open_tree_flags = if flags.intersects(MsFlags::MS_REC) {
+        libc::AT_RECURSIVE
+    } else {
+        0
+    };
+    let empty_path = std::ffi::CString::new("")?;
+
+    let tree_dfd = unsafe {
+        libc::syscall(
+            libc::SYS_open_tree,
+            mount_source_fd.as_raw_fd(),
+            empty_path.as_ptr(),
+            libc::OPEN_TREE_CLONE
+                | libc::OPEN_TREE_CLOEXEC
+                | libc::AT_EMPTY_PATH as u32
+                | open_tree_flags as u32,
         )
-    {
+    };
+
+    if tree_dfd < 0 {
+        return Err(std::io::Error::last_os_error().into());
+    }
+
+    let ret = unsafe {
+        libc::syscall(
+            libc::SYS_move_mount,
+            tree_dfd,
+            empty_path.as_ptr(),
+            mount_destination_fd.as_raw_fd(),
+            empty_path.as_ptr(),
+            0x01 /* MOVE_MOUNT_F_EMPTY_PATH */ | 0x02, /* MOVE_MOUNT_T_EMPTY_PATH */
+        )
+    };
+
+    if ret < 0 {
+        return Err(std::io::Error::last_os_error().into());
+    }
+
+    if flags.intersects(
+        !(MsFlags::MS_REC
+            | MsFlags::MS_REMOUNT
+            | MsFlags::MS_BIND
+            | MsFlags::MS_PRIVATE
+            | MsFlags::MS_SHARED
+            | MsFlags::MS_SLAVE),
+    ) {
+        // TODO(burgerdev): we really should be using mount_setattr here, but the necessary API is not in nix.
+
+        // We successfully resolved the destination within the rootfs above. If nothing else messed
+        // with the filesystem in between, using the path unchecked should be safe.
+        let dest = scoped_join(rootfs, m.destination())?;
         mount(
-            Some(dest.as_str()),
-            dest.as_str(),
+            Some(&dest),
+            &dest,
             None::<&str>,
             flags | MsFlags::MS_REMOUNT,
             None::<&str>,
         )
-        .inspect_err(|e| log_child!(cfd_log, "remout {}: {:?}", dest.as_str(), e))?;
+        .context("remount failed")?;
     }
     Ok(())
 }

src/agent/src/rpc.rs

@@ -4,6 +4,7 @@
 //
 
 use async_trait::async_trait;
+use pathrs::flags::OpenFlags;
 use rustjail::{pipestream::PipeStream, process::StreamType};
 use tokio::io::{AsyncReadExt, AsyncWriteExt, ReadHalf};
 use tokio::sync::Mutex;
@@ -13,6 +14,7 @@ use std::convert::TryFrom;
 use std::ffi::{CString, OsStr};
 use std::fmt::Debug;
 use std::io;
+use std::os::fd::AsRawFd;
 use std::os::unix::ffi::OsStrExt;
 use std::path::Path;
 #[cfg(target_arch = "s390x")]
@@ -99,7 +101,7 @@ use std::os::unix::prelude::PermissionsExt;
 use std::process::{Command, Stdio};
 
 use nix::unistd::{Gid, Uid};
-use std::fs::{File, OpenOptions};
+use std::fs::File;
 use std::io::{BufRead, BufReader, Write};
 use std::os::unix::fs::FileExt;
 use std::path::PathBuf;
@@ -132,6 +134,18 @@ const ERR_NO_SANDBOX_PIDNS: &str = "Sandbox does not have sandbox_pidns";
 // not available.
 const IPTABLES_RESTORE_WAIT_SEC: u64 = 5;
 
+/// This mask is applied to parent directories implicitly created for CopyFile requests.
+const IMPLICIT_DIRECTORY_PERMISSION_MASK: u32 = 0o777;
+
+/// This mask is applied to directories explicitly created for CopyFile requests.
+/// setgid and sticky bit are valid for such directories, whereas setuid is not.
+const EXPLICIT_DIRECTORY_PERMISSION_MASK: u32 = 0o3777;
+
+/// This mask is applied to files created for CopyFile requests.
+/// This constant is used for consistency with *_DIRECTORY_PERMISSION_MASK.
+const FILE_PERMISSION_MASK: u32 = 0o7777;
+
+
 // Convenience function to obtain the scope logger.
 fn sl() -> slog::Logger {
     slog_scope::logger()
@@ -1521,7 +1535,9 @@ impl agent_ttrpc::AgentService for AgentService {
         trace_rpc_call!(ctx, "copy_file", req);
         is_allowed(&req).await?;
 
-        do_copy_file(&req).map_ttrpc_err(same)?;
+        // Potentially untrustworthy data from the host needs to go into the shared dir.
+        let root_path = PathBuf::from(KATA_GUEST_SHARE_DIR);
+        do_copy_file(&req, &root_path).map_ttrpc_err(same)?;
 
         Ok(Empty::new())
     }
@@ -2035,125 +2051,116 @@ fn do_set_guest_date_time(sec: i64, usec: i64) -> Result<()> {
     Ok(())
 }
 
-fn do_copy_file(req: &CopyFileRequest) -> Result<()> {
-    let path = PathBuf::from(req.path.as_str());
+/// do_copy_file creates a file, directory or symlink beneath the provided directory.
+///
+/// The function guarantees that no content is written outside of the directory. However, a symlink
+/// created by this function might point outside the shared directory. Other users of that
+/// directory need to consider whether they trust the host, or handle the directory with the same
+/// care as do_copy_file.
+///
+/// Parent directories are created, if they don't exist already. For these implicit operations, the
+/// permissions are set with req.dir_mode. The actual target is created with permissions from
+/// req.file_mode, even if it's a directory. For symlinks, req.file_mode is
+///
+/// If this function returns an error, the filesystem may be in an unexpected state. This is not
+/// significant for the caller, since errors are almost certainly not retriable. The runtime should
+/// abandon this VM instead.
+fn do_copy_file(req: &CopyFileRequest, shared_dir: &PathBuf) -> Result<()> {
+    let insecure_full_path = PathBuf::from(req.path.as_str());
+    let path = insecure_full_path
+        .strip_prefix(&shared_dir)
+        .context(format!(
+            "removing {:?} prefix from {}",
+            shared_dir, req.path
+        ))?;
 
-    if !path.starts_with(CONTAINER_BASE) {
-        return Err(anyhow!(
-            "Path {:?} does not start with {}",
-            path,
-            CONTAINER_BASE
-        ));
-    }
+    // The shared directory might not exist yet, but we need to create it in order to open the root.
+    std::fs::create_dir_all(shared_dir)?;
+    let root = pathrs::Root::open(shared_dir)?;
+
+    // Remove anything that might already exist at the target location.
+    // This is safe even for a symlink leaf, remove_all removes the named inode in its parent dir.
+    root.remove_all(path).or_else(|e| match e.kind() {
+        pathrs::error::ErrorKind::OsError(Some(errno)) if errno == libc::ENOENT => Ok(()),
+        _ => Err(e),
+    })?;
 
     // Create parent directories if missing
     if let Some(parent) = path.parent() {
-        if !parent.exists() {
-            let dir = parent.to_path_buf();
-            // Attempt to create directory, ignore AlreadyExists errors
-            if let Err(e) = fs::create_dir_all(&dir) {
-                if e.kind() != std::io::ErrorKind::AlreadyExists {
-                    return Err(e.into());
-                }
-            }
+        let dir = root
+            .mkdir_all(
+                parent,
+                &std::fs::Permissions::from_mode(req.dir_mode & IMPLICIT_DIRECTORY_PERMISSION_MASK),
+            )
+            .context("mkdir_all parent")?
+            .reopen(OpenFlags::O_DIRECTORY)
+            .context("reopen parent")?;
 
-            // Set directory permissions and ownership
-            std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(req.dir_mode))?;
-            unistd::chown(
-                &dir,
-                Some(Uid::from_raw(req.uid as u32)),
-                Some(Gid::from_raw(req.gid as u32)),
-            )?;
-        }
+        // TODO(burgerdev): why are we only applying this to the immediate parent?
+        unistd::fchown(
+            dir.as_raw_fd(),
+            Some(Uid::from_raw(req.uid as u32)),
+            Some(Gid::from_raw(req.gid as u32)),
+        )
+        .context("fchown parent")?
     }
 
     let sflag = stat::SFlag::from_bits_truncate(req.file_mode);
 
     if sflag.contains(stat::SFlag::S_IFDIR) {
-        // Remove existing non-directory file if present
-        if path.exists() && !path.is_dir() {
-            fs::remove_file(&path)?;
-        }
+        // mkdir_all does not support the setuid/setgid/sticky bits, so we first create the
+        // directory with the stricter mask and then change permissions with the correct mask.
+        let dir = root
+            .mkdir_all(
+                &path,
+                &std::fs::Permissions::from_mode(req.file_mode & IMPLICIT_DIRECTORY_PERMISSION_MASK),
+            )
+            .context("mkdir_all dir")?
+            .reopen(OpenFlags::O_DIRECTORY)
+            .context("reopen dir")?;
+        dir.set_permissions(std::fs::Permissions::from_mode(req.file_mode & EXPLICIT_DIRECTORY_PERMISSION_MASK))?;
 
-        fs::create_dir(&path).or_else(|e| {
-            if e.kind() != std::io::ErrorKind::AlreadyExists {
-                return Err(e);
-            }
-            Ok(())
-        })?;
-
-        std::fs::set_permissions(&path, std::fs::Permissions::from_mode(req.file_mode))?;
-
-        unistd::chown(
-            &path,
+        unistd::fchown(
+            dir.as_raw_fd(),
             Some(Uid::from_raw(req.uid as u32)),
             Some(Gid::from_raw(req.gid as u32)),
-        )?;
+        )
+        .context("fchown dir")?;
 
         return Ok(());
     }
 
     // Handle symlink creation
     if sflag.contains(stat::SFlag::S_IFLNK) {
-        // Clean up existing path (whether symlink, dir, or file)
-        if path.exists() || path.is_symlink() {
-            // Use appropriate removal method based on path type
-            if path.is_symlink() {
-                unistd::unlink(&path)?;
-            } else if path.is_dir() {
-                fs::remove_dir_all(&path)?;
-            } else {
-                fs::remove_file(&path)?;
-            }
-        }
-
         // Create new symbolic link
         let src = PathBuf::from(OsStr::from_bytes(&req.data));
-        unistd::symlinkat(&src, None, &path)?;
-
-        // Set symlink ownership (permissions not supported for symlinks)
-        let path_str = CString::new(path.as_os_str().as_bytes())?;
-
-        let ret = unsafe { libc::lchown(path_str.as_ptr(), req.uid as u32, req.gid as u32) };
-        Errno::result(ret).map(drop)?;
-
+        root.create(path, &pathrs::InodeType::Symlink(src))
+            .context("create symlink")?;
+        // Symlinks don't have permissions on Linux!
         return Ok(());
     }
 
-    let mut tmpfile = path.clone();
-    tmpfile.set_extension("tmp");
+    // Write file content.
+    let flags = if req.offset == 0 {
+        OpenFlags::O_RDWR | OpenFlags::O_CREAT | OpenFlags::O_TRUNC
+    } else {
+        OpenFlags::O_RDWR | OpenFlags::O_CREAT
+    };
+    let file = root
+        .create_file(path, flags, &std::fs::Permissions::from_mode(req.file_mode & FILE_PERMISSION_MASK))
+        .context("create_file")?;
+    file.write_all_at(req.data.as_slice(), req.offset as u64)
+        .context("write_all_at")?;
+    // Things like umask can change the permissions after create, make sure that they stay
+    file.set_permissions(std::fs::Permissions::from_mode(req.file_mode & FILE_PERMISSION_MASK))
+        .context("set_permissions")?;
 
-    let file = OpenOptions::new()
-        .write(true)
-        .create(true)
-        .truncate(req.offset == 0) // Only truncate when offset is 0
-        .open(&tmpfile)?;
-
-    file.write_all_at(req.data.as_slice(), req.offset as u64)?;
-    let st = stat::stat(&tmpfile)?;
-
-    if st.st_size != req.file_size {
-        return Ok(());
-    }
-
-    file.set_permissions(std::fs::Permissions::from_mode(req.file_mode))?;
-
-    unistd::chown(
-        &tmpfile,
+    unistd::fchown(
+        file.as_raw_fd(),
         Some(Uid::from_raw(req.uid as u32)),
         Some(Gid::from_raw(req.gid as u32)),
-    )?;
-
-    // Remove existing target path before rename
-    if path.exists() || path.is_symlink() {
-        if path.is_dir() {
-            fs::remove_dir_all(&path)?;
-        } else {
-            fs::remove_file(&path)?;
-        }
-    }
-
-    fs::rename(tmpfile, path)?;
+    )
+    .context("fchown")?;
 
     Ok(())
 }
@@ -2444,6 +2451,7 @@ mod tests {
 
     use super::*;
     use crate::{namespace::Namespace, protocols::agent_ttrpc_async::AgentService as _};
+    use anyhow::ensure;
     use nix::mount;
     use nix::sched::{unshare, CloneFlags};
     use oci::{
@@ -3465,4 +3473,274 @@ COMMIT
             assert_eq!(d.result, result, "{msg}");
         }
     }
+
+    #[tokio::test]
+    async fn test_do_copy_file() {
+        let temp_dir = tempdir().expect("creating temp dir failed");
+        // We start one directory deeper such that we catch problems when the shared directory does
+        // not exist yet.
+        let base = temp_dir.path().join("shared");
+
+        struct TestCase {
+            name: String,
+            request: CopyFileRequest,
+            assertions: Box<dyn Fn(&Path) -> Result<()>>,
+            should_fail: bool,
+        }
+
+        let tests = [
+            TestCase {
+                name: "Create a top-level file".into(),
+                request: CopyFileRequest {
+                    path: base.join("f").to_string_lossy().into(),
+                    file_mode: 0o644 | libc::S_IFREG,
+                    ..Default::default()
+                },
+                should_fail: false,
+                assertions: Box::new(|base| -> Result<()> {
+                    let f = base.join("f");
+                    let f_stat = fs::metadata(&f).context("stat ./f failed")?;
+                    ensure!(f_stat.is_file());
+                    ensure!(0o644 == f_stat.permissions().mode() & 0o777);
+                    let content = std::fs::read_to_string(&f).context("read ./f failed")?;
+                    ensure!(content.is_empty());
+                    Ok(())
+                }),
+            },
+            TestCase {
+                name: "Replace a top-level file".into(),
+                request: CopyFileRequest {
+                    path: base.join("f").to_string_lossy().into(),
+                    file_mode: 0o600 | libc::S_IFREG,
+                    data: b"Hello!".to_vec(),
+                    ..Default::default()
+                },
+                should_fail: false,
+                assertions: Box::new(|base| -> Result<()> {
+                    let f = base.join("f");
+                    let f_stat = fs::metadata(&f).context("stat ./f failed")?;
+                    ensure!(f_stat.is_file());
+                    ensure!(0o600 == f_stat.permissions().mode() & 0o777);
+                    let content = std::fs::read_to_string(&f).context("read ./f failed")?;
+                    ensure!("Hello!" == content);
+                    Ok(())
+                }),
+            },
+            TestCase {
+                name: "Create a file and its parent directory".into(),
+                request: CopyFileRequest {
+                    path: base.join("a/b").to_string_lossy().into(),
+                    dir_mode: 0o755 | libc::S_IFDIR,
+                    file_mode: 0o644 | libc::S_IFREG,
+                    ..Default::default()
+                },
+                should_fail: false,
+                assertions: Box::new(|base| -> Result<()> {
+                    let a_stat = fs::metadata(base.join("a")).context("stat ./a failed")?;
+                    ensure!(a_stat.is_dir());
+                    ensure!(0o755 == a_stat.permissions().mode() & 0o777);
+                    let b_stat = fs::metadata(base.join("a/b")).context("stat ./a/b failed")?;
+                    ensure!(b_stat.is_file());
+                    ensure!(0o644 == b_stat.permissions().mode() & 0o777);
+                    Ok(())
+                }),
+            },
+            TestCase {
+                name: "Create a file within an existing directory".into(),
+                request: CopyFileRequest {
+                    path: base.join("a/c").to_string_lossy().into(),
+                    dir_mode: 0o700 | libc::S_IFDIR, // Test that existing directories are not touched - we expect this to stay 0o755.
+                    file_mode: 0o621 | libc::S_IFREG,
+                    ..Default::default()
+                },
+                should_fail: false,
+                assertions: Box::new(|base| -> Result<()> {
+                    let a_stat = fs::metadata(base.join("a")).context("stat ./a failed")?;
+                    ensure!(a_stat.is_dir());
+                    ensure!(0o755 == a_stat.permissions().mode() & 0o777);
+                    let c_stat = fs::metadata(base.join("a/c")).context("stat ./a/c failed")?;
+                    ensure!(c_stat.is_file());
+                    ensure!(0o621 == c_stat.permissions().mode() & 0o777);
+                    Ok(())
+                }),
+            },
+            TestCase {
+                name: "Create a directory".into(),
+                request: CopyFileRequest {
+                    path: base.join("a/d").to_string_lossy().into(),
+                    dir_mode: 0o700 | libc::S_IFDIR, // Test that the permissions are taken from file_mode.
+                    file_mode: 0o755 | libc::S_IFDIR,
+                    ..Default::default()
+                },
+                should_fail: false,
+                assertions: Box::new(|base| -> Result<()> {
+                    let a_stat = fs::metadata(base.join("a")).context("stat ./a failed")?;
+                    ensure!(a_stat.is_dir());
+                    ensure!(0o755 == a_stat.permissions().mode() & 0o777);
+                    let d_stat = fs::metadata(base.join("a/d")).context("stat ./a/d failed")?;
+                    ensure!(d_stat.is_dir());
+                    ensure!(0o755 == d_stat.permissions().mode() & 0o777);
+                    Ok(())
+                }),
+            },
+            TestCase {
+                name: "Create a dir onto an existing file".into(),
+                request: CopyFileRequest {
+                    path: base.join("a/b").to_string_lossy().into(),
+                    dir_mode: 0o700 | libc::S_IFDIR, // Test that the permissions are taken from file_mode.
+                    file_mode: 0o755 | libc::S_IFDIR,
+                    ..Default::default()
+                },
+                should_fail: false,
+                assertions: Box::new(|base| -> Result<()> {
+                    let b_stat = fs::metadata(base.join("a/b")).context("stat ./a/b failed")?;
+                    ensure!(b_stat.is_dir());
+                    ensure!(0o755 == b_stat.permissions().mode() & 0o777);
+                    Ok(())
+                }),
+            },
+            TestCase {
+                name: "Create a file onto an existing dir".into(),
+                request: CopyFileRequest {
+                    path: base.join("a/b").to_string_lossy().into(),
+                    dir_mode: 0o755 | libc::S_IFDIR,
+                    file_mode: 0o644 | libc::S_IFREG,
+                    ..Default::default()
+                },
+                should_fail: false,
+                assertions: Box::new(|base| -> Result<()> {
+                    let b_stat = fs::metadata(base.join("a/b")).context("stat ./a/b failed")?;
+                    ensure!(b_stat.is_file());
+                    ensure!(0o644 == b_stat.permissions().mode() & 0o777);
+                    Ok(())
+                }),
+            },
+            TestCase {
+                name: "Create a symlink".into(),
+                request: CopyFileRequest {
+                    path: base.join("a/link").to_string_lossy().into(),
+                    dir_mode: 0o700 | libc::S_IFDIR, // Test that the permissions are taken from file_mode.
+                    file_mode: 0o755 | libc::S_IFLNK,
+                    data: b"/etc/passwd".to_vec(),
+                    ..Default::default()
+                },
+                should_fail: false,
+                assertions: Box::new(|base| -> Result<()> {
+                    let a_stat = fs::metadata(base.join("a")).context("stat ./a failed")?;
+                    ensure!(a_stat.is_dir());
+                    ensure!(0o755 == a_stat.permissions().mode() & 0o777);
+                    let link = base.join("a/link");
+                    let link_stat = nix::sys::stat::lstat(&link).context("stat ./a/link failed")?;
+                    // Linux symlinks have no permissions!
+                    ensure!(0o777 | libc::S_IFLNK == link_stat.st_mode);
+                    let target = fs::read_link(&link).context("read_link ./a/link failed")?;
+                    ensure!(target.to_string_lossy() == "/etc/passwd");
+                    Ok(())
+                }),
+            },
+            TestCase {
+                name: "Create a directory with setgid and sticky bit".into(),
+                request: CopyFileRequest {
+                    path: base.join("x/y").to_string_lossy().into(),
+                    dir_mode: 0o3755 | libc::S_IFDIR,
+                    file_mode: 0o3770 | libc::S_IFDIR,
+                    ..Default::default()
+                },
+                should_fail: false,
+                assertions: Box::new(|base| -> Result<()> {
+                    // Implicitly created directories should not get a sticky bit.
+                    let x_stat = fs::metadata(base.join("x")).context("stat ./x failed")?;
+                    ensure!(x_stat.is_dir());
+                    ensure!(0o755 == x_stat.permissions().mode() & 0o7777);
+                    // Explicitly created directories should.
+                    let y_stat = fs::metadata(base.join("x/y")).context("stat ./x/y failed")?;
+                    ensure!(y_stat.is_dir());
+                    ensure!(0o3770 == y_stat.permissions().mode() & 0o7777);
+                    Ok(())
+                }),
+            },
+            // =================================
+            // Below are some adversarial tests.
+            // =================================
+            TestCase {
+                name: "Malicious intermediate directory is a symlink".into(),
+                request: CopyFileRequest {
+                    path: base
+                        .join("a/link/this-could-just-be-shadow-but-I-am-not-risking-it")
+                        .to_string_lossy()
+                        .into(),
+                    dir_mode: 0o700 | libc::S_IFDIR, // Test that the permissions are taken from file_mode.
+                    file_mode: 0o755 | libc::S_IFLNK,
+                    data: b"root:password:19000:0:99999:7:::\n".to_vec(),
+                    ..Default::default()
+                },
+                should_fail: true,
+                assertions: Box::new(|base| -> Result<()> {
+                    let link_stat = nix::sys::stat::lstat(&base.join("a/link"))
+                        .context("stat ./a/link failed")?;
+                    ensure!(0o777 | libc::S_IFLNK == link_stat.st_mode);
+                    Ok(())
+                }),
+            },
+            TestCase {
+                name: "Create a symlink onto an existing symlink".into(),
+                request: CopyFileRequest {
+                    path: base.join("a/link").to_string_lossy().into(),
+                    dir_mode: 0o700 | libc::S_IFDIR, // Test that the permissions are taken from file_mode.
+                    file_mode: 0o755 | libc::S_IFLNK,
+                    data: b"/etc".to_vec(),
+                    ..Default::default()
+                },
+                should_fail: false,
+                assertions: Box::new(|base| -> Result<()> {
+                    // The symlink should be created at the same place (not followed), with the new content.
+                    let a_stat = fs::metadata(base.join("a")).context("stat ./a failed")?;
+                    ensure!(a_stat.is_dir());
+                    ensure!(0o755 == a_stat.permissions().mode() & 0o777);
+                    let link = base.join("a/link");
+                    let link_stat = nix::sys::stat::lstat(&link).context("stat ./a/link failed")?;
+                    // Linux symlinks have no permissions!
+                    ensure!(0o777 | libc::S_IFLNK == link_stat.st_mode);
+                    let target = fs::read_link(&link).context("read_link ./a/link failed")?;
+                    ensure!(target.to_string_lossy() == "/etc");
+                    Ok(())
+                }),
+            },
+            TestCase {
+                name: "Create a file onto an existing symlink".into(),
+                request: CopyFileRequest {
+                    path: base.join("a/link").to_string_lossy().into(),
+                    file_mode: 0o600 | libc::S_IFREG,
+                    data: b"Hello!".to_vec(),
+                    ..Default::default()
+                },
+                should_fail: false,
+                assertions: Box::new(|base| -> Result<()> {
+                    // The symlink itself should be replaced with the file, not followed.
+                    let link = base.join("a/link");
+                    let link_stat = nix::sys::stat::lstat(&link).context("stat ./a/link failed")?;
+                    ensure!(0o600 | libc::S_IFREG == link_stat.st_mode);
+                    let content = std::fs::read_to_string(&link).context("read ./a/link failed")?;
+                    ensure!("Hello!" == content);
+                    Ok(())
+                }),
+            },
+        ];
+
+        let uid = unistd::getuid().as_raw() as i32;
+        let gid = unistd::getgid().as_raw() as i32;
+
+        for mut tc in tests {
+            println!("Running test case: {}", tc.name);
+            // Since we're in a unit test, using root ownership causes issues with cleaning the temp dir.
+            tc.request.uid = uid;
+            tc.request.gid = gid;
+
+            let res = do_copy_file(&tc.request, (&base).into());
+            if tc.should_fail != res.is_err() {
+                panic!("{}: unexpected do_copy_file result: {:?}", tc.name, res)
+            }
+            (tc.assertions)(&base).context(tc.name).unwrap()
+        }
+    }
 }

src/agent/src/storage/fs_handler.rs

@@ -10,7 +10,7 @@ use std::sync::Arc;
 
 use crate::storage::{common_storage_handler, new_device, StorageContext, StorageHandler};
 use anyhow::{anyhow, Context, Result};
-use kata_types::device::{DRIVER_9P_TYPE, DRIVER_OVERLAYFS_TYPE, DRIVER_VIRTIOFS_TYPE};
+use kata_types::device::{DRIVER_OVERLAYFS_TYPE, DRIVER_VIRTIOFS_TYPE};
 use kata_types::mount::{StorageDevice, KATA_VOLUME_OVERLAYFS_CREATE_DIR};
 use protocols::agent::Storage;
 use tracing::instrument;
@@ -69,27 +69,6 @@ impl StorageHandler for OverlayfsHandler {
     }
 }
 
-#[derive(Debug)]
-pub struct Virtio9pHandler {}
-
-#[async_trait::async_trait]
-impl StorageHandler for Virtio9pHandler {
-    #[instrument]
-    fn driver_types(&self) -> &[&str] {
-        &[DRIVER_9P_TYPE]
-    }
-
-    #[instrument]
-    async fn create_device(
-        &self,
-        storage: Storage,
-        ctx: &mut StorageContext,
-    ) -> Result<Arc<dyn StorageDevice>> {
-        let path = common_storage_handler(ctx.logger, &storage)?;
-        new_device(path)
-    }
-}
-
 #[derive(Debug)]
 pub struct VirtioFsHandler {}
 

src/agent/src/storage/mod.rs

@@ -23,7 +23,7 @@ use tracing::instrument;
 use self::bind_watcher_handler::BindWatcherHandler;
 use self::block_handler::{PmemHandler, ScsiHandler, VirtioBlkMmioHandler, VirtioBlkPciHandler};
 use self::ephemeral_handler::EphemeralHandler;
-use self::fs_handler::{OverlayfsHandler, Virtio9pHandler, VirtioFsHandler};
+use self::fs_handler::{OverlayfsHandler, VirtioFsHandler};
 use self::image_pull_handler::ImagePullHandler;
 use self::local_handler::LocalHandler;
 use crate::mount::{baremount, is_mounted, remove_mounts};
@@ -134,7 +134,6 @@ lazy_static! {
     pub static ref STORAGE_HANDLERS: StorageHandlerManager<Arc<dyn StorageHandler>> = {
         let mut manager: StorageHandlerManager<Arc<dyn StorageHandler>> = StorageHandlerManager::new();
         let handlers: Vec<Arc<dyn StorageHandler>> = vec![
-            Arc::new(Virtio9pHandler {}),
             Arc::new(VirtioBlkMmioHandler {}),
             Arc::new(VirtioBlkPciHandler {}),
             Arc::new(EphemeralHandler {}),

src/agent/src/watcher.rs

@@ -425,7 +425,7 @@ impl SandboxStorages {
 /// or updated file to a target mount point, or remove the removed file from the target mount point.  All WatchableStorage
 /// target mount points are expected to reside within a single tmpfs, whose root is created by the BindWatcher.
 ///
-/// This is a temporary workaround to handle config map updates until we get inotify on 9p/virtio-fs.
+/// This is a temporary workaround to handle config map updates until we get inotify on virtio-fs.
 /// More context on this:
 /// - https://github.com/kata-containers/runtime/issues/1505
 /// - https://github.com/kata-containers/kata-containers/issues/1879

src/libs/kata-types/src/annotations/mod.rs

@@ -257,7 +257,7 @@ pub const KATA_ANNO_CFG_HYPERVISOR_ENABLE_ROOTLESS_HYPERVISOR: &str =
     "io.katacontainers.config.hypervisor.rootless";
 
 // Hypervisor Shared File System related annotations
-/// A sandbox annotation to specify the shared file system type, either inline-virtio-fs (default), virtio-9p, virtio-fs or virtio-fs-nydus.
+/// A sandbox annotation to specify the shared file system type, either virtio-fs(default), inline-virtio-fs, virtio-fs-nydus or none.
 pub const KATA_ANNO_CFG_HYPERVISOR_SHARED_FS: &str =
     "io.katacontainers.config.hypervisor.shared_fs";
 /// A sandbox annotations to specify virtio-fs vhost-user daemon path.
@@ -272,8 +272,6 @@ pub const KATA_ANNO_CFG_HYPERVISOR_VIRTIO_FS_CACHE_SIZE: &str =
 /// A sandbox annotation to pass options to virtiofsd daemon.
 pub const KATA_ANNO_CFG_HYPERVISOR_VIRTIO_FS_EXTRA_ARGS: &str =
     "io.katacontainers.config.hypervisor.virtio_fs_extra_args";
-/// A sandbox annotation to specify as the msize for 9p shares.
-pub const KATA_ANNO_CFG_HYPERVISOR_MSIZE_9P: &str = "io.katacontainers.config.hypervisor.msize_9p";
 /// The initdata annotation passed in when CVM launchs
 pub const KATA_ANNO_CFG_HYPERVISOR_INIT_DATA: &str =
     "io.katacontainers.config.hypervisor.cc_init_data";
@@ -975,14 +973,6 @@ impl Annotation {
                             hv.shared_fs.virtio_fs_extra_args.push(arg.to_string());
                         }
                     }
-                    KATA_ANNO_CFG_HYPERVISOR_MSIZE_9P => match self.get_value::<u32>(key) {
-                        Ok(v) => {
-                            hv.shared_fs.msize_9p = v.unwrap_or_default();
-                        }
-                        Err(_e) => {
-                            return Err(u32_err);
-                        }
-                    },
                     KATA_ANNO_CFG_HYPERVISOR_BLOCK_DEV_NUM_QUEUES => {
                         match self.get_value::<usize>(key) {
                             Ok(v) => {

src/libs/kata-types/src/config/default.rs

@@ -47,9 +47,6 @@ pub const DEFAULT_BLOCK_DEVICE_QUEUE_SIZE: u32 = 128;
 pub const DEFAULT_SHARED_FS_TYPE: &str = "virtio-fs";
 pub const DEFAULT_VIRTIO_FS_CACHE_MODE: &str = "never";
 pub const DEFAULT_VIRTIO_FS_DAX_SIZE_MB: u32 = 1024;
-pub const DEFAULT_SHARED_9PFS_SIZE_MB: u32 = 8 * 1024;
-pub const MIN_SHARED_9PFS_SIZE_MB: u32 = 4 * 1024;
-pub const MAX_SHARED_9PFS_SIZE_MB: u32 = 8 * 1024 * 1024;
 
 pub const DEFAULT_GUEST_HOOK_PATH: &str = "/opt/kata/hooks";
 pub const DEFAULT_GUEST_DNS_FILE: &str = "/etc/resolv.conf";

src/libs/kata-types/src/config/drop_in.rs

@@ -346,7 +346,7 @@ mod drop_in_directory_handling {
 
             let dropin_override_data = r#"
                 [hypervisor.qemu]
-                shared_fs = "virtio-9p"
+                shared_fs = "none"
                 [runtime]
                 vfio_mode="vfio"
             "#;
@@ -372,7 +372,7 @@ mod drop_in_directory_handling {
             assert_eq!(config.hypervisor["qemu"].device_info.default_bridges, 4);
             assert_eq!(
                 config.hypervisor["qemu"].shared_fs.shared_fs.as_deref(),
-                Some("virtio-9p")
+                Some("none")
             );
             assert!(config.runtime.debug);
             assert!(config.runtime.sandbox_cgroup_only);

src/libs/kata-types/src/config/hypervisor/mod.rs

@@ -68,7 +68,6 @@ mod firecracker;
 pub use self::firecracker::{FirecrackerConfig, HYPERVISOR_NAME_FIRECRACKER};
 
 const NO_VIRTIO_FS: &str = "none";
-const VIRTIO_9P: &str = "virtio-9p";
 const VIRTIO_FS: &str = "virtio-fs";
 const VIRTIO_FS_INLINE: &str = "inline-virtio-fs";
 const MAX_BRIDGE_SIZE: u32 = 5;
@@ -1419,12 +1418,13 @@ impl SecurityInfo {
     }
 }
 
-/// Configuration information for shared filesystems, such as virtio-9p and virtio-fs.
+/// Configuration information for shared filesystems, such as virtio-fs-nydus and virtio-fs.
 #[derive(Clone, Debug, Default, Deserialize, Serialize)]
 pub struct SharedFsInfo {
     /// Type of shared file system to use:
     /// - `virtio-fs` (default)
-    /// - `virtio-9p`
+    /// - `inline-virtio-fs`
+    /// - `virtio-fs-nydus`
     /// - `none` (disables shared filesystem)
     pub shared_fs: Option<String>,
 
@@ -1466,18 +1466,13 @@ pub struct SharedFsInfo {
     /// Enables `virtio-fs` DAX (Direct Access) window if `true`.
     #[serde(default)]
     pub virtio_fs_is_dax: bool,
-
-    /// This is the `msize` used for 9p shares. It represents the number of bytes
-    /// used for the 9p packet payload.
-    #[serde(default)]
-    pub msize_9p: u32,
 }
 
 impl SharedFsInfo {
     /// Adjusts the shared filesystem configuration after loading from a configuration file.
     ///
     /// Handles default values for `shared_fs` type, `virtio-fs` specific settings
-    /// (daemon path, cache mode, DAX), and `virtio-9p` msize.
+    /// (daemon path, cache mode, DAX) or `inline-virtio-fs` settings.
     pub fn adjust_config(&mut self) -> Result<()> {
         if self.shared_fs.as_deref() == Some(NO_VIRTIO_FS) {
             self.shared_fs = None;
@@ -1490,11 +1485,6 @@ impl SharedFsInfo {
         match self.shared_fs.as_deref() {
             Some(VIRTIO_FS) => self.adjust_virtio_fs(false)?,
             Some(VIRTIO_FS_INLINE) => self.adjust_virtio_fs(true)?,
-            Some(VIRTIO_9P) => {
-                if self.msize_9p == 0 {
-                    self.msize_9p = default::DEFAULT_SHARED_9PFS_SIZE_MB;
-                }
-            }
             _ => {}
         }
 
@@ -1504,23 +1494,12 @@ impl SharedFsInfo {
     /// Validates the shared filesystem configuration.
     ///
     /// Checks the validity of the selected `shared_fs` type and
-    /// performs specific validations for `virtio-fs` and `virtio-9p` settings.
+    /// performs specific validations for `virtio-fs` and `inline-virtio-fs` settings.
     pub fn validate(&self) -> Result<()> {
         match self.shared_fs.as_deref() {
             None => Ok(()),
             Some(VIRTIO_FS) => self.validate_virtio_fs(false),
             Some(VIRTIO_FS_INLINE) => self.validate_virtio_fs(true),
-            Some(VIRTIO_9P) => {
-                if self.msize_9p < default::MIN_SHARED_9PFS_SIZE_MB
-                    || self.msize_9p > default::MAX_SHARED_9PFS_SIZE_MB
-                {
-                    return Err(std::io::Error::other(format!(
-                        "Invalid 9p configuration msize 0x{:x}, min value is 0x{:x}, max value is 0x{:x}",
-                        self.msize_9p,default::MIN_SHARED_9PFS_SIZE_MB, default::MAX_SHARED_9PFS_SIZE_MB
-                    )));
-                }
-                Ok(())
-            }
             Some(v) => Err(std::io::Error::other(format!("Invalid shared_fs type {v}"))),
         }
     }

src/libs/kata-types/src/device.rs

@@ -27,8 +27,6 @@ pub const DRIVER_VFIO_AP_TYPE: &str = "vfio-ap";
 /// DRIVER_VFIO_AP_COLD_TYPE is the device driver for vfio-ap coldplug.
 pub const DRIVER_VFIO_AP_COLD_TYPE: &str = "vfio-ap-cold";
 
-/// DRIVER_9P_TYPE is the driver for 9pfs volume.
-pub const DRIVER_9P_TYPE: &str = "9p";
 /// DRIVER_EPHEMERAL_TYPE is the driver for ephemeral volume.
 pub const DRIVER_EPHEMERAL_TYPE: &str = "ephemeral";
 /// DRIVER_LOCAL_TYPE is the driver for local volume.

src/libs/kata-types/tests/texture/configuration-anno-0.toml

@@ -48,7 +48,6 @@ file_mem_backend = "/dev/shm"
 valid_file_mem_backends = ["/dev/shm","/dev/snd","./test_file_backend_mem_root"]
 pflashes = ["/proc/mounts"]
 enable_debug = true
-msize_9p = 16384
 disable_image_nvdimm = true
 hotplug_vfio_on_root_bus = true
 pcie_root_port = 2

src/libs/kata-types/tests/texture/configuration-anno-1.toml

@@ -47,7 +47,6 @@ file_mem_backend = "/dev/shm"
 valid_file_mem_backends = ["/dev/shm"]
 pflashes = ["/proc/mounts"]
 enable_debug = true
-msize_9p = 16384
 disable_image_nvdimm = true
 hotplug_vfio_on_root_bus = true
 pcie_root_port = 2

src/libs/protocols/protos/agent.proto

@@ -493,7 +493,7 @@ message SharedMount {
 // could have been defined through the Mount list of the OCI specification.
 message Storage {
 	// Driver is used to define the way the storage is passed through the
-	// virtual machine. It can be "9p", "blk", or something else, but for
+	// virtual machine. It can be "blk", or something else, but for
 	// all cases, this will define if some extra steps are required before
 	// this storage gets mounted into the container.
 	string driver = 1;
@@ -509,7 +509,7 @@ message Storage {
 	string source = 3;
 	// Fstype represents the filesystem that needs to be used to mount the
 	// storage inside the VM. For instance, it could be "xfs" for block
-	// device, "9p" for shared filesystem, or "tmpfs" for shared /dev/shm.
+	// device, or "tmpfs" for shared /dev/shm.
 	string fstype = 4;
 	// Options describes the additional options that might be needed to
 	// mount properly the storage filesystem.

src/runtime-rs/config/configuration-dragonball.toml.in

@@ -174,7 +174,6 @@ guest_hook_path = ""
 # Shared file system type:
 #   - inline-virtio-fs (default)
 #   - virtio-fs
-#   - virtio-9p
 #   - virtio-fs-nydus
 # "inline-virtio-fs" is the same as "virtio-fs", but it is running in the same process
 # of shim, does not need an external virtiofsd process.

src/runtime-rs/config/configuration-qemu-coco-dev-runtime-rs.toml.in

@@ -179,7 +179,6 @@ disable_block_device_use = @DEFDISABLEBLOCK@
 
 # Shared file system type:
 #   - virtio-fs (default)
-#   - virtio-9p
 #   - virtio-fs-nydus
 #   - none
 shared_fs = "@DEFSHAREDFS_QEMU_COCO_DEV_VIRTIOFS@"

src/runtime-rs/config/configuration-qemu-runtime-rs.toml.in

@@ -163,7 +163,6 @@ disable_block_device_use = @DEFDISABLEBLOCK@
 
 # Shared file system type:
 #   - virtio-fs (default)
-#   - virtio-9p
 #   - virtio-fs-nydus
 #   - none
 shared_fs = "@DEFSHAREDFS_QEMU_VIRTIOFS@"

src/runtime-rs/config/configuration-qemu-se-runtime-rs.toml.in

@@ -162,7 +162,6 @@ disable_block_device_use = @DEFDISABLEBLOCK@
 
 # Shared file system type:
 #   - virtio-fs (default)
-#   - virtio-9p
 #   - virtio-fs-nydus
 #   - none
 shared_fs = "@DEFSHAREDFS_QEMU_SEL_VIRTIOFS@"

src/tools/kata-ctl/src/ops/env_ops.rs

@@ -184,8 +184,6 @@ pub struct HypervisorInfo {
     #[serde(default)]
     virtio_fs_daemon: String,
     #[serde(default)]
-    msize_9p: u32,
-    #[serde(default)]
     memory_slots: u32,
     #[serde(default)]
     pcie_root_port: u32,
@@ -417,7 +415,6 @@ pub fn get_hypervisor_info(
             .clone()
             .unwrap_or_else(|| String::from("none")),
         virtio_fs_daemon: hypervisor_config.shared_fs.virtio_fs_daemon.to_string(),
-        msize_9p: hypervisor_config.shared_fs.msize_9p,
         memory_slots: hypervisor_config.memory_info.memory_slots,
         pcie_root_port: hypervisor_config.device_info.pcie_root_port,
         hotplug_vfio_on_rootbus: hypervisor_config.device_info.hotplug_vfio_on_root_bus,

tools/packaging/kata-deploy/binary/src/artifacts/install.rs

@@ -440,14 +440,46 @@ fn add_kata_deploy_warning(config_file: &Path) -> Result<()> {
     Ok(())
 }
 
+/// Atomically replace a file with a symlink.
+///
+/// Creates the symlink at a temporary path first, then renames it over the
+/// original so the original is preserved if symlink creation fails.
+fn atomic_symlink_replace(file_path: &str, symlink_target: &str) -> Result<()> {
+    let temp_symlink = format!("{}.tmp-link", file_path);
+
+    // Clean up any stale temp symlink from a previous interrupted run
+    if Path::new(&temp_symlink).exists() || Path::new(&temp_symlink).is_symlink() {
+        let _ = fs::remove_file(&temp_symlink);
+    }
+
+    std::os::unix::fs::symlink(symlink_target, &temp_symlink).with_context(|| {
+        format!(
+            "Failed to create temporary symlink {} -> {}",
+            temp_symlink, symlink_target
+        )
+    })?;
+
+    fs::rename(&temp_symlink, file_path).map_err(|err| {
+        let _ = fs::remove_file(&temp_symlink);
+        anyhow::anyhow!(
+            "Failed to atomically replace {} with symlink to {}: {}",
+            file_path,
+            symlink_target,
+            err
+        )
+    })?;
+
+    Ok(())
+}
+
 /// Set up the runtime directory structure for a shim.
 /// Creates: {config_path}/runtimes/{shim}/
 ///          {config_path}/runtimes/{shim}/config.d/
 ///          {config_path}/runtimes/{shim}/configuration-{shim}.toml (copy of original)
 ///
-/// Note: We copy the config file instead of symlinking because kata-containers'
-/// ResolvePath uses filepath.EvalSymlinks, which would resolve to the original
-/// location and look for config.d there instead of in our per-shim directory.
+/// After copying, the original config file is replaced with a symlink pointing
+/// to the runtime copy. This way the runtime's ResolvePath / EvalSymlinks resolves
+/// the symlink and finds config.d next to the real file in the per-shim directory.
 fn setup_runtime_directory(config: &Config, shim: &str) -> Result<()> {
     let original_config_dir = format!(
         "/host{}",
@@ -466,9 +498,9 @@ fn setup_runtime_directory(config: &Config, shim: &str) -> Result<()> {
     fs::create_dir_all(&config_d_dir)
         .with_context(|| format!("Failed to create config.d directory: {}", config_d_dir))?;
 
-    // Copy the original config file to the runtime directory
-    let original_config_file = format!("{}/configuration-{}.toml", original_config_dir, shim);
-    let dest_config_file = format!("{}/configuration-{}.toml", runtime_config_dir, shim);
+    let config_filename = format!("configuration-{}.toml", shim);
+    let original_config_file = format!("{}/{}", original_config_dir, config_filename);
+    let dest_config_file = format!("{}/{}", runtime_config_dir, config_filename);
 
     // Only copy if original exists
     if Path::new(&original_config_file).exists() {
@@ -481,7 +513,7 @@ fn setup_runtime_directory(config: &Config, shim: &str) -> Result<()> {
             })?;
         }
 
-        // Copy the base config file
+        // Copy the base config file to the runtime directory
         fs::copy(&original_config_file, &dest_config_file).with_context(|| {
             format!(
                 "Failed to copy config: {} -> {}",
@@ -493,13 +525,37 @@ fn setup_runtime_directory(config: &Config, shim: &str) -> Result<()> {
         add_kata_deploy_warning(Path::new(&dest_config_file))?;
 
         info!("  Copied base config: {}", dest_config_file);
+
+        let symlink_target = format!("runtimes/{}/{}", shim, config_filename);
+        atomic_symlink_replace(&original_config_file, &symlink_target)?;
+
+        info!(
+            "  Symlinked original config: {} -> {}",
+            original_config_file, symlink_target
+        );
     }
 
     Ok(())
 }
 
-/// Remove the runtime directory for a shim during cleanup
+/// Remove the runtime directory for a shim during cleanup.
+/// Also removes the symlink at the original config location that was created
+/// by setup_runtime_directory.
 fn remove_runtime_directory(config: &Config, shim: &str) -> Result<()> {
+    // Remove the symlink at the original config location (if present)
+    let original_config_dir = format!(
+        "/host{}",
+        utils::get_kata_containers_original_config_path(shim, &config.dest_dir)
+    );
+    let original_config_file = format!("{}/configuration-{}.toml", original_config_dir, shim);
+    let original_path = Path::new(&original_config_file);
+    if original_path.is_symlink() {
+        fs::remove_file(&original_config_file).with_context(|| {
+            format!("Failed to remove config symlink: {}", original_config_file)
+        })?;
+        log::debug!("Removed config symlink: {}", original_config_file);
+    }
+
     let runtime_config_dir = format!(
         "/host{}",
         utils::get_kata_containers_config_path(shim, &config.dest_dir)
@@ -528,7 +584,7 @@ fn remove_runtime_directory(config: &Config, shim: &str) -> Result<()> {
 }
 
 async fn configure_shim_config(config: &Config, shim: &str, container_runtime: &str) -> Result<()> {
-    // Set up the runtime directory structure with symlink to original config
+    // Set up the runtime directory: copy config to per-shim dir and replace original with symlink
     setup_runtime_directory(config, shim)?;
 
     let runtime_config_dir = format!(
@@ -540,11 +596,11 @@ async fn configure_shim_config(config: &Config, shim: &str, container_runtime: &
     let kata_config_file =
         Path::new(&runtime_config_dir).join(format!("configuration-{shim}.toml"));
 
-    // The configuration file (symlink) should exist after setup_runtime_directory()
+    // The configuration file should exist after setup_runtime_directory()
     if !kata_config_file.exists() {
         return Err(anyhow::anyhow!(
             "Configuration file not found: {kata_config_file:?}. This file should have been \
-             symlinked from the original config. Check that the shim '{}' has a valid configuration \
+             copied from the original config. Check that the shim '{}' has a valid configuration \
              file in the artifacts.",
             shim
         ));
@@ -1144,4 +1200,141 @@ mod tests {
             "following the symlink should yield the real content"
         );
     }
+
+    #[test]
+    fn test_atomic_symlink_replace_creates_symlink() {
+        let tmpdir = tempfile::tempdir().unwrap();
+
+        // Create the original file and the target it will point to
+        let target_dir = tmpdir.path().join("runtimes/qemu");
+        fs::create_dir_all(&target_dir).unwrap();
+        let target_file = target_dir.join("configuration-qemu.toml");
+        fs::write(&target_file, "real config content").unwrap();
+
+        let original = tmpdir.path().join("configuration-qemu.toml");
+        fs::write(&original, "original content").unwrap();
+
+        let symlink_target = "runtimes/qemu/configuration-qemu.toml";
+        atomic_symlink_replace(original.to_str().unwrap(), symlink_target).unwrap();
+
+        assert!(original.is_symlink(), "original should now be a symlink");
+        assert_eq!(
+            fs::read_link(&original).unwrap().to_str().unwrap(),
+            symlink_target
+        );
+        assert_eq!(
+            fs::read_to_string(&original).unwrap(),
+            "real config content",
+            "reading through the symlink should yield the target's content"
+        );
+    }
+
+    #[test]
+    fn test_atomic_symlink_replace_is_idempotent() {
+        let tmpdir = tempfile::tempdir().unwrap();
+
+        let target_dir = tmpdir.path().join("runtimes/qemu");
+        fs::create_dir_all(&target_dir).unwrap();
+        let target_file = target_dir.join("configuration-qemu.toml");
+        fs::write(&target_file, "config content").unwrap();
+
+        let original = tmpdir.path().join("configuration-qemu.toml");
+        fs::write(&original, "original").unwrap();
+
+        let symlink_target = "runtimes/qemu/configuration-qemu.toml";
+
+        // First call
+        atomic_symlink_replace(original.to_str().unwrap(), symlink_target).unwrap();
+        assert!(original.is_symlink());
+
+        // Second call (e.g. re-install) should succeed and still be a valid symlink
+        atomic_symlink_replace(original.to_str().unwrap(), symlink_target).unwrap();
+        assert!(original.is_symlink());
+        assert_eq!(
+            fs::read_link(&original).unwrap().to_str().unwrap(),
+            symlink_target
+        );
+    }
+
+    #[test]
+    fn test_atomic_symlink_replace_cleans_stale_temp() {
+        let tmpdir = tempfile::tempdir().unwrap();
+
+        let original = tmpdir.path().join("configuration-qemu.toml");
+        fs::write(&original, "original").unwrap();
+
+        // Simulate a stale temp symlink from an interrupted previous run
+        let stale_temp = tmpdir.path().join("configuration-qemu.toml.tmp-link");
+        std::os::unix::fs::symlink("stale-target", &stale_temp).unwrap();
+        assert!(stale_temp.is_symlink());
+
+        let target_dir = tmpdir.path().join("runtimes/qemu");
+        fs::create_dir_all(&target_dir).unwrap();
+        fs::write(target_dir.join("configuration-qemu.toml"), "content").unwrap();
+
+        let symlink_target = "runtimes/qemu/configuration-qemu.toml";
+        atomic_symlink_replace(original.to_str().unwrap(), symlink_target).unwrap();
+
+        assert!(original.is_symlink());
+        assert_eq!(
+            fs::read_link(&original).unwrap().to_str().unwrap(),
+            symlink_target
+        );
+        // Temp file should not linger
+        assert!(!stale_temp.exists() && !stale_temp.is_symlink());
+    }
+
+    #[test]
+    fn test_setup_and_remove_runtime_directory_symlink() {
+        let tmpdir = tempfile::tempdir().unwrap();
+
+        // Simulate the directory layout that setup_runtime_directory expects
+        // (after copy_artifacts has run), using a Go shim as example.
+        let defaults_dir = tmpdir.path().join("share/defaults/kata-containers");
+        fs::create_dir_all(&defaults_dir).unwrap();
+
+        let config_filename = "configuration-qemu.toml";
+        let original_config = defaults_dir.join(config_filename);
+        fs::write(
+            &original_config,
+            "[hypervisor.qemu]\npath = \"/usr/bin/qemu\"",
+        )
+        .unwrap();
+
+        // Create the runtime directory and copy the config (mimics setup_runtime_directory)
+        let runtime_dir = defaults_dir.join("runtimes/qemu");
+        let config_d_dir = runtime_dir.join("config.d");
+        fs::create_dir_all(&config_d_dir).unwrap();
+
+        let dest_config = runtime_dir.join(config_filename);
+        fs::copy(&original_config, &dest_config).unwrap();
+
+        // Atomically replace the original with a symlink
+        let symlink_target = format!("runtimes/qemu/{}", config_filename);
+        atomic_symlink_replace(original_config.to_str().unwrap(), &symlink_target).unwrap();
+
+        // Verify: original is now a symlink
+        assert!(original_config.is_symlink());
+        assert_eq!(
+            fs::read_link(&original_config).unwrap().to_str().unwrap(),
+            symlink_target
+        );
+
+        // Verify: reading through the symlink yields the real file content
+        assert_eq!(
+            fs::read_to_string(&original_config).unwrap(),
+            fs::read_to_string(&dest_config).unwrap()
+        );
+
+        // Verify: config.d is next to the real file (the resolved path)
+        assert!(dest_config.parent().unwrap().join("config.d").is_dir());
+
+        // Simulate remove_runtime_directory: remove symlink then runtime dir
+        assert!(original_config.is_symlink());
+        fs::remove_file(&original_config).unwrap();
+        assert!(!original_config.exists() && !original_config.is_symlink());
+
+        fs::remove_dir_all(&runtime_dir).unwrap();
+        assert!(!runtime_dir.exists());
+    }
 }