diff mbox

UBIFS: use kmalloc_array() in recomp_data_node()

Message ID 20121117151126.GA16900@elgon.mountain
State New, archived
Headers show

Commit Message

Dan Carpenter Nov. 17, 2012, 3:11 p.m. UTC 
Using kmalloc_array() is a cleanup and it includes a check for integer
overflows.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Comments

Artem Bityutskiy Nov. 22, 2012, 10:31 a.m. UTC | #1 
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.
Dan Carpenter Nov. 22, 2012, 11:14 a.m. UTC | #2 
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.

regards,
dan carpenter
Artem Bityutskiy Nov. 22, 2012, 11:24 a.m. UTC | #3 
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.
Artem Bityutskiy Nov. 22, 2012, 11:26 a.m. UTC | #4 
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().

Did you consider kcalloc() ? Just like malloc() / calloc() libc
functions?
Artem Bityutskiy Nov. 22, 2012, 11:29 a.m. UTC | #5 
On Thu, 2012-11-22 at 13:26 +0200, Artem Bityutskiy wrote:
> Did you consider kcalloc() ? Just like malloc() / calloc() libc
> functions? 

Ah, there is kcaoloc already, so I guess then we could just put the
overflow check there instead?
Dan Carpenter Nov. 22, 2012, 11:50 a.m. UTC | #6 
On Thu, Nov 22, 2012 at 01:26:43PM +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().
> 
> Did you consider kcalloc() ? Just like malloc() / calloc() libc
> functions?

We already have a kcalloc() but it zeroes out the memory.

regards,
dan carpenter
Dan Carpenter Nov. 22, 2012, 12:09 p.m. UTC | #7 
On Thu, Nov 22, 2012 at 01:29:33PM +0200, Artem Bityutskiy wrote:
> On Thu, 2012-11-22 at 13:26 +0200, Artem Bityutskiy wrote:
> > Did you consider kcalloc() ? Just like malloc() / calloc() libc
> > functions? 
> 
> Ah, there is kcaoloc already, so I guess then we could just put the
> overflow check there instead?

Yep.  That's there already.

regards,
dan carpenter
Dan Carpenter Nov. 22, 2012, 12:33 p.m. UTC | #8 
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
Artem Bityutskiy Nov. 22, 2012, 2:48 p.m. UTC | #9 
On Thu, 2012-11-22 at 15:33 +0300, Dan Carpenter wrote:
> 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.

Wow, despite us trying to be very careful about validating what we read
from flash, it seems we indeed never validate 'size'... Let me invent a
fix for this, which should also be sent to -stable.

Thanks!
Dan Carpenter Nov. 22, 2012, 4:41 p.m. UTC | #10 
On Thu, Nov 22, 2012 at 04:48:09PM +0200, Artem Bityutskiy wrote:
> On Thu, 2012-11-22 at 15:33 +0300, Dan Carpenter wrote:
> > 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.
> 
> Wow, despite us trying to be very careful about validating what we read
> from flash, it seems we indeed never validate 'size'... Let me invent a
> fix for this, which should also be sent to -stable.
> 

Thanks.  Could you give me the Reported-by tag?

regards,
dan carpenter
diff mbox

Patch

diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
index afaad07..23c2a20 100644
--- a/fs/ubifs/journal.c
+++ b/fs/ubifs/journal.c
@@ -1104,7 +1104,7 @@  static int recomp_data_node(struct ubifs_data_node *dn, int *new_len)
 	int err, len, compr_type, out_len;
 
 	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;