sandlock-core: harden upper-write escape test and drop redundant re-lock

Signed-off-by: Cong Wang <cwang@multikernel.io>

Author: congwang-mk

crates/sandlock-core/src/cow/dispatch.rs

@@ -550,8 +550,9 @@ pub(crate) async fn handle_cow_write(
         None => return NotifAction::Continue,
     };
 
-    // Phase 1: check if we need to pre-copy a file (under lock, no heavy I/O)
-    let (copy_plan, copy_workdir, copy_rel) = {
+    // Phase 1: check if we need to pre-copy a file (under lock, no heavy I/O).
+    // Capture both layer roots here so Phase 2 needs no second lock.
+    let (copy_plan, copy_workdir, copy_upper, copy_rel) = {
         let mut st = cow_state.lock().await;
         let cow = match st.branch.as_mut() {
             Some(c) => c,
@@ -562,30 +563,21 @@ pub(crate) async fn handle_cow_write(
         match cow_copy_rel(&op, cow) {
             Some((_match_path, ref rel)) => {
                 let workdir = cow.workdir().to_path_buf();
+                let upper = cow.upper_dir().to_path_buf();
                 match cow.prepare_copy(rel) {
-                    Ok(plan) => (Some(plan), workdir, rel.clone()),
+                    Ok(plan) => (Some(plan), workdir, upper, rel.clone()),
                     Err(crate::error::BranchError::QuotaExceeded) => return NotifAction::Errno(libc::ENOSPC),
                     Err(_) => return NotifAction::Continue,
                 }
             }
-            None => (None, std::path::PathBuf::new(), String::new()),
+            None => (None, std::path::PathBuf::new(), std::path::PathBuf::new(), String::new()),
         }
     };
     // Lock is released here
 
     // Phase 2: execute the file copy outside the lock (if needed)
     if let Some(crate::cow::seccomp::CowCopyPlan::NeedsCopy { upper, lower: _lower, file_size }) = copy_plan {
-        // copy_workdir is workdir_root; we need upper_root for the dest side.
-        // Re-acquire it briefly: execute_deferred_copy needs both roots.
-        let uroot = {
-            let st = cow_state.lock().await;
-            st.branch.as_ref().map(|c| c.upper_dir().to_path_buf())
-        };
-        let uroot = match uroot {
-            Some(p) => p,
-            None => return NotifAction::Continue,
-        };
-        if execute_deferred_copy(cow_state, copy_workdir, uroot, copy_rel, upper, file_size).await.is_none() {
+        if execute_deferred_copy(cow_state, copy_workdir, copy_upper, copy_rel, upper, file_size).await.is_none() {
             return NotifAction::Continue;
         }
     }

crates/sandlock-core/src/cow/seccomp.rs

@@ -1631,32 +1631,34 @@ mod tests {
 
     #[test]
     fn upper_write_does_not_escape_through_symlink() {
-        // workdir/evil -> /etc (absolute symlink). Copy it up verbatim into
-        // upper so upper/evil is also an absolute symlink. Then invoke
-        // handle_mkdir with a path that would go through that symlink.
-        // The confined mkdirp_in_root must clamp the walk to the upper root
-        // and never create /etc/sandlock_escape_dir on the host.
+        // workdir/evil -> <outside> (absolute symlink to a writable dir
+        // outside the sandbox). Copy it up verbatim so upper/evil is also that
+        // absolute symlink, then mkdir through it. The confined mkdirp must
+        // clamp to the upper root, refuse, and never create the dir in
+        // <outside>. Pointing at a writable TempDir (not /etc) means the test
+        // distinguishes the fix from the old lexical code even when not root.
         let (workdir, storage) = setup_workdir();
-        std::os::unix::fs::symlink("/etc", workdir.path().join("evil")).unwrap();
+        let outside = tempfile::tempdir().unwrap();
+        std::os::unix::fs::symlink(outside.path(), workdir.path().join("evil")).unwrap();
         let mut branch =
             SeccompCowBranch::create(workdir.path(), Some(storage.path()), 0).unwrap();
 
-        // Copy up the symlink so upper/evil -> /etc verbatim.
         branch.ensure_cow_copy("evil").unwrap();
         assert!(
             branch.upper_dir().join("evil").is_symlink(),
             "precondition: upper/evil must be a verbatim symlink"
         );
 
-        // Attempt to mkdir via the symlinked parent component.
         let wd = workdir.path().canonicalize().unwrap();
         let escape_path = format!("{}/evil/sandlock_escape_dir", wd.display());
-        let _ = branch.handle_mkdir(&escape_path);
-
-        // The confined write must not have escaped.
+        // Confined: the write is clamped to the upper root and must be refused.
+        assert!(
+            !branch.handle_mkdir(&escape_path).unwrap(),
+            "handle_mkdir reported success writing through an escaping symlink"
+        );
         assert!(
-            !std::path::Path::new("/etc/sandlock_escape_dir").exists(),
-            "upper write escaped through symlinked parent into /etc"
+            !outside.path().join("sandlock_escape_dir").exists(),
+            "upper write escaped through symlinked parent into the outside dir"
         );
     }
 }