devices: fs: Strip padding from directory entry names

When calling `getdents64`, the kernel will add additional nul bytes to
the name of the directory entry to make sure the whole thing is 8-byte
aligned.

Previously we would pass on this padded name to the kernel driver.
However, this seems to prevent the driver from detecting the "." and
".." entries, leading to the driver printing warnings like

  VFS: Lookup of '.' in virtiofs virtiofs would have caused loop

Strip out the padding so that the kernel detection of the "." and ".."
entries can work properly.

BUG=b:153677176
TEST=vm.Virtiofs and manually start a vm and check that the kernel
     doesn't print warnings about lookups causing loops

Change-Id: Id015182186cc3cb076e27556a1ab0a2de710aa59
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2145547
Auto-Submit: Chirantan Ekbote <chirantan@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Commit-Queue: Chirantan Ekbote <chirantan@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>

3 files changed, 53 insertions, 15 deletions

diff --git a/devices/src/virtio/fs/filesystem.rs b/devices/src/virtio/fs/filesystem.rs
index eb9726c..3f9528f 100644
--- a/devices/src/virtio/fs/filesystem.rs
+++ b/devices/src/virtio/fs/filesystem.rs
@@ -77,7 +77,7 @@ pub struct DirEntry<'a> {
 
     /// The name of this directory entry. There are no requirements for the contents of this field
     /// and any sequence of bytes is considered valid.
-    pub name: &'a [u8],
+    pub name: &'a CStr,
 }
 
 /// A reply to a `getxattr` method call.
diff --git a/devices/src/virtio/fs/passthrough.rs b/devices/src/virtio/fs/passthrough.rs
index 344ddc7..60f9655 100644
--- a/devices/src/virtio/fs/passthrough.rs
+++ b/devices/src/virtio/fs/passthrough.rs
@@ -197,6 +197,20 @@ fn stat(f: &File) -> io::Result<libc::stat64> {
     }
 }
 
+// Like `CStr::from_bytes_with_nul` but strips any bytes after the first '\0'-byte. Panics if `b`
+// doesn't contain any '\0' bytes.
+fn strip_padding(b: &[u8]) -> &CStr {
+    // It would be nice if we could use memchr here but that's locked behind an unstable gate.
+    let pos = b
+        .iter()
+        .position(|&c| c == 0)
+        .expect("`b` doesn't contain any nul bytes");
+
+    // Safe because we are creating this string with the first nul-byte we found so we can
+    // guarantee that it is nul-terminated and doesn't contain any interior nuls.
+    unsafe { CStr::from_bytes_with_nul_unchecked(&b[..pos + 1]) }
+}
+
 /// The caching policy that the file system should report to the FUSE client. By default the FUSE
 /// protocol uses close-to-open consistency. This means that any cached contents of the file are
 /// invalidated the next time that file is opened.
@@ -574,7 +588,9 @@ impl PassthroughFs {
             let namelen = dirent64.d_reclen as usize - size_of::<LinuxDirent64>();
             debug_assert!(namelen <= back.len(), "back is smaller than `namelen`");
 
-            let name = &back[..namelen];
+            // The kernel will pad the name with additional nul bytes until it is 8-byte aligned so
+            // we need to strip those off here.
+            let name = strip_padding(&back[..namelen]);
             let res = add_entry(DirEntry {
                 ino: dirent64.d_ino,
                 offset: dirent64.d_off as u64,
@@ -926,14 +942,8 @@ impl FileSystem for PassthroughFs {
         F: FnMut(DirEntry, Entry) -> io::Result<usize>,
     {
         self.do_readdir(inode, handle, size, offset, |dir_entry| {
-            // Safe because the kernel guarantees that the buffer is nul-terminated. Additionally,
-            // the kernel will pad the name with '\0' bytes up to 8-byte alignment and there's no
-            // way for us to know exactly how many padding bytes there are. This would cause
-            // `CStr::from_bytes_with_nul` to return an error because it would think there are
-            // interior '\0' bytes. We trust the kernel to provide us with properly formatted data
-            // so we'll just skip the checks here.
-            let name = unsafe { CStr::from_bytes_with_nul_unchecked(dir_entry.name) };
-            let entry = if name.to_bytes() == b"." || name.to_bytes() == b".." {
+            let name = dir_entry.name.to_bytes();
+            let entry = if name == b"." || name == b".." {
                 // Don't do lookups on the current directory or the parent directory. Safe because
                 // this only contains integer fields and any value is valid.
                 let mut attr = unsafe { MaybeUninit::<libc::stat64>::zeroed().assume_init() };
@@ -949,7 +959,7 @@ impl FileSystem for PassthroughFs {
                     entry_timeout: Duration::from_secs(0),
                 }
             } else {
-                self.do_lookup(inode, name)?
+                self.do_lookup(inode, dir_entry.name)?
             };
 
             add_entry(dir_entry, entry)
@@ -1757,3 +1767,29 @@ impl FileSystem for PassthroughFs {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn padded_cstrings() {
+        assert_eq!(strip_padding(b".\0\0\0\0\0\0\0").to_bytes(), b".");
+        assert_eq!(strip_padding(b"..\0\0\0\0\0\0").to_bytes(), b"..");
+        assert_eq!(
+            strip_padding(b"normal cstring\0").to_bytes(),
+            b"normal cstring"
+        );
+        assert_eq!(strip_padding(b"\0\0\0\0").to_bytes(), b"");
+        assert_eq!(
+            strip_padding(b"interior\0nul bytes\0\0\0").to_bytes(),
+            b"interior"
+        );
+    }
+
+    #[test]
+    #[should_panic(expected = "`b` doesn't contain any nul bytes")]
+    fn no_nul_byte() {
+        strip_padding(b"no nul bytes in string");
+    }
+}
diff --git a/devices/src/virtio/fs/server.rs b/devices/src/virtio/fs/server.rs
index c9025f2..bb5f8b6 100644
--- a/devices/src/virtio/fs/server.rs
+++ b/devices/src/virtio/fs/server.rs
@@ -1369,12 +1369,14 @@ fn add_dirent(
     d: DirEntry,
     entry: Option<Entry>,
 ) -> io::Result<usize> {
-    if d.name.len() > ::std::u32::MAX as usize {
+    // Strip the trailing '\0'.
+    let name = d.name.to_bytes();
+    if name.len() > ::std::u32::MAX as usize {
         return Err(io::Error::from_raw_os_error(libc::EOVERFLOW));
     }
 
     let dirent_len = size_of::<Dirent>()
-        .checked_add(d.name.len())
+        .checked_add(name.len())
         .ok_or_else(|| io::Error::from_raw_os_error(libc::EOVERFLOW))?;
 
     // Directory entries must be padded to 8-byte alignment.  If adding 7 causes
@@ -1402,12 +1404,12 @@ fn add_dirent(
         let dirent = Dirent {
             ino: d.ino,
             off: d.offset,
-            namelen: d.name.len() as u32,
+            namelen: name.len() as u32,
             type_: d.type_,
         };
 
         cursor.write_all(dirent.as_slice())?;
-        cursor.write_all(d.name)?;
+        cursor.write_all(name)?;
 
         // We know that `dirent_len` <= `padded_dirent_len` due to the check above
         // so there's no need for checked arithmetic.