rm, chmod, chown, chgrp: compare (st_dev, st_ino) for --preserve-root

The root check compared path strings ("/" or a path whose canonicalize()
is "/"). A bind mount of "/" is an ordinary directory, so canonicalize()
returns the mountpoint and the check never matched: `mount --bind / /mnt;
rm -r /mnt` recursed into /mnt where GNU stops and errors.

Compare (st_dev, st_ino) against stat("/") instead, as GNU does, via a new
uucore::fs::path_is_root_dir that stats "/" once per process. This also
aligns the chmod/chown/chgrp guards: perms::is_root's syntactic "looks like
a directory?" pre-filter skipped a bind mount, and chmod only re-checked
symlinks during descent, not bind mounts met inside the tree.

Author: sylvestre

src/uu/chmod/src/chmod.rs

@@ -16,7 +16,7 @@ use uucore::display::Quotable;
 use uucore::error::{
     ExitCode, UError, UResult, USimpleError, UUsageError, set_exit_code, strip_errno,
 };
-use uucore::fs::{FileInformation, display_permissions_unix};
+use uucore::fs::{FileInformation, display_permissions_unix, path_is_root_dir};
 use uucore::mode;
 use uucore::perms::{TraverseSymlinks, configure_symlink_and_recursion};
 
@@ -516,7 +516,14 @@ impl Chmoder {
                 }
             }
             if self.recursive && self.preserve_root && Self::is_root(file) {
-                return Err(ChmodError::PreserveRoot("/".into()).into());
+                // Name the operand the user gave: with a bind mount of "/" it is
+                // not spelled "/", so say which path it is the same as.
+                return Err(if file.as_os_str() == "/" {
+                    ChmodError::PreserveRoot("/".into())
+                } else {
+                    ChmodError::PreserveRootSameAs(file.into())
+                }
+                .into());
             }
             if self.recursive {
                 let mut ancestors = HashSet::new();
@@ -530,19 +537,17 @@ impl Chmoder {
         r
     }
 
+    /// Whether `file` is `/`, by `(st_dev, st_ino)` rather than by name, so a
+    /// bind mount of `/` cannot slip past `--preserve-root` (as GNU does).
     fn is_root(file: impl AsRef<Path>) -> bool {
-        matches!(fs::canonicalize(&file), Ok(p) if p == Path::new("/"))
+        path_is_root_dir(file, true)
     }
 
-    /// `--preserve-root` guard for the recursive descent.
-    ///
-    /// The operand loop in [`Self::chmod`] only checks the paths named on the
-    /// command line. With `-L`, a symlink met *inside* the tree can resolve to
-    /// `/`, so the failsafe has to be re-checked at every descent or the
-    /// recursion walks straight into the real root. Only symlinks are
-    /// canonicalized, so ordinary trees pay nothing for this.
+    /// `--preserve-root` guard re-checked at every descent: a symlink to `/`
+    /// (under `-L`) or a bind mount of `/` met inside the tree is still `/`, so
+    /// the operand-only check is not enough. GNU re-checks every entry too.
     fn descends_into_root(&self, path: &Path) -> bool {
-        self.preserve_root && path.is_symlink() && Self::is_root(path)
+        self.preserve_root && Self::is_root(path)
     }
 
     // Non-safe traversal implementation for platforms without safe_traversal support
@@ -553,7 +558,8 @@ impl Chmoder {
         is_command_line_arg: bool,
         ancestors: &mut HashSet<FileInformation>,
     ) -> UResult<()> {
-        // Skip (and diagnose) a symlink that resolves to '/' before touching it.
+        // Skip (and diagnose) an entry that is '/' (a symlink to it, or a bind
+        // mount) before touching it.
         if self.descends_into_root(file_path) {
             show!(ChmodError::PreserveRootSameAs(file_path.into()));
             return Ok(());
@@ -630,7 +636,8 @@ impl Chmoder {
         is_command_line_arg: bool,
         ancestors: &mut HashSet<FileInformation>,
     ) -> UResult<()> {
-        // Skip (and diagnose) a symlink that resolves to '/' before touching it.
+        // Skip (and diagnose) an entry that is '/' (a symlink to it, or a bind
+        // mount) before touching it.
         if self.descends_into_root(file_path) {
             show!(ChmodError::PreserveRootSameAs(file_path.into()));
             return Ok(());

src/uu/rm/src/rm.rs

@@ -756,20 +756,27 @@ fn remove_dir_recursive(
     }
 }
 
-/// Check if a path resolves to the root directory.
+/// Check if a path is the root directory.
 /// Returns true if the path is root, false otherwise.
 fn is_root_path(path: &Path) -> bool {
-    // Check simple case: literal "/" path
+    // Check simple case: literal "/" path. Costs no syscall.
     if path.has_root() && path.parent().is_none() {
         return true;
     }
 
-    // Check if path resolves to "/" after following symlinks
+    // Otherwise settle by (st_dev, st_ino): a bind mount of "/" is a directory
+    // whose path never resolves to "/", so a name check misses it (symlinks too).
+    if uucore::fs::path_is_root_dir(path, true) {
+        return true;
+    }
+
+    // Platforms without (st_dev, st_ino) keep the name-based test.
+    #[cfg(not(unix))]
     if let Ok(canonical) = path.canonicalize() {
-        canonical.has_root() && canonical.parent().is_none()
-    } else {
-        false
+        return canonical.has_root() && canonical.parent().is_none();
     }
+
+    false
 }
 
 /// Show error message for attempting to remove root.

src/uucore/src/lib/features/fs.rs

@@ -27,6 +27,8 @@ use std::os::windows::ffi::{OsStrExt, OsStringExt};
 #[cfg(windows)]
 use std::os::windows::io::AsRawHandle;
 use std::path::{Component, MAIN_SEPARATOR, Path, PathBuf};
+#[cfg(unix)]
+use std::sync::OnceLock;
 #[cfg(windows)]
 use windows_sys::Win32::Foundation::MAX_PATH;
 #[cfg(windows)]
@@ -637,6 +639,36 @@ pub fn infos_refer_to_same_file(
     info1.is_ok() && info1.ok() == info2.ok()
 }
 
+/// The identity of `/`, stat'd once per process (like GNU's `get_root_dev_ino`).
+#[cfg(unix)]
+fn root_file_information() -> Option<&'static FileInformation> {
+    static ROOT: OnceLock<Option<FileInformation>> = OnceLock::new();
+    ROOT.get_or_init(|| FileInformation::from_path(Path::new("/"), true).ok())
+        .as_ref()
+}
+
+/// Whether `path` *is* `/`, by `(st_dev, st_ino)` rather than by name.
+///
+/// A bind mount of `/` (`mount --bind / /mnt`) is a real directory whose path
+/// never resolves to `/`, so a name-based `--preserve-root` check misses it;
+/// GNU compares dev/ino for the same reason. `dereference` says whether a
+/// symlink at `path` is about to be followed (only then does a link to `/`
+/// count). Returns `false` if `path` or `/` cannot be stat'd, or off unix.
+pub fn path_is_root_dir<P: AsRef<Path>>(path: P, dereference: bool) -> bool {
+    #[cfg(unix)]
+    {
+        let Some(root) = root_file_information() else {
+            return false;
+        };
+        FileInformation::from_path(path, dereference).is_ok_and(|info| &info == root)
+    }
+    #[cfg(not(unix))]
+    {
+        let _ = (path, dereference);
+        false
+    }
+}
+
 /// Check if two files are identical by comparing their contents.
 ///
 /// Returns `Ok(true)` if both files exist, are regular files, and have identical contents.
@@ -1450,4 +1482,31 @@ mod tests {
         // Non-existent file
         assert!(are_files_identical(file1.path(), "non_existent_file_path").is_err());
     }
+
+    #[cfg(unix)]
+    #[test]
+    fn test_path_is_root_dir() {
+        assert!(path_is_root_dir("/", true));
+        assert!(path_is_root_dir("/", false));
+        // Reached by a different name, still the same directory.
+        assert!(path_is_root_dir("/..", true));
+        assert!(path_is_root_dir("/tmp/..", true));
+
+        let dir = tempdir().unwrap();
+        assert!(!path_is_root_dir(dir.path(), true));
+        assert!(!path_is_root_dir(dir.path().join("nonexistent"), true));
+        assert!(!path_is_root_dir("", true));
+    }
+
+    /// A symlink to `/` counts only when the caller would follow it.
+    #[cfg(unix)]
+    #[test]
+    fn test_path_is_root_dir_symlink() {
+        let dir = tempdir().unwrap();
+        let link = dir.path().join("root-link");
+        unix::fs::symlink("/", &link).unwrap();
+
+        assert!(path_is_root_dir(&link, true));
+        assert!(!path_is_root_dir(&link, false));
+    }
 }

src/uucore/src/lib/features/perms.rs

@@ -27,6 +27,7 @@ use walkdir::WalkDir;
 
 #[cfg(target_os = "linux")]
 use crate::features::fs::FileInformation;
+use crate::features::fs::path_is_root_dir;
 #[cfg(target_os = "linux")]
 use crate::features::safe_traversal::{DirFd, SymlinkBehavior};
 
@@ -37,7 +38,7 @@ use std::io::Result as IOResult;
 use std::os::unix::fs::MetadataExt;
 
 use std::os::unix::ffi::OsStrExt;
-use std::path::{MAIN_SEPARATOR, Path};
+use std::path::Path;
 
 #[derive(Debug, Error)]
 enum PermsError {
@@ -224,56 +225,37 @@ pub fn check_root(path: &Path, would_recurse_symlink: bool) -> bool {
 
 /// In the context of chown and chgrp, check whether we are in a "preserve-root" scenario.
 ///
-/// In particular, we want to prohibit further traversal only if:
+/// Prohibit further traversal only if:
 ///     (--preserve-root and -R present) &&
-///     (path canonicalizes to "/") &&
+///     (path *is* "/" by (st_dev, st_ino), so a bind mount of "/" counts too) &&
 ///     (
 ///         (path is a symlink && would traverse/recurse this symlink) ||
 ///         (path is not a symlink)
 ///     )
-/// The first clause is checked by the caller, the second and third clause is checked here.
+/// The first clause is checked by the caller, the second and third here.
 /// The caller has to evaluate -P/-H/-L into 'would_recurse_symlink'.
-/// Recall that canonicalization resolves both relative paths (e.g. "..") and symlinks.
 fn is_root(path: &Path, would_traverse_symlink: bool) -> bool {
-    // The third clause can be evaluated without any syscalls, so we do that first.
-    // If we would_recurse_symlink, then the clause is true no matter whether the path is a symlink
-    // or not. Otherwise, we only need to check here if the path can syntactically be a symlink:
-    if !would_traverse_symlink {
-        // We cannot check path.is_dir() here, as this would resolve symlinks,
-        // which we need to avoid here.
-        // All directory-ish paths match "*/", except ".", "..", "*/.", and "*/..".
-        let path_bytes = path.as_os_str().as_encoded_bytes();
-        let looks_like_dir = path_bytes == *b"."
-            || path_bytes == *b".."
-            || path_bytes.ends_with(&[MAIN_SEPARATOR as u8])
-            || path_bytes.ends_with(&[MAIN_SEPARATOR as u8, b'.'])
-            || path_bytes.ends_with(&[MAIN_SEPARATOR as u8, b'.', b'.']);
-
-        if !looks_like_dir {
-            return false;
-        }
+    // Compare by (st_dev, st_ino), not name: a bind mount of "/" is an ordinary
+    // directory whose path never resolves to "/", so the old syntactic "looks
+    // like a directory?" pre-filter waved it through. `would_traverse_symlink`
+    // says whether a symlink to "/" here would be followed (only then is it root).
+    //
+    // FIXME: TOCTOU bug! This stat runs at a different time than the recursion
+    // decision it guards; GNU avoids the window by reusing fts's `struct stat`.
+    if !path_is_root_dir(path, would_traverse_symlink) {
+        return false;
     }
 
-    // FIXME: TOCTOU bug! canonicalize() runs at a different time than WalkDir's recursion decision.
-    // However, we're forced to make the decision whether to warn about --preserve-root
-    // *before* even attempting to chown the path, let alone doing the stat inside WalkDir.
-    if let Ok(p) = path.canonicalize() {
-        let path_buf = path.to_path_buf();
-        if p.parent().is_none() {
-            if path_buf.as_os_str() == "/" {
-                show_error!("it is dangerous to operate recursively on '/'");
-            } else {
-                show_error!(
-                    "it is dangerous to operate recursively on {} (same as '/')",
-                    path_buf.quote()
-                );
-            }
-            show_error!("use --no-preserve-root to override this failsafe");
-            return true;
-        }
+    if path.as_os_str() == "/" {
+        show_error!("it is dangerous to operate recursively on '/'");
+    } else {
+        show_error!(
+            "it is dangerous to operate recursively on {} (same as '/')",
+            path.quote()
+        );
     }
-
-    false
+    show_error!("use --no-preserve-root to override this failsafe");
+    true
 }
 
 pub fn get_metadata(file: &Path, follow: bool) -> std::io::Result<Metadata> {

tests/by-util/test_chmod.rs

@@ -565,13 +565,16 @@ fn test_chmod_preserve_root() {
 
 #[test]
 fn test_chmod_preserve_root_with_paths_that_resolve_to_root() {
+    // Only a bare "/" is reported as such; any other spelling of the root
+    // directory is named as the user wrote it, followed by "(same as '/')".
+    // "//" also checks we compare the raw operand, since Path("//") == Path("/").
     new_ucmd!()
         .arg("-R")
         .arg("--preserve-root")
         .arg("755")
-        .arg("/../")
+        .arg("//")
         .fails_with_code(1)
-        .stderr_contains("chmod: it is dangerous to operate recursively on '/'");
+        .stderr_contains("chmod: it is dangerous to operate recursively on '//' (same as '/')");
 }
 
 #[test]

tests/by-util/test_rm.rs

@@ -1511,6 +1511,78 @@ fn test_preserve_root_symlink_removal_without_trailing_slash() {
     assert!(!at.symlink_exists("rootlink"));
 }
 
+/// `--preserve-root` must refuse a bind mount of `/`.
+///
+/// A bind mount of `/` is an ordinary directory - not a symlink, not a cycle -
+/// so `canonicalize()` yields the mountpoint and every name-based check waves it
+/// through. Only `(st_dev, st_ino)` identifies it, and that is what the failsafe
+/// compares.
+///
+/// The mount is of the real root, so nothing here may be able to delete anything
+/// even with the guard gone: `-i` reads stdin at EOF and so answers "no" to the
+/// first "descend into directory?" prompt, before any unlink. **Never make this
+/// `-rf`.** The mount itself lives in a throwaway user + mount namespace.
+#[cfg(target_os = "linux")]
+#[test]
+fn test_preserve_root_bind_mount_of_root() {
+    use std::process::Command;
+
+    let ts = TestScenario::new(util_name!());
+
+    // Unprivileged user namespaces are disabled in some kernels and sandboxes.
+    let can_unshare = Command::new("unshare")
+        .args(["--user", "--map-root-user", "--mount", "true"])
+        .status()
+        .is_ok_and(|status| status.success());
+    if !can_unshare {
+        println!("TEST SKIPPED: no unprivileged mount namespace available");
+        return;
+    }
+
+    // Mount point lives in this test's own temp dir, so parallel runs cannot
+    // collide and it goes away with the scenario.
+    let mount_point = ts.fixtures.plus_as_string("rootbind");
+
+    // `mount --bind /` is refused inside a user namespace because of locked
+    // submounts; --make-rprivate + --rbind produces the same (st_dev, st_ino).
+    // The mount setup can still be blocked (e.g. `mount(2)` denied in a cross
+    // container even though `unshare` starts); if it fails, exit 99 so the test
+    // skips instead of failing for the wrong reason.
+    let script = format!(
+        "mkdir -p {mp}
+         {{ mount --make-rprivate / && mount --rbind / {mp}; }} \
+             || {{ echo MOUNT_SETUP_FAILED >&2; exit 99; }}
+         exec {bin} rm -ri --preserve-root {mp} < /dev/null",
+        mp = shell_quote(&mount_point),
+        bin = shell_quote(&ts.bin_path.to_string_lossy())
+    );
+    let output = Command::new("unshare")
+        .args(["--user", "--map-root-user", "--mount", "sh", "-c", &script])
+        .env("LC_ALL", "C")
+        .env("LANG", "C")
+        .env("LANGUAGE", "C")
+        .output()
+        .expect("failed to spawn unshare");
+
+    let stderr = String::from_utf8_lossy(&output.stderr);
+    if output.status.code() == Some(99) || stderr.contains("MOUNT_SETUP_FAILED") {
+        println!("TEST SKIPPED: could not set up a bind mount in the namespace: {stderr}");
+        return;
+    }
+    assert!(
+        stderr.contains("it is dangerous to operate recursively on")
+            && stderr.contains("(same as '/')"),
+        "--preserve-root did not refuse a bind mount of /: {stderr}"
+    );
+    assert!(!output.status.success());
+}
+
+/// Wrap `s` in single quotes for `sh -c`, escaping any single quote in it.
+#[cfg(target_os = "linux")]
+fn shell_quote(s: &str) -> String {
+    format!("'{}'", s.replace('\'', "'\\''"))
+}
+
 /// Test that literal "/" is still properly protected.
 #[test]
 fn test_preserve_root_literal_root() {