Message-ID: <20131007232358.GG6860@birch.djwong.org>
Date:	Mon, 7 Oct 2013 16:23:58 -0700
From:	"Darrick J. Wong" <darrick.wong@...cle.com>
To:	"Theodore Ts'o" <tytso@....edu>
Cc:	linux-ext4@...r.kernel.org
Subject: Re: [PATCH 06/31] e2p: Fix f[gs]etflags argument size mismatch

Well, it's nastier than this.  For an ioctl that only writes to userspace,
libfuse allocates an internal buffer with whatever junk is in memory at the
time.  If the fuse server's ioctl handler doesn't zero the buffer before
returning, the remnants of this garbage are passed from the server back to the
kernel, which dutifully copies the garbage into the user program's memory.
Even if the result is some error code!

fuse2fs' getflags handler would write an int to the lower end of this buffer,
so garbage + flags would get sent back to userspace.  In my particular case,
gcc was applying some kind of optimization to the chattr binary that places the
flags variable next to the 'name' variable in change_attributes(), and so the
return from the ioctl (through fs/fuse/file.c) would write a 64-bit value over
flags and the lower 32 bits of name.  The next time anyone tried to use name,
it would get a garbage pointer and (probably) crash.

Yuck.  FUSE assumes an interface contract (the data size encoded in the ioctl
number) that neither userspace nor kernel actually abide.  This has gone on for
years with no problems, since both components made the same implicit assumption
about data size in the same way.  Unfortunately, userspace breaks only on FUSE,
so I don't know what to do.

It's possible to adapt both e2p and FUSE servers to allocate an unsigned long
and shift the values around accordingly, but that won't help avoid a crash with
existing e2p binaries.  Even if I comment out the .ioctl function pointer in
fuse2fs.c, running chattr will still crash--FUSE copies the buffer even for an
error result.

Long term I guess we could define a new pair of ioctls that work with pointers
to 64-bit values and deprecate the old ones.  Or perhaps there's a better
suggestion than "don't run chattr/lsattr on a FUSE"?

--D

