Re: [patch] UBIFS: use kmalloc_array() in recomp_data_node()

[Dan Carpenter <dan.carpenter@oracle.com>]

On Thu, Nov 22, 2012 at 01:24:10PM +0200, Artem Bityutskiy wrote:
> On Thu, 2012-11-22 at 14:14 +0300, Dan Carpenter wrote:
> > On Thu, Nov 22, 2012 at 12:31:37PM +0200, Artem Bityutskiy wrote:
> > > On Sat, 2012-11-17 at 18:11 +0300, Dan Carpenter wrote:
> > > >  	out_len = le32_to_cpu(dn->size);
> > > > -	buf = kmalloc(out_len * WORST_COMPR_FACTOR, GFP_NOFS);
> > > > +	buf = kmalloc_array(out_len, WORST_COMPR_FACTOR, GFP_NOFS);
> > > >  	if (!buf)
> > > >  		return -ENOMEM;
> > > 
> > > I think this makes the code unreadable, because we really allocate a
> > > buffer, not an array.
> > 
> > The problem with the original code is that the multiply looks very
> > suspect.  Everyone who reads it has to backtrack to find where
> > dn->size is capped.
> > 
> > I guess in one sense we never allocate an array, we always declare
> > it on the stack.  We debated the naming and there really isn't a
> > good name.  kmalloc_safe() isn't right either.  But anyway, the
> > intent is that eventually someone will right a coccinelle script
> > which replaces all these allocations with kmalloc_array().
> > 
> > When I look at this code more, I still don't see a place where
> > dn->size is capped.  So I think we *need* the integer overflow
> > check as an integer overflow fix and not just as a cleanup.
> 
> It is validated in fs/ubifs/io.c in 'ubifs_check_node()'.
> 
> 'dn' stands for 'direntry node'. We read it from the media and validate
> it immediately after we've read it, including 'dn->len'.
> 
> The entire code is written with the following assumption that whatever
> is read from the flash media is validated.

It's actually dn->size that we care about here.  That's not checked
in ubifs_check_node().  :(  It may be checked somewhere else, I'm
still looking.

regards,
dan cparenter