linux-kernel - Re: [PATCH] vfs: fix statfs64() does not handle errors

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 7 Nov 2016 11:03:11 -0700
From:   Andreas Dilger <adilger@...ger.ca>
To:     Li Wang <liwang@...hat.com>
Cc:     viro@...iv.linux.org.uk, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] vfs: fix statfs64() does not handle errors

On Nov 7, 2016, at 3:21 AM, Li Wang <liwang@...hat.com> wrote:
> 
> statfs64() does NOT return -1 and setting errno to EOVERFLOW when some
> variables(like: f_bsize) overflowed in the returned struct.
> 
> reproducer:
> step1. mount hugetlbfs with two different pagesize on ppc64 arch.
> 
> $ hugeadm --pool-pages-max 16M:0
> $ hugeadm --create-mount
> $ mount | grep -i hugetlbfs
> none on /var/lib/hugetlbfs/pagesize-16MB type hugetlbfs (rw,relatime,seclabel,pagesize=16777216)
> none on /var/lib/hugetlbfs/pagesize-16GB type hugetlbfs (rw,relatime,seclabel,pagesize=17179869184)
> 
> step2. compile & run this C program.
> 
> $ cat statfs64_test.c
> 
> #define _LARGEFILE64_SOURCE
> #include <stdio.h>
> #include <sys/statfs.h>
> 
> int main()
> {
> 	struct statfs64 sb;
> 	int err;
> 
> 	err = statfs64("/var/lib/hugetlbfs/pagesize-16GB", &sb);
> 	if (err)
> 		return -1;
> 
> 	printf("sizeof f_bsize = %d, f_bsize=%ld\n", sizeof(sb.f_bsize), sb.f_bsize);
> 
> 	return 0;
> }
> 
> $ gcc -m32 statfs64_test.c
> $ ./a.out
> sizeof f_bsize = 4, f_bsize=0
> 
> Signed-off-by: Li Wang <liwang@...hat.com>
> ---
> 
> Notes:
>    This is my first patch to kernel fs part, I'm not sure if
>    this one useful, but just want someone have a look.
> 
>    thanks~
> 
> fs/statfs.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
> 
> diff --git a/fs/statfs.c b/fs/statfs.c
> index 083dc0a..849dde95 100644
> --- a/fs/statfs.c
> +++ b/fs/statfs.c
> @@ -151,6 +151,23 @@ static int do_statfs64(struct kstatfs *st, struct statfs64 __user *p)
> 	if (sizeof(buf) == sizeof(*st))
> 		memcpy(&buf, st, sizeof(*st));
> 	else {
> +		if (sizeof buf.f_bsize == 4) {

Linux CodingStyle says this should be used like sizeof(buf.f_bsize).

> +			if ((st->f_blocks | st->f_bfree | st->f_bavail |
> +			     st->f_bsize | st->f_frsize) &
> +			    0xffffffff00000000ULL)
> +				return -EOVERFLOW;

I'm not sure I agree with this check.  Sure, if sizeof(buf.f_bsize) == 4
then the large st->f_bsize will overflow this field, and that is valid.

However, that doesn't mean that large values for f_blocks, f_bfree, f_bavail
should return an error.  I assume you are concerned that the product of the
large f_bsize and one of those values would overflow a 64-bit bytes value,
but that is for userspace to worry about, since the values in the individual
fields themselves are OK.

We're already over 100PiB Lustre filesystems (2^57 bytes) today, and I
don't want statfs() failing prematurely because userspace feels the need
to multiply out the blocks and blocksize into bytes, instead of shifting
the values to KB (which would allow filesystems up to 2^74-1024 bytes to
be handled correctly in userspace).

> +			/*
> +			 * 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;

Why does sizeof(f_bsize) have anything to do with the number of free files?
These two checks are just plain wrong, since f_files and f_ffree are 64-bit
fields in struct statfs64.

Cheers, Andreas

> +		}
> +
> 		buf.f_type = st->f_type;
> 		buf.f_bsize = st->f_bsize;
> 		buf.f_blocks = st->f_blocks;
> --
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)