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

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] [day] [month] [year] [list]

Message-ID: <20180806210336.GN15082@ZenIV.linux.org.uk>
Date:   Mon, 6 Aug 2018 22:03:36 +0100
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Sergey Klyaus <sergey.m.klyaus@...il.com>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Li Wang <liwang@...hat.com>,
        Andreas Dilger <adilger@...ger.ca>,
        Andi Kleen <andi.kleen@...el.com>
Subject: Re: [PATCH] vfs: fix statfs64() returning impossible EOVERFLOW for
 64-bit f_files

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