Re: [Patch] NULL pointer deref with corrupted squashfs image

[Tom Rini <trini@kernel.crashing.org>]

On Fri, Jan 16, 2009 at 08:07:32PM +0100, Jörn Engel wrote:
> On Fri, 16 January 2009 18:45:25 +0100, Eric Sesterhenn wrote:
> > 
> > Non-PPC targets shouldnt inflate images to memory address 0.
> > check strm->next_out for NULL in case on non PPC architecture
> > to prevent a NULL-pointer dereference while inflating corrupted images.
> > 
> > Signed-off-by: Eric Sesterhenn <snakebyte@gmx.de>
> > 
> > --- linux/lib/zlib_inflate/inflate.c.orig	2009-01-16 15:40:04.000000000 +0100
> > +++ linux/lib/zlib_inflate/inflate.c	2009-01-16 15:41:42.000000000 +0100
> > @@ -347,8 +347,12 @@ int zlib_inflate(z_streamp strm, int flu
> >      static const unsigned short order[19] = /* permutation of code lengths */
> >          {16, 17, 18, 0, 8, 7, 9, 6, 10, 5, 11, 4, 12, 3, 13, 2, 14, 1, 15};
> >  
> > -    /* Do not check for strm->next_out == NULL here as ppc zImage
> > -       inflates to strm->next_out = 0 */
> > +    /* Since ppc zImage inflates to 0 we only check
> > +       strm->next_out for non-ppc targets0 */
> > +#ifndef CONFIG_PPC
> > +    if (!strm->next_out)
> > +        return Z_STREAM_ERROR;
> > +#endif
> >  
> >      if (strm == NULL || strm->state == NULL ||
> >          (strm->next_in == NULL && strm->avail_in != 0))
> 
> Unzipping to NULL is not an attribute of PPC, but rather of being called
> from a bootloader that wants to unpack a kernel to NULL.  Which makes
> this patch wrong on two accounts.  It leaves the bug for CONFIG_PPC and
> it may break bootloaders on other architectures.  A quick grep shows
> xtensa - no clue whether it loads the kernel to NULL or elsewhere.

I was about to say... so thanks! :)

> I'd prefer zlib_inflate to take a flag parameter to disable the check.
> Then we can have two wrappers roughly like this:
> 
> int zlib_inflate(z_streamp strm, int flush)
> {
> 	return __zlib_inflate(strm, flush, 1);
> }
> 
> int zlib_inflate_null_ok_for_bootloaders_only(z_streamp strm, int flush)
> {
> 	return __zlib_inflate(strm, flush, 0);
> }
> 
> Or we could even make the two wrappers inline functions and move them to
> zlib.h.

Sounds like a plan to me, except maybe zlib_inflate_unsafe() and a
comment above the wrapper saying what/why is going on?

-- 
Tom Rini
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html