Message ID | 20121117151126.GA16900@elgon.mountain |
---|---|
State | New, archived |
Headers | show |
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.
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
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.
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?
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?
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
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
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
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!
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 --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;
Using kmalloc_array() is a cleanup and it includes a check for integer overflows. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>