Aw: Re: Sanity check for m_ialloc_blks in libxfs_mount()

Aw: Re: Sanity check for m_ialloc_blks in libxfs_mount()

["Marc Schönefeld"]

Hi Dave, [resent due to smtp error] 
 
thanks for the help, now using the for-next branch, there is still an Arithmetic exception, however somewhere else:
 
Program received signal SIGFPE, Arithmetic exception.
xfs_ialloc_setup_geometry (mp=mp@entry=0x6a5e60 <xmount>) at xfs_ialloc.c:2792
2792 do_div(icount, igeo->ialloc_blks);

Thanks
Marc 

 

Gesendet: Donnerstag, 17. Oktober 2019 um 00:28 Uhr
Von: "Dave Chinner" <david@fromorbit.com>
An: "Marc Schönefeld" <marc.schoenefeld@gmx.org>
Cc: linux-xfs@vger.kernel.org
Betreff: Re: Sanity check for m_ialloc_blks in libxfs_mount()
On Wed, Oct 16, 2019 at 09:08:51PM +0200, "Marc Schönefeld" wrote:
> Hi all, 
>
> it looks like there is a sanity check missing for the divisor
> (m_ialloc_blks) in line 664 of xfsprogs-5.2.1/libxfs/init.c: 
> Program received signal SIGFPE, Arithmetic exception.
>
> 0x0000000000427ddf in libxfs_mount (mp=mp@entry=0x6a2de0 <xmount>, sb=sb@entry=0x6a2de0 <xmount>, dev=18446744073709551615, 
>     logdev=<optimized out>, rtdev=<optimized out>, flags=flags@entry=1) at init.c:663
>
> which is 
>
>     663                 mp->m_maxicount = XFS_FSB_TO_INO(mp,
>     664                                 (mp->m_maxicount / mp->m_ialloc_blks) *
>     665                                  mp->m_ialloc_blks);

That's code is gone now. The current calculation in the dev tree is
quite different thanks to:

commit 3a05ab227ebd5982f910f752692c87005c7b3ad3
Author: Darrick J. Wong <darrick.wong@oracle.com>
Date: Wed Aug 28 12:08:08 2019 -0400

xfs: refactor inode geometry setup routines

Source kernel commit: 494dba7b276e12bc3f6ff2b9b584b6e9f693af45

Migrate all of the inode geometry setup code from xfs_mount.c into a
single libxfs function that we can share with xfsprogs.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Eric Sandeen <sandeen@sandeen.net>

And so it doesn't have a divide-by-zero vector in it anymore.

So it's probably best that you update your source tree to the latest
for-next and retest. It's almost always a good idea to test against
the latest dev tree, that way you aren't finding bugs we've already
found and fixed...

> In case it would be required I have a reproducer file for this,
> which I can share via pm. The bug is reachable from user input via
> the "xfs_db -c _cmd_ _xfsfile_" command.   

I'm guessing that you are fuzzing filesystem images and the issue is
that the inode geometry values in the superblock have been fuzzed to
be incorrect? What fuzzer are you using to generate the image, and
what's the mkfs.xfs output that was used to create the base image
that was then fuzzed?

Cheers,

Dave.
--
Dave Chinner
david@fromorbit.com

Re: Re: Sanity check for m_ialloc_blks in libxfs_mount()

[Dave Chinner]

On Thu, Oct 17, 2019 at 08:56:21AM +0200, "Marc Schönefeld" wrote:
> Hi Dave, [resent due to smtp error] 

It got rejected because you sent a HTML-only email to the list.

> thanks for the help, now using the for-next branch, there is still an Arithmetic exception, however somewhere else:

Also, while on list-etiquette, can you please wrap your comments at
72 columns, and please try not to top post as it makes it really hard
to keep the discussion context straight.

> Program received signal SIGFPE, Arithmetic exception.
> xfs_ialloc_setup_geometry (mp=mp@entry=0x6a5e60 <xmount>) at xfs_ialloc.c:2792
> 2792 do_div(icount, igeo->ialloc_blks);

So, same as last time, there's a discrepancy between two fields
in the superblock: sbp->sb_inopblock and sbp->sb_inopblog.

Basically, the inodes per block is smaller than the log2 value of
the number of inodes per block. which implies that sb_inopblog is
greater than 7, unless you've configured the filesystem with a block
size > 4kB.

It also implies that this verifier check:

	(sbp->sb_blocklog - sbp->sb_inodelog != sbp->sb_inopblog)

has also passed, which means either sb_blocklog (the filesystem
block size) and/or the sb_inodelog (inode size) values have also
been tweaked in a way for this test to pass, but to still ahve an
a mismatch betwen sb_inopblock and sb_inopblog.

But we also have a check:

	sbp->sb_inopblock != howmany(sbp->sb_blocksize,sbp->sb_inodesize)

which checks taht the number of inodes per block matches the
filesystem block size and the inode size configured, and:

	sbp->sb_blocksize != (1 << sbp->sb_blocklog)

and
	sbp->sb_inodesize != (1 << sbp->sb_inodelog)

which validate the log2 values match the byte based values.

So I can't see how it got to this code with such a mismatch unless
xfs_db actually ignored it.  And without all the output from xfs_db,
I don't know what errors it has detected and ignored. Hence, when
reporting a problem, can you please include the full output from the
program that has failed, including the command line used to invoke
it?

Further, knowing what the filesystem geometry is supposed to be
tells me an awful lot, too, which is why I asked this last time:

> I'm guessing that you are fuzzing filesystem images and the issue is
> that the inode geometry values in the superblock have been fuzzed to
> be incorrect? What fuzzer are you using to generate the image, and
> what's the mkfs.xfs output that was used to create the base image
> that was then fuzzed?

Because then I know what the values are supposed to be before I look
at the fuzzed image and can clearly tell waht has been manipulated
by the fuzzer.

Also, keep in mind that xfs_db is a diagnostic tool for developers -
it's not a user tool. We use it for digging around in corrupt
structures and hence it often reports then ignores corruption iti
detects so it can display the corrupt structure to the user. i.e.
it's a tool intended to what it is asked to do regardless of the
fact it might not be able to handle the result cleanly.

Hence I'm not sure there is a huge value in actually fuzz testing
xfs_db. It's certainly not at all interesting from a security point
of view...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com