kcalloc manages count*sizeof overflow.
Cc: Bob Copeland <[email protected]>
Cc: Andrew Morton <[email protected]>
Signed-off-by: Fabian Frederick <[email protected]>
---
fs/omfs/inode.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/omfs/inode.c b/fs/omfs/inode.c
index ec58c76..ba88197 100644
--- a/fs/omfs/inode.c
+++ b/fs/omfs/inode.c
@@ -321,7 +321,7 @@ static int omfs_get_imap(struct super_block *sb)
goto out;
sbi->s_imap_size = array_size;
- sbi->s_imap = kzalloc(array_size * sizeof(unsigned long *), GFP_KERNEL);
+ sbi->s_imap = kcalloc(array_size, sizeof(unsigned long *), GFP_KERNEL);
if (!sbi->s_imap)
goto nomem;
--
1.9.1
On Wed, Jun 25, 2014 at 11:17 AM, Fabian Frederick <[email protected]> wrote:
> kcalloc manages count*sizeof overflow.
As far as I can tell, any overflow has happened long before, in
bitmap_size = DIV_ROUND_UP(sbi->s_num_blocks, 8);
where 'sbi->s_num_blocks' i san u64, and 'bitmap_size' is an 'int'.
I don't think the patch is necessarily a bad thing, but I think it
might be more important to sanity-check that part instead.
Linus
On Wed, Jun 25, 2014 at 11:27:21AM -0700, Linus Torvalds wrote:
> On Wed, Jun 25, 2014 at 11:17 AM, Fabian Frederick <[email protected]> wrote:
> > kcalloc manages count*sizeof overflow.
>
> As far as I can tell, any overflow has happened long before, in
>
> bitmap_size = DIV_ROUND_UP(sbi->s_num_blocks, 8);
>
> where 'sbi->s_num_blocks' i san u64, and 'bitmap_size' is an 'int'.
>
> I don't think the patch is necessarily a bad thing, but I think it
> might be more important to sanity-check that part instead.
Agreed - even though the FS data structures support 64-bit block
count, I've never seen an OMFS fs with more than about 2M blocks
(typical device had 20 gigs w/ 8k blocks). So it would make
sense to bail in omfs_fill_super if that number is greater than
2^31 or so.
(I am fine with the kcalloc patch too, though.)
--
Bob Copeland %% http://www.bobcopeland.com
On Wed, Jun 25, 2014 at 08:17:17PM +0200, Fabian Frederick wrote:
> kcalloc manages count*sizeof overflow.
>
> Cc: Bob Copeland <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Signed-off-by: Fabian Frederick <[email protected]>
Acked-by: Bob Copeland <[email protected]>
--
Bob Copeland %% http://www.bobcopeland.com
> Le 25 juin 2014 à 21:02, Bob Copeland <[email protected]> a écrit :
>
>
> On Wed, Jun 25, 2014 at 11:27:21AM -0700, Linus Torvalds wrote:
> > On Wed, Jun 25, 2014 at 11:17 AM, Fabian Frederick <[email protected]> wrote:
> > > kcalloc manages count*sizeof overflow.
> >
> > As far as I can tell, any overflow has happened long before, in
> >
> > bitmap_size = DIV_ROUND_UP(sbi->s_num_blocks, 8);
> >
> > where 'sbi->s_num_blocks' i san u64, and 'bitmap_size' is an 'int'.
> >
> > I don't think the patch is necessarily a bad thing, but I think it
> > might be more important to sanity-check that part instead.
>
> Agreed - even though the FS data structures support 64-bit block
> count, I've never seen an OMFS fs with more than about 2M blocks
> (typical device had 20 gigs w/ 8k blocks). So it would make
> sense to bail in omfs_fill_super if that number is greater than
> 2^31 or so.
We could use unsigned int for bitmap instead of int or simply u64 ?
>
> (I am fine with the kcalloc patch too, though.)
>
> --
> Bob Copeland %% http://www.bobcopeland.com
On Wed, Jun 25, 2014 at 10:03:26PM +0200, Fabian Frederick wrote:
> > >? ? ?bitmap_size = DIV_ROUND_UP(sbi->s_num_blocks, 8);
> > >
> > Agreed - even though the FS data structures support 64-bit block
> > count, I've never seen an OMFS fs with more than about 2M blocks
> > (typical device had 20 gigs w/ 8k blocks).? So it would make
> > sense to bail in omfs_fill_super if that number is greater than
> > 2^31 or so.
> We could use unsigned int for bitmap instead of int or simply u64 ?
It doesn't really make sense to be a signed int, sure -- but even so
making it a u64 without at least including a sanity check is probably
not the way to go.
OMFS allocates space for the entire free-space bitmap in memory, rather
than loading its blocks on demand. That's admittedly pretty dumb, but I
did it so that I could eventually support those FSes without a free-space
bitmap (I've never been asked for that feature, though, and didn't have
ReplayTV myself, so I don't believe that actually happened).
If s_num_blocks won't fit in a u32, well then that's a pretty huge chunk of
memory to allocate, and would represent a disk much bigger than the ones
that were available when this FS was used on a few devices.
(As for why the designers used u64 for all data structures, I guess just
optimism?)
--
Bob Copeland %% http://www.bobcopeland.com