[rminnich]

We have been told that leaving /tmp/cpu up is considered a security risk. Once the namespace is ready, tear it down.

[Copilot]

🟡 Changes recommended

Critical teardown defects discard unmount errors and target the wrong path when TMPDIR is customized, allowing the command to run while the exported filesystem remains accessible.

2 open issues

• Unmount 9p at the configured temporary-directory path · New
• Propagate unmount failures and abort command launch · New

What changed in this PR

This PR adds cleanup of the temporary client filesystem after namespace setup, aiming to reduce filesystem exposure before the session command runs.

Changes:

• Attempts to unmount /tmp/cpu after namespace and CPU_FSTAB setup.
• Documents the security motivation for removing the mount.

File	Description
session/​session.go	Adds the unmount attempt before terminal setup and command execution.

_{Review effort: Balanced}

[brho]

from gemini:

A few issues with this change:

1. **Unmount errors are silently discarded, and `Run()` doesn't fail closed**:
   - `if err := unix.Unmount(...)` shadows `err`, so `err = errors.Join(errs, ...)` writes to the inner `err` variable that immediately goes out of
   scope.
   - Even if assigned to `errs`, `Session.Run()` only returns `errs` when `wtf != ""` (off in normal builds) or when `DropPrivs()` fails—at the end of
   `Run()` it returns `err` from `RunCmd(c)`. If unmounting `/tmp/cpu` fails (e.g. `EBUSY`), `Run()` will still drop privileges and run the user command
   with `/tmp/cpu` mounted.

2. **Unconditional `unix.Unmount` returns `EINVAL` when `/tmp/cpu` wasn't mounted**:
   - `s.TmpMounts()` always creates `filepath.Join(os.TempDir(), "cpu")` as an ordinary directory, but nothing is mounted on it when `CPUNONCE` is unset
   (`-9p=false` / `WithRoot("")`) and `CPU_FSTAB` doesn't mount `/tmp/cpu` (e.g., `TestRemoteNoNameSpace`, or `cpu` with `-9p=false -namespace=""`).
   - Once error handling in (1) is fixed, `unix.Unmount` on an unmounted `/tmp/cpu` directory will fail with `EINVAL` (or `ENOENT` if `/` or `/tmp` was
   mounted over). Ignoring `unix.EINVAL` and `unix.ENOENT` avoids breaking no-9p sessions.

3. **Use `filepath.Join(os.TempDir(), "cpu")` instead of hardcoded `"/tmp/cpu"`**:
   - `TmpMounts()` (`session/session.go:115`) and `Namespace()` (`session/session_linux.go:118`) use `filepath.Join(os.TempDir(), "cpu")`.

4. **`Session.NameSpace()` (`session/session.go:258`) also needs this**:
   - `cpuns` (`cmds/cpuns/main_linux.go:188`) calls `s.NameSpace()` rather than `s.Run()` when mounting NFS at `/tmp/cpu` and bind-mounting `CPU_FSTAB`,
   so `Session.NameSpace()` should also unmount `tmpCPU`.

5. **Tests / docs referencing `/tmp/cpu`**:
   - `vm/vm_linux_test.go` (`TestCPUAMD64`, `TestCPUARM`) runs `/bbin/dd if=/tmp/cpu/a of=/tmp/cpu/b`, and `cmds/cpu/doc.go` / `cmds/decpu/doc.go`
   document accessing `/tmp/cpu` from inside the session.

Suggested replacement in `Run()` (and `NameSpace()`):
```go
    tmpCPU := filepath.Join(os.TempDir(), "cpu")
    if err := unix.Unmount(tmpCPU, 0); err != nil && !errors.Is(err, unix.EINVAL) && !errors.Is(err, unix.ENOENT) {
        return errors.Join(errs, fmt.Errorf("CPUD:unmounting %s: %w", tmpCPU, err))
    }
```

[rminnich]

Thanks for review, great suggestions.

[Copilot]

🟡 Changes recommended

A failed unmount still allows the remote command to run with the mount accessible, and a successful command hides the teardown error, leaving a critical security cleanup gap.

1 open issue

• Abort command when unmount fails · New

2 resolved since last review

• Propagate unmount failures and abort command launch
• Unmount 9p at the configured temporary-directory path

_{Review effort: Balanced}