[PATCH] vfs: fix statfs64() returning impossible EOVERFLOW for 64-bit f_files

[PATCH] vfs: fix statfs64() returning impossible EOVERFLOW for 64-bit f_files

[Sergey Klyaus]

compat_statfs64 structure has some 32-bit and some 64-bit fields, so
64d2ab32e "vfs: fix put_compat_statfs64() does not handle errors" fixed
32-bit overflow checks not being performed, but accidentally enabled
checks for f_files and f_ffree that are 64-bit and cannot have overflow.
Now checks for both groups of fields are enabled by different
conditions.

This broke my Steam runtime and can be reproduced with this test case:

    # mount -t tmpfs -o nr_inodes=4294967297 tmpfs /mnt
    $ cat statfs.c

int main() {
        struct statvfs sv;
        statvfs("/mnt", &sv);
        printf("%d %llu %llu\n", errno,
                (unsigned long long) sv.f_files,
                (unsigned long long) sv.f_ffree);
        return 0;
}
    $ gcc -g -m32 -D_FILE_OFFSET_BITS=64 statfs.c
    $ ./a.out
    75 134513445 0
    |  \- some junk on stack
    EOVERFLOW

Signed-off-by: Sergey Klyaus <sergey.m.klyaus@gmail.com>
---
 fs/statfs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/statfs.c b/fs/statfs.c
