* [PATCH v2] vfs: Improve overflow checking for stat*() compat fields
2017-10-06 1:31 ` Linus Torvalds
@ 2017-10-18 16:04 ` Sergey Klyaus
2018-08-06 17:06 ` [PATCH] vfs: fix statfs64() returning impossible EOVERFLOW for 64-bit f_files Al Viro
1 sibling, 0 replies; 9+ messages in thread
From: Sergey Klyaus @ 2017-10-18 16:04 UTC (permalink / raw)
To: Sergey Klyaus; +Cc: Al Viro, linux-fsdevel, Linux Kernel Mailing List
The checks for overflown fields in compat_statfs[64] structures are
heuristic and are not error prone. This patch introduces
ASSIGN_CHECK_OVERFLOW() family of macros which assign a field from a
kernel representation and check if value is get truncated or changed
its sign if the types of compat and in-kernel fields are different (and
if so, they set a flag to a "true").
These macros may use compiler builtin for overflow detection. They are
also used for stat*() syscalls.
Signed-off-by: Sergey Klyaus <sergey.m.klyaus@gmail.com>
---
This is replacement for vfs: fix statfs64() returning impossible EOVERFLOW for
64-bit f_files
fs/stat.c | 33 ++++----
fs/statfs.c | 195 +++++++++++++++++++++++++------------------
include/linux/build_bug.h | 10 +++
include/linux/compiler-gcc.h | 5 ++
include/linux/compiler.h | 43 ++++++++++
5 files changed, 190 insertions(+), 96 deletions(-)
diff --git a/fs/stat.c b/fs/stat.c
index 8a6aa8c..e18539c 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -16,6 +16,7 @@
#include <linux/syscalls.h>
#include <linux/pagemap.h>
#include <linux/compat.h>
+#include <linux/compiler.h>
#include <linux/uaccess.h>
#include <asm/unistd.h>
@@ -207,6 +208,7 @@ static int cp_old_stat(struct kstat *stat, struct __old_kernel_stat __user * sta
{
static int warncount = 5;
struct __old_kernel_stat tmp;
+ bool offlag = false;
if (warncount > 0) {
warncount--;
@@ -219,12 +221,10 @@ static int cp_old_stat(struct kstat *stat, struct __old_kernel_stat __user * sta
memset(&tmp, 0, sizeof(struct __old_kernel_stat));
tmp.st_dev = old_encode_dev(stat->dev);
- tmp.st_ino = stat->ino;
- if (sizeof(tmp.st_ino) < sizeof(stat->ino) && tmp.st_ino != stat->ino)
- return -EOVERFLOW;
+ ASSIGN_CHECK_OVERFLOW(tmp.st_ino, stat->ino, offlag);
tmp.st_mode = stat->mode;
- tmp.st_nlink = stat->nlink;
- if (tmp.st_nlink != stat->nlink)
+ ASSIGN_CHECK_OVERFLOW(tmp.st_nlink, stat->nlink, offlag);
+ if (unlikely(offlag))
return -EOVERFLOW;
SET_UID(tmp.st_uid, from_kuid_munged(current_user_ns(), stat->uid));
SET_GID(tmp.st_gid, from_kgid_munged(current_user_ns(), stat->gid));
@@ -237,6 +237,7 @@ static int cp_old_stat(struct kstat *stat, struct __old_kernel_stat __user * sta
tmp.st_atime = stat->atime.tv_sec;
tmp.st_mtime = stat->mtime.tv_sec;
tmp.st_ctime = stat->ctime.tv_sec;
+
return copy_to_user(statbuf,&tmp,sizeof(tmp)) ? -EFAULT : 0;
}
@@ -295,6 +296,7 @@ SYSCALL_DEFINE2(fstat, unsigned int, fd, struct __old_kernel_stat __user *, stat
static int cp_new_stat(struct kstat *stat, struct stat __user *statbuf)
{
struct stat tmp;
+ bool offlag = false;
if (!valid_dev(stat->dev) || !valid_dev(stat->rdev))
return -EOVERFLOW;
@@ -305,12 +307,11 @@ static int cp_new_stat(struct kstat *stat, struct stat __user *statbuf)
INIT_STRUCT_STAT_PADDING(tmp);
tmp.st_dev = encode_dev(stat->dev);
- tmp.st_ino = stat->ino;
- if (sizeof(tmp.st_ino) < sizeof(stat->ino) && tmp.st_ino != stat->ino)
- return -EOVERFLOW;
+ ASSIGN_CHECK_OVERFLOW(tmp.st_ino, stat->ino, offlag);
tmp.st_mode = stat->mode;
tmp.st_nlink = stat->nlink;
- if (tmp.st_nlink != stat->nlink)
+ ASSIGN_CHECK_OVERFLOW(tmp.st_nlink, stat->nlink, offlag);
+ if (unlikely(offlag))
return -EOVERFLOW;
SET_UID(tmp.st_uid, from_kuid_munged(current_user_ns(), stat->uid));
SET_GID(tmp.st_gid, from_kgid_munged(current_user_ns(), stat->gid));
@@ -431,6 +432,7 @@ SYSCALL_DEFINE3(readlink, const char __user *, path, char __user *, buf,
static long cp_new_stat64(struct kstat *stat, struct stat64 __user *statbuf)
{
struct stat64 tmp;
+ bool offlag = false;
INIT_STRUCT_STAT64_PADDING(tmp);
#ifdef CONFIG_MIPS
@@ -441,8 +443,8 @@ static long cp_new_stat64(struct kstat *stat, struct stat64 __user *statbuf)
tmp.st_dev = huge_encode_dev(stat->dev);
tmp.st_rdev = huge_encode_dev(stat->rdev);
#endif
- tmp.st_ino = stat->ino;
- if (sizeof(tmp.st_ino) < sizeof(stat->ino) && tmp.st_ino != stat->ino)
+ ASSIGN_CHECK_OVERFLOW(tmp.st_ino, stat->ino, offlag);
+ if (unlikely(offlag))
return -EOVERFLOW;
#ifdef STAT64_HAS_BROKEN_ST_INO
tmp.__st_ino = stat->ino;
@@ -580,18 +582,17 @@ SYSCALL_DEFINE5(statx,
static int cp_compat_stat(struct kstat *stat, struct compat_stat __user *ubuf)
{
struct compat_stat tmp;
+ bool offlag = false;
if (!old_valid_dev(stat->dev) || !old_valid_dev(stat->rdev))
return -EOVERFLOW;
memset(&tmp, 0, sizeof(tmp));
tmp.st_dev = old_encode_dev(stat->dev);
- tmp.st_ino = stat->ino;
- if (sizeof(tmp.st_ino) < sizeof(stat->ino) && tmp.st_ino != stat->ino)
- return -EOVERFLOW;
+ ASSIGN_CHECK_OVERFLOW(tmp.st_ino, stat->ino, offlag);
tmp.st_mode = stat->mode;
- tmp.st_nlink = stat->nlink;
- if (tmp.st_nlink != stat->nlink)
+ ASSIGN_CHECK_OVERFLOW(tmp.st_nlink, stat->nlink, offlag);
+ if (unlikely(offlag))
return -EOVERFLOW;
SET_UID(tmp.st_uid, from_kuid_munged(current_user_ns(), stat->uid));
SET_GID(tmp.st_gid, from_kgid_munged(current_user_ns(), stat->gid));
diff --git a/fs/statfs.c b/fs/statfs.c
index fab9b6a..47cf963 100644
--- a/fs/statfs.c
+++ b/fs/statfs.c
@@ -8,8 +8,34 @@
#include <linux/security.h>
#include <linux/uaccess.h>
#include <linux/compat.h>
+#include <linux/compiler.h>
+#include <linux/bitops.h>
#include "internal.h"
+/**
+ * ASSIGN_CHECK_TRUNCATED_BITS() assigns src value to dst and checks if
+ * neither set bit in src lost during assignment. It uses fls()/fls64()
+ * to avoid sign extension induced by 32-bit -> 64-bit conversion.
+ */
+#define fls3264(value) \
+ ({ \
+ typeof(value) __val = (value); \
+ int __last_bit = 0; \
+ if (sizeof(__val) == 8) { \
+ __last_bit = fls64(__val); \
+ } else { \
+ __last_bit = fls(__val); \
+ } \
+ __last_bit; \
+ })
+#define ASSIGN_CHECK_TRUNCATED_BITS(dest, src, offlag) \
+ do { \
+ typeof(src) __src = (src); \
+ typeof(dest) __dst = (dest = __src); \
+ if (fls3264(__dst) != fls3264(__src)) \
+ offlag = true; \
+ } while (0)
+
static int flags_by_mnt(int mnt_flags)
{
int flags = 0;
@@ -110,39 +136,41 @@ static int do_statfs_native(struct kstatfs *st, struct statfs __user *p)
{
struct statfs buf;
- if (sizeof(buf) == sizeof(*st))
+ if (sizeof(buf) == sizeof(*st)) {
memcpy(&buf, st, sizeof(*st));
- else {
- if (sizeof buf.f_blocks == 4) {
- if ((st->f_blocks | st->f_bfree | st->f_bavail |
- st->f_bsize | st->f_frsize) &
- 0xffffffff00000000ULL)
- return -EOVERFLOW;
- /*
- * f_files and f_ffree may be -1; it's okay to stuff
- * that into 32 bits
- */
- if (st->f_files != -1 &&
- (st->f_files & 0xffffffff00000000ULL))
- return -EOVERFLOW;
- if (st->f_ffree != -1 &&
- (st->f_ffree & 0xffffffff00000000ULL))
- return -EOVERFLOW;
- }
-
- buf.f_type = st->f_type;
- buf.f_bsize = st->f_bsize;
- buf.f_blocks = st->f_blocks;
- buf.f_bfree = st->f_bfree;
- buf.f_bavail = st->f_bavail;
- buf.f_files = st->f_files;
- buf.f_ffree = st->f_ffree;
- buf.f_fsid = st->f_fsid;
- buf.f_namelen = st->f_namelen;
- buf.f_frsize = st->f_frsize;
- buf.f_flags = st->f_flags;
+ } else {
+ bool offlag = false;
+ /* f_type can be signed 32-bits on some architectures, but it
+ * contains magic value, so we do not care if value overflows
+ * destination structure field if bits are intact.
+ */
+ ASSIGN_CHECK_TRUNCATED_BITS(buf.f_type, st->f_type, offlag);
+ if (WARN_ON(offlag))
+ return -EOVERFLOW;
+ ASSIGN_CHECK_OVERFLOW(buf.f_namelen, st->f_namelen, offlag);
+ if (WARN_ON(offlag))
+ return -EOVERFLOW;
+ ASSIGN_CHECK_OVERFLOW(buf.f_bsize, st->f_bsize, offlag);
+ ASSIGN_CHECK_OVERFLOW(buf.f_blocks, st->f_blocks, offlag);
+ ASSIGN_CHECK_OVERFLOW(buf.f_bfree, st->f_bfree, offlag);
+ ASSIGN_CHECK_OVERFLOW(buf.f_bavail, st->f_bavail, offlag);
+ /* f_files and f_ffree may be -1; it's okay to put that into
+ * 32 bits
+ */
+ ASSIGN_CHECK_OVERFLOW_EXCEPT(buf.f_files, st->f_files, offlag,
+ 0xffffffffffffffffULL);
+ ASSIGN_CHECK_OVERFLOW_EXCEPT(buf.f_ffree, st->f_ffree, offlag,
+ 0xffffffffffffffffULL);
+ ASSIGN_CHECK_SAME_TYPE(buf.f_fsid.val[0], st->f_fsid.val[0]);
+ ASSIGN_CHECK_SAME_TYPE(buf.f_fsid.val[1], st->f_fsid.val[1]);
+ ASSIGN_CHECK_OVERFLOW(buf.f_frsize, st->f_frsize, offlag);
+ ASSIGN_CHECK_TRUNCATED_BITS(buf.f_flags, st->f_flags, offlag);
memset(buf.f_spare, 0, sizeof(buf.f_spare));
+
+ if (unlikely(offlag))
+ return -EOVERFLOW;
}
+
if (copy_to_user(p, &buf, sizeof(buf)))
return -EFAULT;
return 0;
@@ -247,32 +275,37 @@ SYSCALL_DEFINE2(ustat, unsigned, dev, struct ustat __user *, ubuf)
static int put_compat_statfs(struct compat_statfs __user *ubuf, struct kstatfs *kbuf)
{
struct compat_statfs buf;
- if (sizeof ubuf->f_blocks == 4) {
- if ((kbuf->f_blocks | kbuf->f_bfree | kbuf->f_bavail |
- kbuf->f_bsize | kbuf->f_frsize) & 0xffffffff00000000ULL)
- return -EOVERFLOW;
- /* f_files and f_ffree may be -1; it's okay
- * to stuff that into 32 bits */
- if (kbuf->f_files != 0xffffffffffffffffULL
- && (kbuf->f_files & 0xffffffff00000000ULL))
- return -EOVERFLOW;
- if (kbuf->f_ffree != 0xffffffffffffffffULL
- && (kbuf->f_ffree & 0xffffffff00000000ULL))
- return -EOVERFLOW;
- }
+ bool offlag = false;
+
memset(&buf, 0, sizeof(struct compat_statfs));
- buf.f_type = kbuf->f_type;
- buf.f_bsize = kbuf->f_bsize;
- buf.f_blocks = kbuf->f_blocks;
- buf.f_bfree = kbuf->f_bfree;
- buf.f_bavail = kbuf->f_bavail;
- buf.f_files = kbuf->f_files;
- buf.f_ffree = kbuf->f_ffree;
- buf.f_namelen = kbuf->f_namelen;
- buf.f_fsid.val[0] = kbuf->f_fsid.val[0];
- buf.f_fsid.val[1] = kbuf->f_fsid.val[1];
- buf.f_frsize = kbuf->f_frsize;
- buf.f_flags = kbuf->f_flags;
+
+ /* f_type can be signed 32-bits on some architectures, but it contains
+ * magic value, so we do not care if value overflows destination
+ * structure field if bits are intact.
+ */
+ ASSIGN_CHECK_TRUNCATED_BITS(buf.f_type, kbuf->f_type, offlag);
+ if (WARN_ON(offlag))
+ return -EOVERFLOW;
+ ASSIGN_CHECK_OVERFLOW(buf.f_namelen, kbuf->f_namelen, offlag);
+ if (WARN_ON(offlag))
+ return -EOVERFLOW;
+
+ ASSIGN_CHECK_OVERFLOW(buf.f_bsize, kbuf->f_bsize, offlag);
+ ASSIGN_CHECK_OVERFLOW(buf.f_blocks, kbuf->f_blocks, offlag);
+ ASSIGN_CHECK_OVERFLOW(buf.f_bfree, kbuf->f_bfree, offlag);
+ ASSIGN_CHECK_OVERFLOW(buf.f_bavail, kbuf->f_bavail, offlag);
+ /* f_files and f_ffree may be -1; it's okay to put that into 32 bits */
+ ASSIGN_CHECK_OVERFLOW_EXCEPT(buf.f_files, kbuf->f_files, offlag,
+ 0xffffffffffffffffULL);
+ ASSIGN_CHECK_OVERFLOW_EXCEPT(buf.f_ffree, kbuf->f_ffree, offlag,
+ 0xffffffffffffffffULL);
+ ASSIGN_CHECK_SAME_TYPE(buf.f_fsid.val[0], kbuf->f_fsid.val[0]);
+ ASSIGN_CHECK_SAME_TYPE(buf.f_fsid.val[1], kbuf->f_fsid.val[1]);
+ ASSIGN_CHECK_OVERFLOW(buf.f_frsize, kbuf->f_frsize, offlag);
+ ASSIGN_CHECK_TRUNCATED_BITS(buf.f_flags, kbuf->f_flags, offlag);
+
+ if (unlikely(offlag))
+ return -EOVERFLOW;
if (copy_to_user(ubuf, &buf, sizeof(struct compat_statfs)))
return -EFAULT;
return 0;
@@ -303,32 +336,34 @@ COMPAT_SYSCALL_DEFINE2(fstatfs, unsigned int, fd, struct compat_statfs __user *,
static int put_compat_statfs64(struct compat_statfs64 __user *ubuf, struct kstatfs *kbuf)
{
struct compat_statfs64 buf;
- if (sizeof(ubuf->f_bsize) == 4) {
- if ((kbuf->f_type | kbuf->f_bsize | kbuf->f_namelen |
- kbuf->f_frsize | kbuf->f_flags) & 0xffffffff00000000ULL)
- return -EOVERFLOW;
- /* f_files and f_ffree may be -1; it's okay
- * to stuff that into 32 bits */
- if (kbuf->f_files != 0xffffffffffffffffULL
- && (kbuf->f_files & 0xffffffff00000000ULL))
- return -EOVERFLOW;
- if (kbuf->f_ffree != 0xffffffffffffffffULL
- && (kbuf->f_ffree & 0xffffffff00000000ULL))
- return -EOVERFLOW;
- }
+ bool offlag = false;
+
memset(&buf, 0, sizeof(struct compat_statfs64));
- buf.f_type = kbuf->f_type;
- buf.f_bsize = kbuf->f_bsize;
- buf.f_blocks = kbuf->f_blocks;
- buf.f_bfree = kbuf->f_bfree;
- buf.f_bavail = kbuf->f_bavail;
- buf.f_files = kbuf->f_files;
- buf.f_ffree = kbuf->f_ffree;
- buf.f_namelen = kbuf->f_namelen;
- buf.f_fsid.val[0] = kbuf->f_fsid.val[0];
- buf.f_fsid.val[1] = kbuf->f_fsid.val[1];
- buf.f_frsize = kbuf->f_frsize;
- buf.f_flags = kbuf->f_flags;
+
+ /* f_type can be signed 32-bits on some architectures, but it contains
+ * magic value, so we do not care if value overflows destination
+ * structure field if bits are intact.
+ */
+ ASSIGN_CHECK_TRUNCATED_BITS(buf.f_type, kbuf->f_type, offlag);
+ if (WARN_ON(offlag))
+ return -EOVERFLOW;
+ ASSIGN_CHECK_OVERFLOW(buf.f_namelen, kbuf->f_namelen, offlag);
+ if (WARN_ON(offlag))
+ return -EOVERFLOW;
+
+ ASSIGN_CHECK_OVERFLOW(buf.f_bsize, kbuf->f_bsize, offlag);
+ ASSIGN_CHECK_SAME_TYPE(buf.f_blocks, kbuf->f_blocks);
+ ASSIGN_CHECK_SAME_TYPE(buf.f_bfree, kbuf->f_bfree);
+ ASSIGN_CHECK_SAME_TYPE(buf.f_bavail, kbuf->f_bavail);
+ ASSIGN_CHECK_SAME_TYPE(buf.f_files, kbuf->f_files);
+ ASSIGN_CHECK_SAME_TYPE(buf.f_ffree, kbuf->f_ffree);
+ ASSIGN_CHECK_OVERFLOW(buf.f_fsid.val[0], kbuf->f_fsid.val[0], offlag);
+ ASSIGN_CHECK_OVERFLOW(buf.f_fsid.val[1], kbuf->f_fsid.val[1], offlag);
+ ASSIGN_CHECK_OVERFLOW(buf.f_frsize, kbuf->f_frsize, offlag);
+ ASSIGN_CHECK_TRUNCATED_BITS(buf.f_flags, kbuf->f_flags, offlag);
+
+ if (unlikely(offlag))
+ return -EOVERFLOW;
if (copy_to_user(ubuf, &buf, sizeof(struct compat_statfs64)))
return -EFAULT;
return 0;
diff --git a/include/linux/build_bug.h b/include/linux/build_bug.h
index b7d22d6..1d2f563 100644
--- a/include/linux/build_bug.h
+++ b/include/linux/build_bug.h
@@ -79,6 +79,16 @@
*/
#define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
+/**
+ * ASSIGN_CHECK_SAME_TYPE() -- assigns src value to dest but only if they have
+ * same type (will break build otherwise).
+ */
+#define ASSIGN_CHECK_SAME_TYPE(dest, src) \
+ do { \
+ BUILD_BUG_ON(!__same_type((dest), (src))); \
+ (dest) = (src); \
+ } while (0)
+
#endif /* __CHECKER__ */
#endif /* _LINUX_BUILD_BUG_H */
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index 16d41de..71b4c29 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -305,6 +305,11 @@
#define __no_sanitize_address __attribute__((no_sanitize_address))
#endif
+#if GCC_VERSION >= 50000
+#define __ASSIGN_CHECK_OVERFLOW(dest, src) \
+ __builtin_add_overflow(0, (src), &dest)
+#endif
+
#if GCC_VERSION >= 50100
/*
* Mark structures as requiring designated initializers.
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index f260ff3..6822b17 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -605,4 +605,47 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
(volatile typeof(x) *)&(x); })
#define ACCESS_ONCE(x) (*__ACCESS_ONCE(x))
+/**
+ * This is a helper macros for assignment of 32-bit fields in some syscall
+ * structures where kernel works with larger, 64-bit values and could induce
+ * overflow in 32-bit caller. If flag is set, it is expected that syscall
+ * will return -EOVERFLOW.
+ *
+ * Sizes of the fields are not compared here as it allows to validate for
+ * signedness difference in field types. Compiler will likely to eliminate
+ * overflow check if types of the fields are exactly matching.
+ *
+ * ASSIGN_CHECK_OVERFLOW_EXCEPT() is similar but allows to truncate a special
+ * magic constant (such as -1 represented as unsigned).
+ *
+ * @dest: name of the destination field
+ * @src: name of the source field
+ * @offlag: name of bool variable used to store overflow flag
+ * @value: magic value which is OK to be truncated
+ */
+#ifndef __ASSIGN_CHECK_OVERFLOW
+#define __ASSIGN_CHECK_OVERFLOW(dest, src) \
+ ({ \
+ typeof(src) ____src = (src); \
+ bool __offlag = false; \
+ (dest) = ____src; \
+ do { \
+ typeof(src) __val = dest; \
+ __offlag = (__val != ____src); \
+ } while (0); \
+ __offlag; \
+ })
+#endif
+#define ASSIGN_CHECK_OVERFLOW(dest, src, offlag) \
+ do { \
+ if (__ASSIGN_CHECK_OVERFLOW(dest, src)) \
+ offlag = true; \
+ } while (0)
+#define ASSIGN_CHECK_OVERFLOW_EXCEPT(dest, src, offlag, value) \
+ do { \
+ typeof(src) __src = (src); \
+ if (__ASSIGN_CHECK_OVERFLOW(dest, __src) && __src != (value)) \
+ offlag = true; \
+ } while (0)
+
#endif /* __LINUX_COMPILER_H */
--
2.10.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] vfs: fix statfs64() returning impossible EOVERFLOW for 64-bit f_files
2017-10-06 1:31 ` Linus Torvalds
2017-10-18 16:04 ` [PATCH v2] vfs: Improve overflow checking for stat*() compat fields Sergey Klyaus
@ 2018-08-06 17:06 ` Al Viro
2018-08-06 18:45 ` Linus Torvalds
1 sibling, 1 reply; 9+ messages in thread
From: Al Viro @ 2018-08-06 17:06 UTC (permalink / raw)
To: Linus Torvalds
Cc: Sergey Klyaus, linux-fsdevel, Linux Kernel Mailing List, Li Wang,
Andreas Dilger, Andi Kleen
On Thu, Oct 05, 2017 at 06:31:29PM -0700, Linus Torvalds wrote:
> On Thu, Oct 5, 2017 at 4:06 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > Just to make sure we are on the same page: out of kstatfs fields
> > we have 5 u64 ones (see above; all of them are u64 is struct statfs64
> > on all architectures), an opaque 64bit f_fsid and 5 fields that
> > are long: f_type (magic numbers, all 32bit), f_namelen (max filename
> > length), f_frsize (0 on most of filesystems, always fits into 32 bits),
> > f_flags (guaranteed to be 32bit) and f_bsize.
>
> Please just use that FITS_IN() kind of macro regardless.
>
> If the sizes match, the compiler will optimize the test away.
>
> If the sizes don't match, that FITS_IN() will do the right thing.
>
> Do *not* manually go and say "these fields are ok, because..". The
> whole bug was because people were confused about the field widths.
To bring that thread back, with apologies for having it fall through
the cracks back last autumn:
compat_statfs64 "overflow checks" are completely bogus. Some relevant
information:
* all struct compat_statfs64 instances (all 3 of them) have
identical field sizes. So any ifdefs on sizeof of anything in them
are nonsense to start with.
* most of the fields have the same size as their struct kstatfs
counterparts, with the following exceptions - f_type, f_bsize, f_frsize,
f_namelen and f_flags are u32 in compat_statfs64 and long in kstatfs.
Anything other than those fields should not be getting any overflow
checks for obvious reasons - in particular this
/* f_files and f_ffree may be -1; it's okay
* to stuff that into 32 bits */
if (kbuf->f_files != 0xffffffffffffffffULL
&& (kbuf->f_files & 0xffffffff00000000ULL))
return -EOVERFLOW;
if (kbuf->f_ffree != 0xffffffffffffffffULL
&& (kbuf->f_ffree & 0xffffffff00000000ULL))
return -EOVERFLOW;
is garbage, blindly copied from handling of compat_statfs where f_files is
32bit.
* anyone who seriously suggests that some fs might want to report
f_namelen greater than 4Gb is welcome to bludgeon themselves with GNU Hurd
manuals, repeating the mantras about avoiding arbitrary limits until they
get enlightened, go join Hurd development or succeed in bashing out
whatever they had between their ears. Either way, they won't be pestering
us again.
* f_type is an opaque magic value; if it doesn't fit into 32 bits,
such filesystem can't be used on 32bit host. Bug.
* f_flags is generated right there in fs/statfs.c and the value is
currently limited to 13 bits.
That leaves us with f_bsize and f_frsize (the latter defaulting to the former).
Hugetlbfs can put greater than 4Gb values in there, for really huge pages.
And that's the only thing worth checking in there.
So the whole put_compat_statfs64() thing should be
static int put_compat_statfs64(struct compat_statfs64 __user *ubuf, struct kstatfs *kbuf)
{
struct compat_statfs64 buf;
if ((kbuf->f_bsize | kbuf->f_frsize) & 0xffffffff00000000ULL)
return -EOVERFLOW;
memset(&buf, 0, sizeof(struct compat_statfs64));
buf.f_type = kbuf->f_type;
buf.f_bsize = kbuf->f_bsize;
buf.f_blocks = kbuf->f_blocks;
buf.f_bfree = kbuf->f_bfree;
buf.f_bavail = kbuf->f_bavail;
buf.f_files = kbuf->f_files;
buf.f_ffree = kbuf->f_ffree;
buf.f_namelen = kbuf->f_namelen;
buf.f_fsid.val[0] = kbuf->f_fsid.val[0];
buf.f_fsid.val[1] = kbuf->f_fsid.val[1];
buf.f_frsize = kbuf->f_frsize;
buf.f_flags = kbuf->f_flags;
if (copy_to_user(ubuf, &buf, sizeof(struct compat_statfs64)))
return -EFAULT;
return 0;
}
and that's it for compat_statfs64 bug reported by Sergey.
I'm somewhat tempted to get rid of those 'long' in struct kstatfs,
turning f_bsize and f_frsize into u64, with f_type, f_namelen and f_flags
becoming u32 and f_spare going to hell (not a single filesystem stores
anything in f_spare and e.g. mips zeroes those on the way to userland in
*native* statfs(2) anyway). We'd lose the "fast" case of do_statfs64()
and do_statfs_native(), but frankly, it's not a great loss. Doing e.g.
struct kstatfs {
u32 f_type;
u32 f_namelen; // moved up here
u64 f_bsize;
u64 f_blocks;
u64 f_bfree;
u64 f_bavail;
u64 f_files;
u64 f_ffree;
__kernel_fsid_t f_fsid;
u64 f_frsize;
u32 f_flags;
};
with
#define STATFS_COPYOUT(type) \
static int put##type(struct kstatfs *st, struct type __user *p) \
{ \
struct type buf; \
\
memset(&buf, 0, sizeof(buf)); \
buf.f_type = st->f_type; \
if (!FIT_IN(st->f_bsize, buf.f_bsize)) \
return -EOVERFLOW; \
buf.f_bsize = st->f_bsize; \
if (!FIT_IN(st->f_blocks, buf.f_blocks)) \
return -EOVERFLOW; \
buf.f_blocks = st->f_blocks; \
if (!FIT_IN(st->f_bfree, buf.f_bfree)) \
return -EOVERFLOW; \
buf.f_bfree = st->f_bfree; \
if (!FIT_IN(st->f_bavail, buf.f_bavail)) \
return -EOVERFLOW; \
buf.f_bavail = st->f_bavail; \
if (!FIT_IN(st->f_files, buf.f_files) && st->f_files != -1) \
return -EOVERFLOW; \
buf.f_files = st->f_files; \
if (!FIT_IN(st->f_ffree, buf.f_ffree) && st->f_ffree != -1) \
return -EOVERFLOW; \
buf.f_ffree = st->f_ffree; \
buf.f_fsid = st->f_fsid; \
buf.f_namelen = st->f_namelen; \
if (!FIT_IN(st->f_frsize, buf.f_frsize)) \
return -EOVERFLOW; \
buf.f_frsize = st->f_frsize; \
buf.f_flags = st->f_flags; \
if (copy_to_user(p, &buf, sizeof(buf))) \
return -EFAULT; \
return 0; \
}
STATFS_COPYOUT(statfs)
STATFS_COPYOUT(compat_statfs)
STATFS_COPYOUT(statfs64)
STATFS_COPYOUT(compat_statfs64)
providing all the copyout helpers should be reasonably sane, IMO.
As for the convoluted macros Sergey has proposed... Not without a very
good evidence that they win enough performance to be worth the complexity.
Comments? Again, my apologies for losing that thread back then - I've just
found it while doing (unrelated) grep through the old mailboxen...
^ permalink raw reply [flat|nested] 9+ messages in thread