linux-kernel - Re: [PATCH 11/13] x86/microcode/AMD: Unify load_ucode_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: <alpine.DEB.2.20.1701172145470.3645@nanos>
Date:   Tue, 17 Jan 2017 21:58:00 +0100 (CET)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Borislav Petkov <bp@...en8.de>
cc:     X86 ML <x86@...nel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 11/13] x86/microcode/AMD: Unify load_ucode_amd_ap()

On Tue, 17 Jan 2017, Borislav Petkov wrote:
>  void load_ucode_amd_ap(unsigned int cpuid_1_eax)
>  {
>  	struct equiv_cpu_entry *eq;
>  	struct microcode_amd *mc;
> +	struct cont_desc *desc;
>  	u16 eq_id;
>  
> +	if (IS_ENABLED(CONFIG_X86_32)) {
> +		mc   = (struct microcode_amd *)__pa_nodebug(amd_ucode_patch);
> +		desc = (struct cont_desc *)__pa_nodebug(&cont);
> +	} else {
> +		mc   = (struct microcode_amd *)amd_ucode_patch;
> +		desc = &cont;

Bah! Now I realize that 'cont' is not a local variable as I assumed when
looking at the other patch. 'cont' is a pretty bad name for a (file) global
variable. Can we please use a more obvious name ?

While at it please make that thing static as there cant be a user outside
of that file.

> +	}
> +
>  	/* First AP hasn't cached it yet, go through the blob. */
> -	if (!cont.data) {
> -		struct cpio_data cp;
> +	if (!desc->data) {
> +		struct cpio_data cp = { };
>  
> -		if (cont.size == -1)
> +		if (desc->size == -1)
>  			return;

I'm not really fond of abusing the size member for this. And that '-1'
seems to have different meanings depending on other members. Really not
intuitive.

Please introduce a proper state member which tells what the descriptor
struct contains, i.e. EMPTY, VALID, INVALID ....

Thanks,

	tglx