Re: fix to CVE-2009-4307

[Xi Wang <xi.wang () gmail com>]

On Apr 3, 2012, at 10:55 PM, Kurt Seifried wrote:
> For #2 I'm not sure how we handle something like a compiler possibly
> mangling code so that an issue is introduced (is that a compiler
> problem? a code problem? the intersection of both? Steve: can I get a
> comment/referees decision here =)

Thanks for bringing this up.

I think the compiler is all right in this case.  The code is not.

CVE-2009-4307 says that an attacker could trigger a division by
zero by crafting a large s_log_groups_per_flex.

The first commit (503358ae) fixes the division by zero.  The fix
is not perfect because:

1) Theoretically, a standard-conforming C compiler could generate
code that is still vulnerable to division by zero, but I was not
aware of any compilers doing that.

2) Logically, we should have groups_per_flex = 2^s_log_groups_per_flex,
and the fix doesn't really ensure that.  This is obviously not good,
but not sure how bad the consequence would be.

BTW, the second commit (d50f2ab6) might still allow a buffer overflow
later.  See another patch https://lkml.org/lkml/2012/2/20/422 (though
it was rejected).

In ext4_resize_fs():

   flexbg_size = 1 << es->s_log_groups_per_flex;
   ...
   flex_gd = alloc_flex_gd(flexbg_size);

and in alloc_flex_gd():

   flex_gd->count = flexbg_size;
   flex_gd->groups = kmalloc(sizeof(...) * flexbg_size, ...);

Note that the kmalloc size could be smaller than expected due to
multiplication overflow (flexbg_size = 1 << s_log_groups_per_flex
could be very large since s_log_groups_per_flex could be as large
as 31).  Array access flex_gd groups[i] could be out of bounds in
that case.

- xi