index fab9b6a3c116..073bb2d1871e 100644
--- a/fs/statfs.c
+++ b/fs/statfs.c
@@ -307,6 +307,8 @@ static int put_compat_statfs64(struct compat_statfs64 __user *ubuf, struct kstat
 		if ((kbuf->f_type | kbuf->f_bsize | kbuf->f_namelen |
 		     kbuf->f_frsize | kbuf->f_flags) & 0xffffffff00000000ULL)
 			return -EOVERFLOW;
+	}
+	if (sizeof(ubuf->f_blocks) == 4) {
 		/* f_files and f_ffree may be -1; it's okay
 		 * to stuff that into 32 bits */
 		if (kbuf->f_files != 0xffffffffffffffffULL
-- 
2.14.1

Re: [PATCH] vfs: fix statfs64() returning impossible EOVERFLOW for 64-bit f_files

[Al Viro]

On Thu, Oct 05, 2017 at 09:36:36PM +0300, Sergey Klyaus wrote:
> compat_statfs64 structure has some 32-bit and some 64-bit fields, so
> 64d2ab32e "vfs: fix put_compat_statfs64() does not handle errors" fixed
> 32-bit overflow checks not being performed, but accidentally enabled
> checks for f_files and f_ffree that are 64-bit and cannot have overflow.
> Now checks for both groups of fields are enabled by different
> conditions.

TBH, the logics in there looks very dubious.  First of all, could somebody
show an architecture where compat_statfs64 would *not* have 32bit f_bsize?

AFAICS, there are only 3 variants of struct compat_statfs64 declaration in
the entire tree:
arch/mips/include/uapi/asm/statfs.h:82:struct compat_statfs64 {
arch/s390/include/asm/compat.h:167:struct compat_statfs64 {
include/uapi/asm-generic/statfs.h:68:struct compat_statfs64 {

mips one has
        __u32   f_bsize;
s390 -
        u32             f_bsize;
and generic -
        __u32 f_bsize;

So what is that if (sizeof... == 4) about?  Before or after the commit in
question - f_blocks is consistently 64bit, f_bsize - 32bit.  IOW, that
commit has turned an obfuscated if (0) into equally obfuscated if (1).

In any case, that thing is supposed to behave like statfs64(2) on matching
32bit host, so what the hell is that EOVERFLOW about, anyway?  ->f_type value
not fitting into 32 bits?  That'd be an fs bug; I could see WARN_ON() on that,
but -EOVERFLOW is bloody odd.  And ->f_namelen exceeding 4Gb is even funnier...

Seriously, the old logics had been complete BS and the only saving grace had
been the fact that it never triggered.  What the hell is f_files and f_ffree
logics about?  Those are 64bit in *all* struct statfs64 variants.  Always
had been.

AFAICS, the real bug here is in hugetlbfs; that's where obscene values in
->f_bsize come from.  IMO all that code in put_compat_statfs64() should be
replaced with
	if (kbuf->bsize != (u32)kbuf->bsize)
		return -EOVERFLOW;
That, or hugetlbfs could be taught to fake saner ->f_bsize (recalculating
->f_bavail/->f_bfree/->f_blocks to go with that).

Comments?

Re: [PATCH] vfs: fix statfs64() returning impossible EOVERFLOW for 64-bit f_files

[Linus Torvalds]

On Thu, Oct 5, 2017 at 1:57 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> AFAICS, the real bug here is in hugetlbfs; that's where obscene values in
> ->f_bsize come from.  IMO all that code in put_compat_statfs64() should be
> replaced with
>         if (kbuf->bsize != (u32)kbuf->bsize)
>                 return -EOVERFLOW;
> That, or hugetlbfs could be taught to fake saner ->f_bsize (recalculating
> ->f_bavail/->f_bfree/->f_blocks to go with that).

Make it so. Except you shouldn't do

     if (kbuf->bsize != (u32)kbuf->bsize)

you should do something like

    #define FITS_IN(x,y)  ({ typeof x __x = (x); typeof y __y = __x;
__x == __y; })

and then do

    if (!FITS_IN(kbuf->bsize, ubuf->bsize)) ...

because there is nothing that specifies that the ubuf size of all
fields has to be 32 bits.

But yes,m either you need to then special-case that crazy all-ones
value, or just fix hugetlbfs to not use crazy crap.

                  Linus

Re: [PATCH] vfs: fix statfs64() returning impossible EOVERFLOW for 64-bit f_files

[Al Viro]

On Thu, Oct 05, 2017 at 03:31:05PM -0700, Linus Torvalds wrote:
> On Thu, Oct 5, 2017 at 1:57 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > AFAICS, the real bug here is in hugetlbfs; that's where obscene values in
> > ->f_bsize come from.  IMO all that code in put_compat_statfs64() should be
> > replaced with
> >         if (kbuf->bsize != (u32)kbuf->bsize)
> >                 return -EOVERFLOW;
> > That, or hugetlbfs could be taught to fake saner ->f_bsize (recalculating
> > ->f_bavail/->f_bfree/->f_blocks to go with that).
> 
> Make it so. Except you shouldn't do
> 
>      if (kbuf->bsize != (u32)kbuf->bsize)
> 
> you should do something like
> 
>     #define FITS_IN(x,y)  ({ typeof x __x = (x); typeof y __y = __x;
> __x == __y; })
> 
> and then do
> 
>     if (!FITS_IN(kbuf->bsize, ubuf->bsize)) ...
> 
> because there is nothing that specifies that the ubuf size of all
> fields has to be 32 bits.
> 
> But yes,m either you need to then special-case that crazy all-ones
> value, or just fix hugetlbfs to not use crazy crap.

All-ones is not a problem at all - those two fields are consistently
64bit in struct statfs64 on all 32bit architectures.  That had pretty
much been the rationale for statfs64(2) in the first place - statfs(2)
couldn't be used on large filesystems; 4Gfiles and you get an overflow
on 32bit.  So the entire "let's check if f_files/f_ffree/f_bavail/f_bfree/
f_blocks fit into 32 bits" had been an utter nonsense from the very
beginning and the only reason it hadn't been spotted earlier was that
this logics was under if (sizeof(u64) == 4) until the last November.

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.

f_bsize is a mess - normal practice for Unices is to have f_blocks in
units of f_frsize, leaving f_bsize as preferred IO granularity.  Linux
didn't have f_frsize until 2003 or so, and f_bsize got used for units
of f_blocks.

hugetlbfs uses it to report the huge page size; the real problem
last year commit tried to deal with was that on boxen with huge pages
4Gb or bigger we get 0 observed in that field by 32bit processes
calling statfs64(2).  I'm not sure whether we treat that use of
f_bsize by hugetlbfs as an accidental ABI (in that case we need to
check that it fits into u32 and fail with EOVERFLOW otherwise;
again, all compat_statfs64 have f_bsize 32bit) or just cap it with
something sane (2Gb?) and adjast f_blocks/f_bavail/f_bfree accordingly.

Fields that are u64 in kstatfs don't need any checks - they are
64bit in compat_statfs64 as well.  Other four 32bit fields... sure,
we could check them, but for those the reasonable reaction is not
EOVERFLOW - it's WARN_ON().

Re: [PATCH] vfs: fix statfs64() returning impossible EOVERFLOW for 64-bit f_files

[Linus Torvalds]

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.

                 Linus

[PATCH v2] vfs: Improve overflow checking for stat*() compat fields

[Sergey Klyaus]

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

Re: [PATCH] vfs: fix statfs64() returning impossible EOVERFLOW for 64-bit f_files

[Al Viro]

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...

Re: [PATCH] vfs: fix statfs64() returning impossible EOVERFLOW for 64-bit f_files

[Linus Torvalds]

On Mon, Aug 6, 2018 at 10:06 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> 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

Ack, I'm ok with this simplification.

> I'm somewhat tempted to get rid of those 'long' in struct kstatfs,

I'm ok with this one too.

> with
>
> #define STATFS_COPYOUT(type)                                            \
> static int put##type(struct kstatfs *st, struct type __user *p)         \

No. Don't do this.

I'm ok with the #define to avoid duplication, but don't bother with
the FIT_IN() after you've above successfully argued that it's
pointless for anything but f_bsize/frsize.

So if you do the macro to generate the different copyout versions,
just use your simplified code for that macro instead. With FIT_IN()
just for f_bsize/frsize.

                    Linus

Re: [PATCH] vfs: fix statfs64() returning impossible EOVERFLOW for 64-bit f_files

[Al Viro]

On Mon, Aug 06, 2018 at 11:45:29AM -0700, Linus Torvalds wrote:
> On Mon, Aug 6, 2018 at 10:06 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > 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
> 
> Ack, I'm ok with this simplification.
> 
> > I'm somewhat tempted to get rid of those 'long' in struct kstatfs,
> 
> I'm ok with this one too.
> 
> > with
> >
> > #define STATFS_COPYOUT(type)                                            \
> > static int put##type(struct kstatfs *st, struct type __user *p)         \
> 
> No. Don't do this.
> 
> I'm ok with the #define to avoid duplication, but don't bother with
> the FIT_IN() after you've above successfully argued that it's
> pointless for anything but f_bsize/frsize.
> 
> So if you do the macro to generate the different copyout versions,
> just use your simplified code for that macro instead. With FIT_IN()
> just for f_bsize/frsize.

For statfs64 (both native and compat) that would do nicely; for statfs,
however...  The following describes the field sizes in all that mess:

        kstatfs	statfs          statfs64        compat_statfs compat_statfs64
       		!s390   s390   !s390   s390
type:   W	W       32      W       32              32      32
namelen:W	W       32      W       32              32      32
flags:  W	W       32      W       32              32      32
bsize:  W	W       32      W       32              32      32
frsize: W	W       32      W       32              32      32
blocks: 64	W       64      64      64              32      64
bfree:  64	W       64      64      64              32      64
bavail: 64	W       64      64      64              32      64
files:  64	W       64      64      64              32      64
ffree:  64	W       64      64      64              32      64
fsid:   __kernel_fsid_t on all

First of all, nobody gives a fuck about type, namelen and flags
overflows - can't happen.

blocks/bfree/bavail/files/ffree: can overflow in plain statfs on 32bit
and in compat statfs.  For files/ffree we also have "-1 means (s32)-1,
not an overflow" there.

bsize/frsize: can oveflow in anything on s390 or mips64 and any compat on anything

Oh, and then there's signedness - in compat_statfs64 everything's unsigned,
but for compat_statfs a bunch of those 32bit ones are signed.  Native on
32bit tend to be unsigned; native on 64bit and compat are often signed.
IMO that's a bug - compat ones should all be same as native 32bit.
As it is,
	arm, parisc, ppc, sparc, x86: on 32bit - unsigned, compat on 64bit - signed
	mips: both signed
	s390: both unsigned
I think we want to switch compat_statfs fields on the first group to u32.  These
declarations are not exposed to userland anyway.  mips is interesting - I've no
idea how does mips32 userland react to e.g. statfs() on 3G block filesystem...