linux-kernel - Re: MCE boot crash in qemu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20090617065720.GA4852@elte.hu>
Date:	Wed, 17 Jun 2009 08:57:20 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Pekka Enberg <penberg@...helsinki.fi>,
	Hidetoshi Seto <seto.hidetoshi@...fujitsu.com>,
	"H. Peter Anvin" <hpa@...or.com>
Cc:	Andi Kleen <andi@...stfloor.org>,
	Vegard Nossum <vegard.nossum@...il.com>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: MCE boot crash in qemu


* Pekka Enberg <penberg@...helsinki.fi> wrote:

> On Mon, 2009-06-15 at 16:22 +0300, Pekka Enberg wrote:
> > On Mon, 2009-06-15 at 14:52 +0200, Andi Kleen wrote:
> > > x86: mce: Handle banks == 0 case in K7 quirk
> > > 
> > > This happens on QEMU which reports MCA capability, but no banks.
> > > Without this patch there is a buffer overrun and boot ops because the code
> > > would try to initialize the 0 element of a zero length kmalloc()
> > > buffer.
> > > 
> > > Signed-off-by: Andi Kleen <ak@...ux.intel.com>
> > 
> > This fixes the bug for me!
> > 
> > Tested-by: Pekka Enberg <penberg@...helsinki.fi>
> 
> Ingo, I hit this again in my testing after rebasing to 
> linus/master so I really would like this in mainline.

yep, i've tidied up the changelog and have committed it to 
x86/urgent.

But the bank[] code is quirky and butt-ugly and that needs to be 
cleaned up - it's no wonder that bugs like this slip in.

 - There's zero description about the hw model it represents 
   and how it relates to the bank[] array - what do the banks mean, 
   how are they organized.

 - It's full of magic constants and implicitly-assumed size
   calculations with little explanation and little extensibility:

       ...
                if (c->x86 == 15 && banks > 4) {
                        /*
                         * disable GART TBL walk error reporting, which
                         * trips off incorrectly with the IOMMU & 3ware
                         * & Cerberus:
                         */
                        clear_bit(10, (unsigned long *)&bank[4]);
                }
       ...
                bank = kmalloc(banks * sizeof(u64), GFP_KERNEL);
       ...
                memset(bank, 0xff, banks * sizeof(u64));
       ...

 - There's lots of bitmaps, arrays, flags interacting, creating a
   maze of logic.

Instead of this messy code, the proper approach is to introduce an 
abstract data structure representing the attributes of an MCE bank 
register:

  struct mce_bank_register {
	int enabled;
	int polled;
	int dont_init;
	int msr_idx;
  };

( There's lots of other structural problems with the MCE code too - 
  but now that it's unified lets first fix the most obvious ones... )

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/