[sstriker]

This PR fixes one of the gaps that the harness-container survey (#2) found in our code: a read-only bind that is not recursive (docs/harness-containers.md, "A read-only bind that is not recursive").

The problem

The Linux backend binds a read-only grant with MS_BIND | MS_REC, then remounts it read-only. The remount applies to the top mount only. On WSL2, /usr/lib/wsl/lib and /usr/lib/modules/<kernel> are separate mounts under /usr, and they stayed read-write inside the sandbox. Only file ownership stopped a write. A submount that the user owns under a read-only grant was writable from the box.

The change (crates/sandbox/src/linux.rs)

• The recursive flags. After the read-only remount, bind() calls read_only_below(target). This calls mount_setattr(AT_FDCWD, target, AT_RECURSIVE, {MOUNT_ATTR_RDONLY | MOUNT_ATTR_NOSUID}), which sets the flags on the whole tree. It sets flags and clears none, so flags that the kernel locked stay. Every read-only grant goes through bind(): the system mounts, the image's rootfs, the harness, the memory binary, the projection and the system files.
• No mount_setattr. On a kernel before Linux 5.12, or under a seccomp profile that refuses mount_setattr, the launch fails with this message: "mount_setattr is not available here: it needs Linux 5.12 or later, and a seccomp profile that allows it". It does not fall back to a remount for each mount from /proc/self/mountinfo, because that cannot reach a submount that another mount hides. The cost: a host without mount_setattr cannot make a confined launch. --unconfined stays available.
• availability(), which amem doctor uses, now probes mount_setattr with arguments that the kernel refuses with EINVAL when the call exists. So doctor reports the same reason, and does not pass when the launch then fails.

Failing first

cargo test -p dm-sandbox --test read_only against main: exit 101.

probe: top 30
probe: submount ok
  left: {"submount": "ok", "top": "30"}
 right: {"submount": "30", "top": "30"}

The submount is a tmpfs that belongs to the caller, so ownership cannot be what refuses the write. The test expects EROFS (30).

Against 3708443, before the second commit:

• doctor without mount_setattr: {"availability": "ok", "launch": "refused"} under a filter that answers mount_setattr with ENOSYS.
• MOUNT_ATTR_NOSUID removed (a mutation): "submount_nosuid": "0" where "1" is expected.

make check: 622 tests, 622 passed, 3 skipped. The 3 skipped are probe binaries that the tests run inside the sandbox.

Adversarial pass

A fresh agent attacked 3708443. It verified these points:

• A writable grant under a read-only one stays writable. Each grant's mount_setattr runs before later binds, and the plan order puts the home and the work tree after the system mounts.
• Locked flags from an outer user namespace cause no EPERM.
• The WSL2 submounts under /usr are now read-only and nosuid inside.
• The current seccomp profiles of Moby, containerd and podman allow mount_setattr with CAP_SYS_ADMIN. The spike's kind profile allows it too.

It found two gaps, both fixed in 7ce1da5. Doctor passed where mount_setattr is missing, and the message blamed the kernel even when a seccomp profile was the cause. The nosuid half had no test.

Notes for review

• Depends on ci: run the confined tests on Linux, and fail rather than skip #18. The sandbox tests now fail on CI when the sandbox cannot run, and do not skip. On a runner that refuses unprivileged user namespaces, CI goes red. ci: run the confined tests on Linux, and fail rather than skip #18 lets the runner create user namespaces. ci: run the confined tests on Linux, and fail rather than skip #18 is now on main, and this branch has main merged in. Before this change, the tests skipped on CI: in run 40096054625 they reported ok about 2 ms after the binary started. The evidence above is from a WSL2 host with kernel 6.18.
• Merge with the seccomp fix (fix: refuse vsock and io_uring in the Linux sandbox #14). Both PRs change the same lines of availability(). The merge of main (fb89f0f) keeps both checks. The probe child returns 1 for the user namespace, 2 for mount_setattr and 3 for seccomp. make check after the merge: 643 tests, 643 passed, 6 skipped.

Second adversarial pass (after the merge of main)

A fresh agent attacked fb89f0f. With CI=1 the confined tests ran on a WSL2 host: read_only 2 passed, seccomp 3 passed, launch 20 passed. It found that the merge lost nothing. The probe order (user namespace, mount_setattr, seccomp) matches the order of a launch.

It found these problems:

• Fixed in 4585a6a. The probe took every answer except ENOSYS as the call. A seccomp profile whose default answer is EPERM passed the probe, and then every launch failed. The probe now takes only EINVAL as the call. A new test runs the probe under a filter that answers EPERM. Against the old probe it failed: left: {"availability": "ok", "launch": "refused"}.
• Not in this PR. amem doctor files a missing mount_setattr under "user namespaces" and gives the AppArmor fix. main has the same problem for seccomp. A separate change gives each reason its own check.
• Not in this PR. docs/harness-containers.md still lists this work as open. The docs PR after these fixes updates it.

make check after the fix: 644 tests, 644 passed, 6 skipped.

Posted by Claude on behalf of @sstriker

[Copilot]

🔵 Needs a closer look

It is security-critical confinement code that relies on raw unsafe mount-namespace syscalls whose correctness depends on kernel behavior that cannot be executed and fully verified in this review environment.

No open issues 🎉

What changed in this PR

This PR closes a confinement gap in the Linux namespaces backend: a read-only bind grant was only read-only at its top mount, so submounts copied in by MS_REC (for example /usr/lib/wsl/lib under /usr on WSL2) stayed writable inside the sandbox when the caller owned them. The fix makes a read-only grant read-only (and nosuid) all the way down, and makes amem doctor report the same precondition the launch requires.

Changes:

• Add read_only_below() in crates/sandbox/src/linux.rs, invoked from bind() after the read-only remount, which calls mount_setattr(AT_RECURSIVE, {MOUNT_ATTR_RDONLY | MOUNT_ATTR_NOSUID}) to apply the flags to the whole subtree; a missing mount_setattr fails the launch rather than silently leaving a submount writable.
• Teach availability() to probe mount_setattr (via has_mount_setattr()) in the child and surface a dedicated NO_MOUNT_SETATTR message so doctor and the launch agree.
• Add integration tests covering the recursive read-only/nosuid behavior over a tmpfs submount and the mount_setattr-absent (ENOSYS seccomp) path, plus a shared common test harness for running probes inside the sandbox.

File	Description
crates/​sandbox/​src/​linux.rs	Adds recursive read-only propagation via mount_setattr, a has_mount_setattr probe, and wires both into bind() and availability().
crates/​sandbox/​tests/​read_only.rs	New integration tests: submount is read-only/nosuid inside, and a missing mount_setattr is reported and refuses launch.
crates/​sandbox/​tests/​common/​mod.rs	New shared harness to build a Plan and run an ignored probe test inside the namespaces sandbox.

I reviewed the syscall argument order and mount_attr layout in read_only_below, the size=0 → EINVAL vs ENOSYS logic in has_mount_setattr, the availability() exit-code mapping (0/1/2) and its unchanged Result<(), String> contract used by doctor.rs, and the test harness (including how the launch error reaches stderr through the pre_exec eprintln). I did not find a concrete defect.

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

It changes security-critical unsafe sandbox confinement with new syscall semantics and has coordinated cross-PR merge dependencies (#14, #18) on the same availability() lines, which warrant final human review.

No open issues 🎉

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

It changes security-critical sandbox confinement using unsafe low-level mount_setattr syscalls and pre-exec namespace code, where subtle kernel-semantics errors warrant final human review.

No open issues 🎉

_{Review effort: Balanced}