Date:	Wed, 24 Jul 2013 16:44:45 +0200
From:	Torsten Kaiser <just.for.lkml@...glemail.com>
To:	Borislav Petkov <bp@...en8.de>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>, Jacob Shin <jacob.shin@....com>,
	Johannes Hirte <johannes.hirte@....tu-ilmenau.de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH]Fix early microcode loading on AMD

On Wed, Jul 24, 2013 at 3:56 PM, Borislav Petkov <bp@...en8.de> wrote:
> On Tue, Jul 23, 2013 at 06:57:12PM +0200, Torsten Kaiser wrote:
>> >> * Save the amd_bsp_mpb on apply, not on load. Otherwise someone could
>> >> later load an older microcode file that would overwrite amd_bsp_mpb,
>> >> but would not be applied to the CPUs
>> >
>> > See the patch id check in apply_ucode_in_initrd()?
>> >
>> >         if (eq_id == mc->hdr.processor_rev_id && rev < mc->hdr.patch_id)
>>
>> I meant with "load" load_microcode_amd() not the loading of the
>> microcode into the CPU.
>>
>> 1.: load microcode rev X into CPU (early or normal is not important)
>> 2.: get older microcode file that only contains rev Y with Y<X
>> 3.: trigger load_microcode_amd() with a corrupt file: This will call
>> cleanup() and empty pcache.
>
> Ok, that's actually a good catch. So I wonder: why in hell would we
> flush the pcache if some of the blobs we're loading are corrupted. So
> what?! Jacob, what were you thinking - I'd be very interested to know
> what the idea behind this was.
>
> So, just to refresh everybody: the idea of the pcache is to keep all
> patches for the current family in memory so that we can support all
> sorts of hotplug and cpu mixed stepping diddling.

Then it would probably be the best to kill free_cache() completely.
Which would mean cleanup() should also go.
Which will make unloading microcode_amd.ko impossible.
But that is probably a good idea anyway: If you unload the module
there is no way to keep pcache.

But I still have another way to kill you: free_equiv_cpu_table()
Without that table find_patch() can't work and will not return the
correct information.

And that can be triggered by:
* start of load_microcode_amd(): If you reach that function (Only
UCODE_MAGIC needs to be in the file) that table is dead.
* __load_microcode_amd(): If the file only contains the table but no
patches ("invalid type field in container file section header\n")

>> 4.: trigger load_microcode_amd() with the older file:
>>  * this will now load rev Y into pcache
>>  * rev Y will be returned by find_patch and copied into amd_bsp_mpb
>>  * any try to apply rev Y will be skipped in apply_microcode_amd()
>>
>> So now the CPU still correctly has rev X, but amd_bsp_mpb will contain
>> the wrong rev Y.
>
> Right, so this shouldn't happen - what should happen is, pcache would
> hold both X and Y and find_patch would automatically give you the right
> one.
>
> And this is guaranteed since we keep the patches in a sorted linked list
> by ->patch_id which is guaranteed to be increasing.
>
> So actually load_microcode_amd() shouldn't be doing cleanup() but simply
> return ret upwards.

But it already called free_equiv_cpu_table() and so pcache is inaccessible.

And I don't think just preserving equiv_cpu_table for restoring in the
error case will be the right solution: If the new firmware file
contains a new table with fewer entries (or different entries!) some
of the patches in pcache might become inaccessible.

>> That copying already in load_microcode also is suspicious if someone
>> would only load the microcode but not apply it. But I did not find
>> a codepath in arch/x86/kernel/microcode_core.c to load it without a
>> followup apply.
>
> Yeah, we always load and apply.
>
> So now back to the original problem - load_microcode_amd() shouldn't
> clear the pcache and, in that case, a subsequent find_patch() would
> always give the right patch.

Not if equiv_cpu_table got mangled.
So should install_equiv_cpu_table() be turned into
add_to_equiv_cpu_table() or should pcache save all cpu_sig with each
patch, so that find_patch() no longer needs equiv_cpu_table?
I suspect saving that in struct ucode_patch might be better, to
prevent changes in equiv_id <-> cpu_sig mapping to make a patch
inaccessible.

Torsten
--
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/

Powered by blists - more mailing lists