From 53cd18e0629ca6cebd61fe6daf03c6ac6f629bd4 Mon Sep 17 00:00:00 2001 From: Daniel Verkamp Date: Wed, 5 Oct 2022 16:21:43 -0700 Subject: [PATCH] p9: use *at() functions for set_attr Use fchmodat(), fchownat(), and utimensat() to implement the SET_ATTR request rather than using the non-'at' variants of these functions. These can operate on a file descriptor path using the /proc file handle and "self/fd/N" filename to modify the attributes of a file without actually opening it, which means we can avoid problems like not being able to open a read-only file with O_RDWR, which happened previously with chmod requests. This means we don't need to open the file at all, except in the case of a request that needs to set the size, since there is no equivalent truncateat() function. BUG=chromium:1369647 TEST=touch /mnt/chromeos/MyFiles/Downloads/hello.txt TEST=chmod -w /mnt/chromeos/MyFiles/Downloads/hello.txt TEST=chmod +w /mnt/chromeos/MyFiles/Downloads/hello.txt TEST=chmod a-r /mnt/chromeos/MyFiles/Downloads/hello.txt TEST=chmod a+r /mnt/chromeos/MyFiles/Downloads/hello.txt TEST=chown $USER /mnt/chromeos/MyFiles/Downloads/hello.txt TEST=truncate -s1 /mnt/chromeos/MyFiles/Downloads/hello.txt Change-Id: I0461ed231cc78b26bcc37ede1a364af984c87f8b Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/3935537 Reviewed-by: Alexandre Courbot Reviewed-by: Keiichi Watanabe Commit-Queue: Daniel Verkamp --- common/p9/src/server/mod.rs | 41 +++++++++++++++++++------------- seccomp/aarch64/9p_device.policy | 2 ++ seccomp/arm/9p_device.policy | 2 ++ seccomp/x86_64/9p_device.policy | 2 ++ 4 files changed, 31 insertions(+), 16 deletions(-) diff --git a/common/p9/src/server/mod.rs b/common/p9/src/server/mod.rs index af5549e827..e324ecc101 100644 --- a/common/p9/src/server/mod.rs +++ b/common/p9/src/server/mod.rs @@ -775,21 +775,13 @@ impl Server { fn set_attr(&mut self, set_attr: &Tsetattr) -> io::Result<()> { let fid = self.fids.get(&set_attr.fid).ok_or_else(ebadf)?; - - let file = if let Some(ref file) = fid.file { - MaybeOwned::Borrowed(file) - } else { - let flags = match fid.filetype { - FileType::Regular => P9_RDWR, - FileType::Directory => P9_RDONLY | P9_DIRECTORY, - FileType::Other => P9_RDWR, - }; - MaybeOwned::Owned(open_fid(&self.proc, &fid.path, P9_NONBLOCK | flags)?) - }; + let path = string_to_cstring(format!("self/fd/{}", fid.path.as_raw_fd()))?; if set_attr.valid & P9_SETATTR_MODE != 0 { // Safe because this doesn't modify any memory and we check the return value. - syscall!(unsafe { libc::fchmod(file.as_raw_fd(), set_attr.mode) })?; + syscall!(unsafe { + libc::fchmodat(self.proc.as_raw_fd(), path.as_ptr(), set_attr.mode, 0) + })?; } if set_attr.valid & (P9_SETATTR_UID | P9_SETATTR_GID) != 0 { @@ -805,10 +797,18 @@ impl Server { }; // Safe because this doesn't modify any memory and we check the return value. - syscall!(unsafe { libc::fchown(file.as_raw_fd(), uid, gid) })?; + syscall!(unsafe { libc::fchownat(self.proc.as_raw_fd(), path.as_ptr(), uid, gid, 0) })?; } if set_attr.valid & P9_SETATTR_SIZE != 0 { + let file = if fid.filetype == FileType::Directory { + return Err(io::Error::from_raw_os_error(libc::EISDIR)); + } else if let Some(ref file) = fid.file { + MaybeOwned::Borrowed(file) + } else { + MaybeOwned::Owned(open_fid(&self.proc, &fid.path, P9_NONBLOCK | P9_RDWR)?) + }; + file.set_len(set_attr.size)?; } @@ -837,7 +837,14 @@ impl Server { ]; // Safe because file is valid and we have initialized times fully. - let ret = unsafe { libc::futimens(file.as_raw_fd(), × as *const libc::timespec) }; + let ret = unsafe { + libc::utimensat( + self.proc.as_raw_fd(), + path.as_ptr(), + × as *const libc::timespec, + 0, + ) + }; if ret < 0 { return Err(io::Error::last_os_error()); } @@ -849,10 +856,12 @@ impl Server { // Setting -1 as the uid and gid will not actually change anything but will // still update the ctime. let ret = unsafe { - libc::fchown( - file.as_raw_fd(), + libc::fchownat( + self.proc.as_raw_fd(), + path.as_ptr(), libc::uid_t::max_value(), libc::gid_t::max_value(), + 0, ) }; if ret < 0 { diff --git a/seccomp/aarch64/9p_device.policy b/seccomp/aarch64/9p_device.policy index 14ead34208..8582eb1c33 100644 --- a/seccomp/aarch64/9p_device.policy +++ b/seccomp/aarch64/9p_device.policy @@ -23,7 +23,9 @@ socket: arg0 == AF_UNIX utimensat: 1 ftruncate: 1 fchmod: 1 +fchmodat: 1 fchown: 1 +fchownat: 1 fstatfs: 1 newfstatat: 1 prctl: arg0 == PR_SET_NAME diff --git a/seccomp/arm/9p_device.policy b/seccomp/arm/9p_device.policy index 1e9d96073c..cde9354e16 100644 --- a/seccomp/arm/9p_device.policy +++ b/seccomp/arm/9p_device.policy @@ -26,7 +26,9 @@ utimensat: 1 utimensat_time64: 1 ftruncate64: 1 fchmod: 1 +fchmodat: 1 fchown: 1 +fchownat: 1 fstatfs: 1 fstatfs64: 1 fstatat64: 1 diff --git a/seccomp/x86_64/9p_device.policy b/seccomp/x86_64/9p_device.policy index 7cfd4fdfd9..ddb7e6321c 100644 --- a/seccomp/x86_64/9p_device.policy +++ b/seccomp/x86_64/9p_device.policy @@ -25,7 +25,9 @@ fdatasync: 1 utimensat: 1 ftruncate: 1 fchmod: 1 +fchmodat: 1 fchown: 1 +fchownat: 1 fstatfs: 1 newfstatat: 1 prctl: arg0 == PR_SET_NAME