* [patch] UBIFS: use kmalloc_array() in recomp_data_node()
@ 2012-11-17 15:11 ` Dan Carpenter
0 siblings, 0 replies; 22+ messages in thread
From: Dan Carpenter @ 2012-11-17 15:11 UTC (permalink / raw)
To: Artem Bityutskiy; +Cc: kernel-janitors, linux-mtd, Adrian Hunter
Using kmalloc_array() is a cleanup and it includes a check for integer
overflows.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
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;
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [patch] UBIFS: use kmalloc_array() in recomp_data_node()
@ 2012-11-17 15:11 ` Dan Carpenter
0 siblings, 0 replies; 22+ messages in thread
From: Dan Carpenter @ 2012-11-17 15:11 UTC (permalink / raw)
To: Artem Bityutskiy; +Cc: kernel-janitors, linux-mtd, Adrian Hunter
Using kmalloc_array() is a cleanup and it includes a check for integer
overflows.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
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;
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [patch] UBIFS: use kmalloc_array() in recomp_data_node()
2012-11-17 15:11 ` Dan Carpenter
@ 2012-11-22 10:31 ` Artem Bityutskiy
-1 siblings, 0 replies; 22+ messages in thread
From: Artem Bityutskiy @ 2012-11-22 10:31 UTC (permalink / raw)
To: Dan Carpenter; +Cc: kernel-janitors, linux-mtd, Adrian Hunter
[-- Attachment #1: Type: text/plain, Size: 392 bytes --]
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.
--
Best Regards,
Artem Bityutskiy
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch] UBIFS: use kmalloc_array() in recomp_data_node()
@ 2012-11-22 10:31 ` Artem Bityutskiy
0 siblings, 0 replies; 22+ messages in thread
From: Artem Bityutskiy @ 2012-11-22 10:31 UTC (permalink / raw)
To: Dan Carpenter; +Cc: kernel-janitors, linux-mtd, Adrian Hunter
[-- Attachment #1: Type: text/plain, Size: 392 bytes --]
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.
--
Best Regards,
Artem Bityutskiy
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch] UBIFS: use kmalloc_array() in recomp_data_node()
2012-11-22 10:31 ` Artem Bityutskiy
@ 2012-11-22 11:14 ` Dan Carpenter
-1 siblings, 0 replies; 22+ messages in thread
From: Dan Carpenter @ 2012-11-22 11:14 UTC (permalink / raw)
To: Artem Bityutskiy; +Cc: kernel-janitors, linux-mtd, Adrian Hunter
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch] UBIFS: use kmalloc_array() in recomp_data_node()
@ 2012-11-22 11:14 ` Dan Carpenter
0 siblings, 0 replies; 22+ messages in thread
From: Dan Carpenter @ 2012-11-22 11:14 UTC (permalink / raw)
To: Artem Bityutskiy; +Cc: kernel-janitors, linux-mtd, Adrian Hunter
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch] UBIFS: use kmalloc_array() in recomp_data_node()
2012-11-22 11:14 ` Dan Carpenter
@ 2012-11-22 11:24 ` Artem Bityutskiy
-1 siblings, 0 replies; 22+ messages in thread
From: Artem Bityutskiy @ 2012-11-22 11:24 UTC (permalink / raw)
To: Dan Carpenter; +Cc: kernel-janitors, linux-mtd, Adrian Hunter
[-- Attachment #1: Type: text/plain, Size: 1567 bytes --]
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.
--
Best Regards,
Artem Bityutskiy
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch] UBIFS: use kmalloc_array() in recomp_data_node()
@ 2012-11-22 11:24 ` Artem Bityutskiy
0 siblings, 0 replies; 22+ messages in thread
From: Artem Bityutskiy @ 2012-11-22 11:24 UTC (permalink / raw)
To: Dan Carpenter; +Cc: kernel-janitors, linux-mtd, Adrian Hunter
[-- Attachment #1: Type: text/plain, Size: 1567 bytes --]
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.
--
Best Regards,
Artem Bityutskiy
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch] UBIFS: use kmalloc_array() in recomp_data_node()
2012-11-22 11:14 ` Dan Carpenter
@ 2012-11-22 11:26 ` Artem Bityutskiy
-1 siblings, 0 replies; 22+ messages in thread
From: Artem Bityutskiy @ 2012-11-22 11:26 UTC (permalink / raw)
To: Dan Carpenter; +Cc: kernel-janitors, linux-mtd, Adrian Hunter
[-- Attachment #1: Type: text/plain, Size: 1135 bytes --]
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?
--
Best Regards,
Artem Bityutskiy
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch] UBIFS: use kmalloc_array() in recomp_data_node()
@ 2012-11-22 11:26 ` Artem Bityutskiy
0 siblings, 0 replies; 22+ messages in thread
From: Artem Bityutskiy @ 2012-11-22 11:26 UTC (permalink / raw)
To: Dan Carpenter; +Cc: kernel-janitors, linux-mtd, Adrian Hunter
[-- Attachment #1: Type: text/plain, Size: 1135 bytes --]
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?
--
Best Regards,
Artem Bityutskiy
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch] UBIFS: use kmalloc_array() in recomp_data_node()
2012-11-22 11:26 ` Artem Bityutskiy
@ 2012-11-22 11:29 ` Artem Bityutskiy
-1 siblings, 0 replies; 22+ messages in thread
From: Artem Bityutskiy @ 2012-11-22 11:29 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-mtd, kernel-janitors, Adrian Hunter
[-- Attachment #1: Type: text/plain, Size: 284 bytes --]
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?
--
Best Regards,
Artem Bityutskiy
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch] UBIFS: use kmalloc_array() in recomp_data_node()
@ 2012-11-22 11:29 ` Artem Bityutskiy
0 siblings, 0 replies; 22+ messages in thread
From: Artem Bityutskiy @ 2012-11-22 11:29 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-mtd, kernel-janitors, Adrian Hunter
[-- Attachment #1: Type: text/plain, Size: 284 bytes --]
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?
--
Best Regards,
Artem Bityutskiy
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch] UBIFS: use kmalloc_array() in recomp_data_node()
2012-11-22 11:26 ` Artem Bityutskiy
@ 2012-11-22 11:50 ` Dan Carpenter
-1 siblings, 0 replies; 22+ messages in thread
From: Dan Carpenter @ 2012-11-22 11:50 UTC (permalink / raw)
To: Artem Bityutskiy; +Cc: kernel-janitors, linux-mtd, Adrian Hunter
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch] UBIFS: use kmalloc_array() in recomp_data_node()
@ 2012-11-22 11:50 ` Dan Carpenter
0 siblings, 0 replies; 22+ messages in thread
From: Dan Carpenter @ 2012-11-22 11:50 UTC (permalink / raw)
To: Artem Bityutskiy; +Cc: kernel-janitors, linux-mtd, Adrian Hunter
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch] UBIFS: use kmalloc_array() in recomp_data_node()
2012-11-22 11:29 ` Artem Bityutskiy
@ 2012-11-22 12:09 ` Dan Carpenter
-1 siblings, 0 replies; 22+ messages in thread
From: Dan Carpenter @ 2012-11-22 12:09 UTC (permalink / raw)
To: Artem Bityutskiy; +Cc: linux-mtd, kernel-janitors, Adrian Hunter
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch] UBIFS: use kmalloc_array() in recomp_data_node()
@ 2012-11-22 12:09 ` Dan Carpenter
0 siblings, 0 replies; 22+ messages in thread
From: Dan Carpenter @ 2012-11-22 12:09 UTC (permalink / raw)
To: Artem Bityutskiy; +Cc: linux-mtd, kernel-janitors, Adrian Hunter
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch] UBIFS: use kmalloc_array() in recomp_data_node()
2012-11-22 11:24 ` Artem Bityutskiy
@ 2012-11-22 12:33 ` Dan Carpenter
-1 siblings, 0 replies; 22+ messages in thread
From: Dan Carpenter @ 2012-11-22 12:33 UTC (permalink / raw)
To: Artem Bityutskiy; +Cc: kernel-janitors, linux-mtd, Adrian Hunter
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch] UBIFS: use kmalloc_array() in recomp_data_node()
@ 2012-11-22 12:33 ` Dan Carpenter
0 siblings, 0 replies; 22+ messages in thread
From: Dan Carpenter @ 2012-11-22 12:33 UTC (permalink / raw)
To: Artem Bityutskiy; +Cc: kernel-janitors, linux-mtd, Adrian Hunter
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch] UBIFS: use kmalloc_array() in recomp_data_node()
2012-11-22 12:33 ` Dan Carpenter
@ 2012-11-22 14:48 ` Artem Bityutskiy
-1 siblings, 0 replies; 22+ messages in thread
From: Artem Bityutskiy @ 2012-11-22 14:48 UTC (permalink / raw)
To: Dan Carpenter; +Cc: kernel-janitors, linux-mtd, Adrian Hunter
[-- Attachment #1: Type: text/plain, Size: 467 bytes --]
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!
--
Best Regards,
Artem Bityutskiy
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch] UBIFS: use kmalloc_array() in recomp_data_node()
@ 2012-11-22 14:48 ` Artem Bityutskiy
0 siblings, 0 replies; 22+ messages in thread
From: Artem Bityutskiy @ 2012-11-22 14:48 UTC (permalink / raw)
To: Dan Carpenter; +Cc: kernel-janitors, linux-mtd, Adrian Hunter
[-- Attachment #1: Type: text/plain, Size: 467 bytes --]
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!
--
Best Regards,
Artem Bityutskiy
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch] UBIFS: use kmalloc_array() in recomp_data_node()
2012-11-22 14:48 ` Artem Bityutskiy
@ 2012-11-22 16:41 ` Dan Carpenter
-1 siblings, 0 replies; 22+ messages in thread
From: Dan Carpenter @ 2012-11-22 16:41 UTC (permalink / raw)
To: Artem Bityutskiy; +Cc: kernel-janitors, linux-mtd, Adrian Hunter
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch] UBIFS: use kmalloc_array() in recomp_data_node()
@ 2012-11-22 16:41 ` Dan Carpenter
0 siblings, 0 replies; 22+ messages in thread
From: Dan Carpenter @ 2012-11-22 16:41 UTC (permalink / raw)
To: Artem Bityutskiy; +Cc: kernel-janitors, linux-mtd, Adrian Hunter
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
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2012-11-22 16:42 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-11-17 15:11 [patch] UBIFS: use kmalloc_array() in recomp_data_node() Dan Carpenter
2012-11-17 15:11 ` Dan Carpenter
2012-11-22 10:31 ` Artem Bityutskiy
2012-11-22 10:31 ` Artem Bityutskiy
2012-11-22 11:14 ` Dan Carpenter
2012-11-22 11:14 ` Dan Carpenter
2012-11-22 11:24 ` Artem Bityutskiy
2012-11-22 11:24 ` Artem Bityutskiy
2012-11-22 12:33 ` Dan Carpenter
2012-11-22 12:33 ` Dan Carpenter
2012-11-22 14:48 ` Artem Bityutskiy
2012-11-22 14:48 ` Artem Bityutskiy
2012-11-22 16:41 ` Dan Carpenter
2012-11-22 16:41 ` Dan Carpenter
2012-11-22 11:26 ` Artem Bityutskiy
2012-11-22 11:26 ` Artem Bityutskiy
2012-11-22 11:29 ` Artem Bityutskiy
2012-11-22 11:29 ` Artem Bityutskiy
2012-11-22 12:09 ` Dan Carpenter
2012-11-22 12:09 ` Dan Carpenter
2012-11-22 11:50 ` Dan Carpenter
2012-11-22 11:50 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.