On Mon, Oct 07, 2013 at 01:40:55PM -0700, Darrick J. Wong wrote:
> On Mon, Oct 07, 2013 at 09:33:04AM -0400, Theodore Ts'o wrote:
> > On Mon, Sep 30, 2013 at 06:27:21PM -0700, Darrick J. Wong wrote:
> > 
> > > The EXT2_IOC_[GS]ETFLAGS ioctls take longs as arguments, however
> > > this code only reserves enough storage for an int.  The kernel
> > > drivers (so far) don't transfer more than an int but FUSE sees the
> > > long and assumes that it's ok to write the full size of the long,
> > > which crashes if sizeof(long) > sizeof(int).
> > 
> > All of the kernel code I was able to audit is treating the
> > EXT2_IOC_[SG]ETFLAGS ioctls as taking an int, not a long.  So the
> > defacto definition of [SG]ETFLAGS is that that they take ints, not
> > longs.  If we make this change which you are proposing, it will cause
> > problems on big-endian systems where sizeof(long) > sizeof(int) ---
> > for example, as would be the case on the ppc64 architecture.
> > 
> > I'm not sure what the FUSE problem is?  Can you say more?  Is there
> > some other way we can work around the problem you are trying to solve?
> 
> If I mount a FUSE fs (specifically the fuse2fs thing in the last patch) and try
> to run 'chattr +i /mnt/foo', chattr segfaults.  valgrind had this[1] to offer.
> It seems that FUSE's ioctl implementation depends on the 'size' argument to
> _IOR (in the EXT2_IOC_[SG]ETFLAGS definition) to figure out how much data to
> transfer in or out of userspace.  Unfortunately for fgetflags, it passes in a
> pointer to an int, and fuse seems to smash up the stack trying to write back a
> long.  Valgrind and gdb show that the lower 32-bits of name get overwritten,
> which causes the segfault.
> 
> Unfortunately, this is a bit of a heisenbug; if I build with -O0 (gcc 4.6.3,
> Ubuntu 12.04, libfuse 2.9.2 backport) it goes away.  The stack corruption also
> seems to go away if I print the address of f, though save_errno gets
> overwritten with some suspicious looking 32695 value.
> 
> I'll keep poking at what's going on here, though I'll try to come up with a
> clever solution for BE machines.
> 
> --D
> 
> [1] Valgrind messsage:
> ==3512== Memcheck, a memory error detector
> ==3512== Copyright (C) 2002-2011, and GNU GPL'd, by Julian Seward et al.
> ==3512== Using Valgrind-3.7.0 and LibVEX; rerun with -h for copyright info
> ==3512== Command: chattr +i /mnt/foo
> ==3512== 
> ==3512== Syscall param lstat(file_name) points to unaddressable byte(s)
> ==3512==    at 0x53232B5: _lxstat (lxstat.c:38)
> ==3512==    by 0x4E34610: fsetflags (in /lib/x86_64-linux-gnu/libe2p.so.2.3)
> ==3512==    by 0x401350: ??? (in /usr/bin/chattr)
> ==3512==    by 0x4010A0: ??? (in /usr/bin/chattr)
> ==3512==    by 0x525E76C: (below main) (libc-start.c:226)
> ==3512==  Address 0x700007fb7 is not stack'd, malloc'd or (recently) free'd
> ==3512== 
> ==3512== Syscall param open(filename) points to unaddressable byte(s)
> ==3512==    at 0x53236C0: __open_nocancel (syscall-template.S:82)
> ==3512==    by 0x4E3463E: fsetflags (in /lib/x86_64-linux-gnu/libe2p.so.2.3)
> ==3512==    by 0x401350: ??? (in /usr/bin/chattr)
> ==3512==    by 0x4010A0: ??? (in /usr/bin/chattr)
> ==3512==    by 0x525E76C: (below main) (libc-start.c:226)
> ==3512==  Address 0x700007fb7 is not stack'd, malloc'd or (recently) free'd
> ==3512== 
> chattr: Bad address ==3512== Invalid read of size 1
> ==3512==    at 0x52883B1: vfprintf (vfprintf.c:1630)
> ==3512==    by 0x528B1A3: buffered_vfprintf (vfprintf.c:2313)
> ==3512==    by 0x5285BDD: vfprintf (vfprintf.c:1316)
> ==3512==    by 0x53463D7: __vfprintf_chk (vfprintf_chk.c:35)
> ==3512==    by 0x503ABA8: ??? (in /lib/x86_64-linux-gnu/libcom_err.so.2.1)
> ==3512==    by 0x503AD1E: com_err (in /lib/x86_64-linux-gnu/libcom_err.so.2.1)
> ==3512==    by 0x401435: ??? (in /usr/bin/chattr)
> ==3512==    by 0x4010A0: ??? (in /usr/bin/chattr)
> ==3512==    by 0x525E76C: (below main) (libc-start.c:226)
> ==3512==  Address 0x700007fb7 is not stack'd, malloc'd or (recently) free'd
> ==3512== 
> ==3512== 
> ==3512== Process terminating with default action of signal 11 (SIGSEGV)
> ==3512==  Access not within mapped region at address 0x700007FB7
> ==3512==    at 0x52883B1: vfprintf (vfprintf.c:1630)
> ==3512==    by 0x528B1A3: buffered_vfprintf (vfprintf.c:2313)
> ==3512==    by 0x5285BDD: vfprintf (vfprintf.c:1316)
> ==3512==    by 0x53463D7: __vfprintf_chk (vfprintf_chk.c:35)
> ==3512==    by 0x503ABA8: ??? (in /lib/x86_64-linux-gnu/libcom_err.so.2.1)
> ==3512==    by 0x503AD1E: com_err (in /lib/x86_64-linux-gnu/libcom_err.so.2.1)
> ==3512==    by 0x401435: ??? (in /usr/bin/chattr)
> ==3512==    by 0x4010A0: ??? (in /usr/bin/chattr)
> ==3512==    by 0x525E76C: (below main) (libc-start.c:226)
> ==3512==  If you believe this happened as a result of a stack
> ==3512==  overflow in your program's main thread (unlikely but
> ==3512==  possible), you can try to increase the size of the
> ==3512==  main thread stack using the --main-stacksize= flag.
> ==3512==  The main thread stack size used in this run was 8388608.
> ==3512== 
> ==3512== HEAP SUMMARY:
> ==3512==     in use at exit: 0 bytes in 0 blocks
> ==3512==   total heap usage: 247 allocs, 247 frees, 22,481 bytes allocated
> ==3512== 
> ==3512== All heap blocks were freed -- no leaks are possible
> ==3512== 
> ==3512== For counts of detected and suppressed errors, rerun with: -v
> ==3512== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 2 from 2)
> Segmentation fault
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists