Re: fix to CVE-2009-4307

[Kurt Seifried <kseifried () redhat com>]

On 04/03/2012 04:32 PM, akuster wrote:
> Hello,
>
> Was there a CVE assigned to commit d50f2ab6f050311dbf7b8f5501b25f0bf64a439b?
>
> Commit 503358ae01b70ce6909d19dd01287093f6b6271c ("ext4: avoid divide by
> zero when trying to mount a corrupted file system") fixes CVE-2009-4307
> by performing a sanity check on s_log_groups_per_flex, since it can be
> set to a bogus value by an attacker.
>
> - Armin

I assume you are talking about this:

http://git.kernel.org/?p=virt/kvm/kvm.git;a=commitdiff;h=d50f2ab6f050311dbf7b8f5501b25f0bf64a439b

================================================
Commit 503358ae01b70ce6909d19dd01287093f6b6271c ("ext4: avoid divide by
zero when trying to mount a corrupted file system") fixes CVE-2009-4307
by performing a sanity check on s_log_groups_per_flex, since it can be
set to a bogus value by an attacker.

sbi->s_log_groups_per_flex = sbi->s_es->s_log_groups_per_flex;
groups_per_flex = 1 << sbi->s_log_groups_per_flex;

if (groups_per_flex < 2) { ... }

This patch fixes two potential issues in the previous commit.

1) The sanity check might only work on architectures like PowerPC.
On x86, 5 bits are used for the shifting amount.  That means, given a
large s_log_groups_per_flex value like 36, groups_per_flex = 1 << 36
is essentially 1 << 4 = 16, rather than 0.  This will bypass the check,
leaving s_log_groups_per_flex and groups_per_flex inconsistent.

2) The sanity check relies on undefined behavior, i.e., oversized shift.
A standard-confirming C compiler could rewrite the check in unexpected
ways.  Consider the following equivalent form, assuming groups_per_flex
is unsigned for simplicity.

groups_per_flex = 1 << sbi->s_log_groups_per_flex;
if (groups_per_flex == 0 || groups_per_flex == 1) {

We compile the code snippet using Clang 3.0 and GCC 4.6.  Clang will
completely optimize away the check groups_per_flex == 0, leaving the
patched code as vulnerable as the original.  GCC keeps the check, but
there is no guarantee that future versions will do the same.

Signed-off-by: Xi Wang <xi.wang () gmail com>
Signed-off-by: "Theodore Ts'o" <tytso () mit edu>
Cc: stable () vger kernel org
================================================

What specific do you want a CVE assigned for?

For #1 I can see a CVE of the "a previous patch didn't completely fix
the issue, yada yada" type.

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 =)

-- 
Kurt Seifried Red Hat Security Response Team (SRT)