linux-kernel - Re: [PATCH] x86/cpuid: Deal with broken firmware once more

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: <alpine.DEB.2.20.1611091936480.3501@nanos>
Date:   Wed, 9 Nov 2016 19:37:40 +0100 (CET)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     "Charles (Chas) Williams" <ciwillia@...cade.com>
cc:     Peter Zijlstra <peterz@...radead.org>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        x86@...nel.org, LKML <linux-kernel@...r.kernel.org>,
        "M. Vefa Bicakci" <m.v.b@...box.com>,
        Borislav Petkov <bp@...en8.de>
Subject: Re: [PATCH] x86/cpuid: Deal with broken firmware once more

On Wed, 9 Nov 2016, Charles (Chas) Williams wrote:
> On 11/09/2016 11:03 AM, Peter Zijlstra wrote:
> > On Wed, Nov 09, 2016 at 04:35:51PM +0100, Thomas Gleixner wrote:
> > > Both ACPI and MP specifications require that the APIC id in the respective
> > > tables must be the same as the APIC id in CPUID.
> > > 
> > > The kernel retrieves the physical package id from the APIC id during the
> > > ACPI/MP table scan and builds the physical to logical package map.
> > > 
> > > There exist Virtualbox and Xen implementations which violate the spec. As
> > > a
> > 
> > ISTR it was VMware, not VirtualBox, but whatever.. they're both crazy
> > virt stuff.
> 
> Yes, this was VMware in particular.  It would be good to get this comment
> right so as not to mislead anyone.

Sure, will fix.

> 
> > >  /*
> > > + * The physical to logical package id mapping is initialized from the
> > > + * acpi/mptables information. Make sure that CPUID actually agrees with
> > > + * that.
> > > + */
> > > +static void sanitize_package_id(struct cpuinfo_x86 *c)
> > > +{
> > > +#ifdef CONFIG_SMP
> > > +	unsigned int pkg, apicid, cpu = smp_processor_id();
> > > +
> > > +	apicid = apic->cpu_present_to_apicid(cpu);
> > > +	pkg = apicid >> boot_cpu_data.x86_coreid_bits;
> > > +
> > > +	if (apicid != c->initial_apicid) {
> > > +		pr_err(FW_BUG "CPU%u: APIC id mismatch. Firmware: %x CPUID:
> > > %x\n",
> > > +		       cpu, apicid, c->initial_apicid);
> > 
> > Should we not also 'fix' c->initial_apicid ?
> 
> Since we have c->apicid and c->initial_apicid it seems reasonable to keep one
> set to the "correct" value.  I don't think c->initial_apicid is used past
> this.

It is, but just for a printk in the MCE code, so it should not matter at all.

> I should have some tests on this patch later today.
>