From 0577197caf75f9ffe6a7db0b9259ddde23e5e065 Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Thu, 7 Jul 2022 08:39:34 -0700 Subject: [PATCH 1/5] Guard against future `statx` fields. Future versions of Linux may recognize new `statx` mask flags, which enable writing to new `struct statx` fields. If a rustix user constructs a flags value with `from_bits_unchecked`, they could potentially create a flags value that causes `statx` on such a future Linux version to write fields outside the buffer, evoking undefined behavior. This is very unlikely to be a practical problem today, because: - Scanning all known public users of rustix, I don't see any constructing `StatxFlags` values this way. - It's unlikely that any rustix user would ever have a need to construct `StatxFlags` values this way. - No existing version of Linux increases the size of `struct statx` beyond what rustix currently knows about. - `struct statx` contains several spare fields, including a `__u64 __spare3[12]` specifically for expansion, so even if a future Linux version defines new fields, it has lots of space available before it needs to start increasing the size of `struct statx`. --- src/backend/libc/fs/syscalls.rs | 18 +++++++++++++ src/backend/linux_raw/fs/syscalls.rs | 20 +++++++++++++- tests/fs/main.rs | 1 + tests/fs/statx.rs | 39 ++++++++++++++++++++++++++++ 4 files changed, 77 insertions(+), 1 deletion(-) create mode 100644 tests/fs/statx.rs diff --git a/src/backend/libc/fs/syscalls.rs b/src/backend/libc/fs/syscalls.rs index 662804bb0..d44397672 100644 --- a/src/backend/libc/fs/syscalls.rs +++ b/src/backend/libc/fs/syscalls.rs @@ -1456,6 +1456,24 @@ pub(crate) fn statx( flags: AtFlags, mask: StatxFlags, ) -> io::Result { + // If a future Linux kernel adds more fields to `struct statx` and users + // passing flags unknown to rustix in `StatxFlags`, we could end up + // writing outside of the buffer. To prevent this possibility, we mask off + // any flags that we don't know about. + // + // This includes `STATX__RESERVED`, which has a value that we know, but + // which could take on arbitrary new meaning in the future. Linux currently + // rejects this flag with `EINVAL`, so we do the same. + // + // This doesn't rely on `STATX_ALL` because [it's deprecated] and already + // doesn't represent all the known flags. + // + // [it's deprecated]: https://patchwork.kernel.org/project/linux-fsdevel/patch/20200505095915.11275-7-mszeredi@redhat.com/ + if (mask.bits() & libc::STATX__RESERVED as u32) == libc::STATX__RESERVED as u32 { + return Err(io::Errno::INVAL); + } + let mask = mask & StatxFlags::all(); + let mut statx_buf = MaybeUninit::::uninit(); unsafe { ret(sys::statx( diff --git a/src/backend/linux_raw/fs/syscalls.rs b/src/backend/linux_raw/fs/syscalls.rs index f5b455dff..4cf0dcebb 100644 --- a/src/backend/linux_raw/fs/syscalls.rs +++ b/src/backend/linux_raw/fs/syscalls.rs @@ -38,7 +38,7 @@ use linux_raw_sys::general::{ __kernel_fsid_t, __kernel_timespec, open_how, statx, AT_EACCESS, AT_FDCWD, AT_REMOVEDIR, AT_SYMLINK_NOFOLLOW, F_ADD_SEALS, F_DUPFD, F_DUPFD_CLOEXEC, F_GETFD, F_GETFL, F_GETLEASE, F_GETOWN, F_GETPIPE_SZ, F_GETSIG, F_GET_SEALS, F_SETFD, F_SETFL, F_SETPIPE_SZ, SEEK_CUR, - SEEK_END, SEEK_SET, + SEEK_END, SEEK_SET, STATX__RESERVED, }; #[cfg(target_pointer_width = "32")] use { @@ -721,6 +721,24 @@ pub(crate) fn statx( flags: AtFlags, mask: StatxFlags, ) -> io::Result { + // If a future Linux kernel adds more fields to `struct statx` and users + // passing flags unknown to rustix in `StatxFlags`, we could end up + // writing outside of the buffer. To prevent this possibility, we mask off + // any flags that we don't know about. + // + // This includes `STATX__RESERVED`, which has a value that we know, but + // which could take on arbitrary new meaning in the future. Linux currently + // rejects this flag with `EINVAL`, so we do the same. + // + // This doesn't rely on `STATX_ALL` because [it's deprecated] and already + // doesn't represent all the known flags. + // + // [it's deprecated]: https://patchwork.kernel.org/project/linux-fsdevel/patch/20200505095915.11275-7-mszeredi@redhat.com/ + if (mask.bits() & STATX__RESERVED) == STATX__RESERVED { + return Err(io::Errno::INVAL); + } + let mask = mask & StatxFlags::all(); + unsafe { let mut statx_buf = MaybeUninit::::uninit(); ret(syscall!( diff --git a/tests/fs/main.rs b/tests/fs/main.rs index 279a517e4..453089b5f 100644 --- a/tests/fs/main.rs +++ b/tests/fs/main.rs @@ -37,5 +37,6 @@ mod readdir; mod renameat; #[cfg(not(any(target_os = "illumos", target_os = "redox", target_os = "wasi")))] mod statfs; +mod statx; mod utimensat; mod y2038; diff --git a/tests/fs/statx.rs b/tests/fs/statx.rs new file mode 100644 index 000000000..779e02471 --- /dev/null +++ b/tests/fs/statx.rs @@ -0,0 +1,39 @@ +#[cfg(any(target_os = "android", target_os = "linux"))] +#[test] +fn test_statx_unknown_flags() { + use rustix::fs::{AtFlags, StatxFlags}; + + let f = std::fs::File::open(".").unwrap(); + + // It's ok (though still unwise) to construct flags values that have + // unknown bits. Exclude `STATX__RESERVED` here as that evokes an explicit + // failure; that's tested separately below. + let too_many_flags = + unsafe { StatxFlags::from_bits_unchecked(!0 & !linux_raw_sys::general::STATX__RESERVED) }; + + // It's also ok to pass such flags to `statx`. + let result = rustix::fs::statx(&f, "Cargo.toml", AtFlags::empty(), too_many_flags).unwrap(); + + // But, rustix should mask off bits it doesn't recognize, because these + // extra flags may tell future kernels to set extra fields beyond the + // extend of rustix's statx buffer. So make sure we didn't get extra + // fields. + assert_eq!(result.stx_mask & !StatxFlags::all().bits(), 0); +} + +#[cfg(any(target_os = "android", target_os = "linux"))] +#[test] +fn test_statx_reserved() { + use rustix::fs::{AtFlags, StatxFlags}; + + let f = std::fs::File::open(".").unwrap(); + + // It's ok (though still unwise) to construct a `STATX__RESERVED` flag + // value but `statx` should reliably fail with `INVAL`. + let reserved = + unsafe { StatxFlags::from_bits_unchecked(linux_raw_sys::general::STATX__RESERVED) }; + assert_eq!( + rustix::fs::statx(&f, "Cargo.toml", AtFlags::empty(), reserved).unwrap_err(), + rustix::io::Errno::INVAL + ); +} From 0828d08876a8e578fa0be82f8adcf9555658272c Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Thu, 7 Jul 2022 09:04:10 -0700 Subject: [PATCH 2/5] Factor out a `cfg` from the `statx` test. --- tests/fs/main.rs | 1 + tests/fs/statx.rs | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/fs/main.rs b/tests/fs/main.rs index 453089b5f..0ef54eb53 100644 --- a/tests/fs/main.rs +++ b/tests/fs/main.rs @@ -37,6 +37,7 @@ mod readdir; mod renameat; #[cfg(not(any(target_os = "illumos", target_os = "redox", target_os = "wasi")))] mod statfs; +#[cfg(any(target_os = "android", target_os = "linux"))] mod statx; mod utimensat; mod y2038; diff --git a/tests/fs/statx.rs b/tests/fs/statx.rs index 779e02471..50a3f341a 100644 --- a/tests/fs/statx.rs +++ b/tests/fs/statx.rs @@ -1,4 +1,3 @@ -#[cfg(any(target_os = "android", target_os = "linux"))] #[test] fn test_statx_unknown_flags() { use rustix::fs::{AtFlags, StatxFlags}; @@ -21,7 +20,6 @@ fn test_statx_unknown_flags() { assert_eq!(result.stx_mask & !StatxFlags::all().bits(), 0); } -#[cfg(any(target_os = "android", target_os = "linux"))] #[test] fn test_statx_reserved() { use rustix::fs::{AtFlags, StatxFlags}; From 30fbdca092dd15b0141641769467ab10dbb24f2e Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Thu, 7 Jul 2022 09:05:32 -0700 Subject: [PATCH 3/5] Fix the definition of `STATX__RESERVED` on musl. --- src/backend/libc/fs/syscalls.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/backend/libc/fs/syscalls.rs b/src/backend/libc/fs/syscalls.rs index d44397672..248288979 100644 --- a/src/backend/libc/fs/syscalls.rs +++ b/src/backend/libc/fs/syscalls.rs @@ -1469,7 +1469,11 @@ pub(crate) fn statx( // doesn't represent all the known flags. // // [it's deprecated]: https://patchwork.kernel.org/project/linux-fsdevel/patch/20200505095915.11275-7-mszeredi@redhat.com/ - if (mask.bits() & libc::STATX__RESERVED as u32) == libc::STATX__RESERVED as u32 { + #[cfg(not(target_env = "musl"))] + const STATX__RESERVED: u32 = libc::STATX__RESERVED as u32; + #[cfg(target_env = "musl")] + const STATX__RESERVED: u32 = linux_raw_sys::general::STATX__RESERVED; + if (mask.bits() & STATX__RESERVED) == STATX__RESERVED { return Err(io::Errno::INVAL); } let mask = mask & StatxFlags::all(); From 49003f617f1cee5d96be3c384431439dcd8305ff Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Thu, 7 Jul 2022 10:02:09 -0700 Subject: [PATCH 4/5] Fix the definition of `STATX__RESERVED` on android. --- src/backend/libc/fs/syscalls.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/backend/libc/fs/syscalls.rs b/src/backend/libc/fs/syscalls.rs index 248288979..a1e962174 100644 --- a/src/backend/libc/fs/syscalls.rs +++ b/src/backend/libc/fs/syscalls.rs @@ -1469,9 +1469,9 @@ pub(crate) fn statx( // doesn't represent all the known flags. // // [it's deprecated]: https://patchwork.kernel.org/project/linux-fsdevel/patch/20200505095915.11275-7-mszeredi@redhat.com/ - #[cfg(not(target_env = "musl"))] + #[cfg(not(any(target_os = "android", target_env = "musl")))] const STATX__RESERVED: u32 = libc::STATX__RESERVED as u32; - #[cfg(target_env = "musl")] + #[cfg(any(target_os = "android", target_env = "musl"))] const STATX__RESERVED: u32 = linux_raw_sys::general::STATX__RESERVED; if (mask.bits() & STATX__RESERVED) == STATX__RESERVED { return Err(io::Errno::INVAL); From 3cdc0ea1cfffb373c91d61d1257d704caa8fba34 Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Thu, 7 Jul 2022 10:35:34 -0700 Subject: [PATCH 5/5] Fix compilation on platforms where `statx` doesn't implement `Debug`. --- tests/fs/statx.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/fs/statx.rs b/tests/fs/statx.rs index 50a3f341a..d02e9cf72 100644 --- a/tests/fs/statx.rs +++ b/tests/fs/statx.rs @@ -30,8 +30,8 @@ fn test_statx_reserved() { // value but `statx` should reliably fail with `INVAL`. let reserved = unsafe { StatxFlags::from_bits_unchecked(linux_raw_sys::general::STATX__RESERVED) }; - assert_eq!( - rustix::fs::statx(&f, "Cargo.toml", AtFlags::empty(), reserved).unwrap_err(), - rustix::io::Errno::INVAL - ); + match rustix::fs::statx(&f, "Cargo.toml", AtFlags::empty(), reserved) { + Ok(_) => panic!("statx succeeded with `STATX__RESERVED`"), + Err(err) => assert_eq!(err, rustix::io::Errno::INVAL), + } }