linux-kernel - Re: [PATCH]Fix early microcode loading on AMD

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: <CAPVoSvSWBmuZKAuQiOq1VS0oFqQSAwr8gUWisXEdfiYMGrZZEw@mail.gmail.com>
Date:	Tue, 23 Jul 2013 23: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 Tue, Jul 23, 2013 at 5:15 PM, Borislav Petkov <bp@...en8.de> wrote:
> On Tue, Jul 23, 2013 at 01:58:53PM +0200, Torsten Kaiser wrote:
>> Fixup the early AMD microcode loading.
>>
>> * load_microcode_amd() (and the helper its using) should not have an
>> cpu parameter.
>
> Hmm, I don't think so - we get the cpu handed down from microcode_core
> and besides the early load on 32bit needs to do find_patch(cpu).
>
>> The microcode loading is not depending on the CPU it is
>
> Mostly. There are mixed-stepping boxes which need to differentiate
> between which cpu we're applying the patch for.

I redid the patch in 5 parts, hopefully now better to understand.
Without the other changes the microcode_amd.c-part of patch 5/5 should
make it much more obvious that my change did not result in a different
behavior about which patches get loaded into the microcode cache
'pcache'.

> Btw, your config boots on my F14h box with "nomodeset" on the command
> line because it is missing radeon firmware for my gpu.
>
>> executed and all the loaded patches will end up in a global list for all
>> CPUs anyway.
>> * Return -1 (like Intels apply_microcode) when the loading fails,
>> also do not set the active microcode level on failure.
>
> Yep, this part I want. Please send it as a separate patch.

That is now patch 1/5.
Patch 2/5 is new, I skipped that part originally because I did not
want to make it even bigger...

> So I see a couple of issues in this patch and they should be separated
> into single patches - one patch taking care of one issue and explaining
> what the problem is in the commit message (I know you can do that good
> :)).

I'm still seeing some things in the microcode code that look suspicious:

Why is the X86_64 code updating uci->cpu_sig.rev, but the 32bit
version does not? And I can't see anything that reads that value.

Should apply_microcode_amd() really update uci->mc even before
checking if the microcode is newer?

The X86_32 hunk in save_microcode_in_initrd_amd() now seems obsolete.
load_microcode_amd() is no longer using find_patch() so it doesn't use
ucode_cpu_info anymore. But why is that code using
boot_cpu_data.cpu_index to find the BSP but always then passing 0 as
cpu parameter to load_microcode_amd()? If boot_cpu_data.cpu_index is
ever !=0 that code would fail.

... and collect_cpu_info_amd() also looks very weird. If csig would
not point to uci->cpu_sig then find_patch() will not be happy.
Wouldn't directly passing cpuid_eax(0x00000001) to find_patch() be a
better interface? Then the early microcode loading code would not need
to access ucode_cpu_info at all.

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/