Bug 211668

Steve neglected to mention that this was on a corrupted cramfs filesytem :) 
Steve, do you still have the image that produced this crash?

Eric is right. This is only seen with a corrupt image. The easiest way to
repoduce the problem is download fsfuzzer from
http://people.redhat.com/sgurbb/files. Untar, make, then as root run "./fsfuzz
cramfs"

In about 10 seconds you'll have a file with the problem. If you don't want to do
this, I can crash my machine and make an image, but its really simple to make
your own.

Regarding squashfs, its on bugzilla #211237. I have given Eric an image for
squashfs that is pure evil. Maybe he can attach that image to 211237 also.

Hi Guys


Dave Jones sent me a link to this bug, and added me to the CC list, as it is
perhaps of relevance to Squashfs.


Anyway, the good news is it is not oopsing in zlib_inflate().  That's good
because zlib_inflate should checksum the zlib data and return error if the data
is corrupt.  Zlib oopsing on invalid data would be a really horrible bug to fix :-)

The line "Error -3 while decompressing!" is printed after zlib_inflate has
returned.  What then happens is cramfs_uncompress_block returns 0 bytes which
means cramfs_readpage returns a completely zero-filled block.  This means the
"Unable to handle kernel paging request at 000000002252d15d RIP:" can't be
generated by cramfs_readpage returning failure, because it never does.  This
means it is probably caused by stack corruption or other memory corruption.

The error line "ffffffff8852d146(-1711276009)->ffff810033dad000(4096)" is
generated by cramfs_uncompress_block which I think indicates the error.  The
size of the source block is given as -1711276009, which both zlib_inflate() and
cramfs_read() handle as an unsigned int, or a large number.  Cramfs_read()
doesn't fall over reading this because it never reads more than BUFFER_SIZE
bytes (16K, 4*PAGE_CACHE_SIZE), but it is likely zlib_inflate() is corrupting
memory given such a large block to evaluate/checksum.

Ultimately, the bug is caused by the corrupted block length field read by
cramfs_readpage().  The fix is to add a sanity check in cramfs_readpage() when
the block length is read.

Line 509 (in kernel 2.6.18) cramfs/inode.c reads

                if (compr_len == 0)

Changing this to 

                if (compr_len == 0 || compr_len > (PAGE_CACHE_SIZE << 1))

Should do the trick, which is to generate a zero-filled page when the block
length is out of bounds, and not try to read or decompress it.  BTW the
(PAGE_CACHE_SIZE << 1) size is important - even though the uncompressed data is
not going to be larger than PAGE_CACHE_SIZE, gzip sometimes generates compressed
data larger than the original source data, but never more than PAGE_CACHE_SIZE
<< 1.  Of course cramfs could use the original uncompressed data in this case,
but it doesn't.

Phillip

BTW can someone send me the Squashfs image that produces bug number 211237 ?

Thanks

Phillip

Created attachment 139689 [details]
cramfs patch

To save anyone else time typing this in, I'm attaching the patch described
above. It does indeed seem to solve all issues with cramfs that I can see for
now. Please  include this fix in the next kernel release if possible. Thanks!

So, is anyone going to send the patch to lkml? The fix should be public before
the month of kernel bugs starts (nov 1). 

I can do it, although I don't have the best understanding of zlib, to answer any
followup questions...

Philip, on second thought this is your work and it'd be best for you to send it
out if you can.  I do wonder, though, if the code should silently zero-fill in
this case?  Shouldn't it at least issue a warning about corrupted compressed data?

Philip, on third thought :)

It seems to me that it would be better for cramfs_read to do the checking here.
 It claims:

/*
 * Returns a pointer to a buffer containing at least LEN bytes of
 * filesystem starting at byte offset OFFSET into the filesystem.
 */

but it doesn't... if you pass it a very large length, the buffers do NOT cover
it.   But it still returns a pointer for zlib to ultimately use, -thinking- that
it has "len" bytes available starting from there.

What do you think of this?

Index: linux-2.6.18-1.2732.el5/fs/cramfs/inode.c
===================================================================
--- linux-2.6.18-1.2732.el5.orig/fs/cramfs/inode.c
+++ linux-2.6.18-1.2732.el5/fs/cramfs/inode.c
@@ -152,7 +152,7 @@ static void *cramfs_read(struct super_bl
        unsigned long devsize;
        char *data;
 
-       if (!len)
+       if (!len || len > (PAGE_CACHE_SIZE << 1))
                return NULL;
        blocknr = offset >> PAGE_CACHE_SHIFT;
        offset &= PAGE_CACHE_SIZE - 1;

This would take care of it for any caller of cramfs_read() (granted, today there
is only one, but I think this makes the function more consistent w/ it's
documentation)

Hi Eric,


I thought about putting the check into cramfs_read(), but decided against it for
a number of inter-related reasons:

1. Putting it where I did in cramfs_readpage() means a sensible error message
can be printed.  The patch I sent to LKML will complain that the block length is
bad.  Putting the check in cramfs_read() looses this knowledge.


2. Cramfs_read() is called from a large number of places (not just one place). 
However all the other calls to Cramfs_read() pass in constant compile-time
length values (i.e. sizeof(xxx)).  Putting the check in cramfs_read therefore
seemed unnecessary overhead which isn't needed in the majority of cases.

3. Only doing the check in Cramfs_read() means in cases of an incorrect length,
the read_mutex is unnecessarily taken and released around the calls to
cramfs_uncompress_block() and cramfs_read().
 
4. With the current code, cramfs_read() is called as a argument to
cramfs_uncompress_block(), i.e. 

  bytes_filled = cramfs_uncompress_block(pgdata,
                                 PAGE_CACHE_SIZE,
                                 cramfs_read(sb, start_offset, compr_len),
                                 compr_len);

Making cramfs_read() return NULL means a NULL pointer is passed into
cramfs_uncompressed_block() along with the corrupted length field.  Now, this
means an additional check has to be done in cramfs_uncompress_block() to check
for the NULL pointer, i.e. the check in cramfs_read() achieves nothing on its
own.    There isn't a check in cramfs_uncompress_block(), which means it will
happily pass a NULL pointer to zlib_inflate.  The reason why the current NULL
return in cramfs_read() doesn't cause problems is because nothing calls
cramfs_read() with 0 bytes and so it never does return NULL.

  The alternative of rewriting the call to cramfs_uncompress_block, so the call
 to cramfs_read is done separately (not as part of the arguments), means an
additional check in cramfs_readpage() for the NULL return...

Checking the block length in cramfs_readpage() seems a more sensible approach.

Phillip

Hi Philip, yep, I just went down those paths and I tend to agree with you. 
Though I do think hardening cramfs_read would still be a good idea as well, it
can't be good to ask for a pointer to a gigabyte worth of memory, and get an
address back without error.  :)  but, your patch will clean up the present issue
just fine.

Thanks,

-Eric

patch upstream in mm:

http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.19-rc5/2.6.19-rc5-mm1/broken-out/corrupted-cramfs-filesystems-cause-kernel-oops.patch

I'll get this pushed out to RH kernels now.

Hi Eric,


Eric wrote "Though I do think hardening cramfs_read would still be a good idea
as well"


I didn't mean to imply you were wrong.  I think hardening cramfs_read and cramfs
in general is a good idea.


Phillip

Phillip, understood :)
-Eric

I think this was fixed in kernel-2.6.18-1.2869. Closing.