linux-kernel - Re: [PATCH 07/13] x86/microcode/intel: Rename update_match

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: <20150410115455.GA23346@chrystal.uk.oracle.com>
Date:	Fri, 10 Apr 2015 13:54:56 +0200
From:	Quentin Casasnovas <quentin.casasnovas@...cle.com>
To:	Borislav Petkov <bp@...en8.de>
Cc:	Quentin Casasnovas <quentin.casasnovas@...cle.com>,
	X86 ML <x86@...nel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 07/13] x86/microcode/intel: Rename update_match_revision()

On Fri, Apr 10, 2015 at 01:12:18PM +0200, Borislav Petkov wrote:
> On Tue, Feb 24, 2015 at 05:23:18PM +0100, Quentin Casasnovas wrote:
> > Minor nit-pick, if you reverse your inequality, you don't need for the
> > ternary operator.
> 
> Yeah, so I started looking at that and it seems the rabbit hole goes
> deeper.
> 
> Let's look at the call to revision_is_newer() in _save_mc():
> 
> save_mc:
> 
> 	new_rev      = mc_hdr->rev;
> 
> 	...
> 
> 	if (!revision_is_newer(mc_hdr, new_rev))
> 	->

Ha good catch!

BTW, I could not find that the 'rev' argument to get_matching_sig() was
used at all..

> 
> ---
> From: Borislav Petkov <bp@...e.de>
> Date: Fri, 10 Apr 2015 12:50:57 +0200
> Subject: [PATCH] x86/microcode/intel: Get rid of revision_is_newer()
>
> ...
> 
> diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
> index cd47a510a3f1..63b0a2e059ee 100644
> --- a/arch/x86/kernel/cpu/microcode/intel_lib.c
> +++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
> @@ -154,13 +154,13 @@ int get_matching_sig(unsigned int csig, int cpf, int rev, void *mc)
>  /*
>   * Returns 1 if update has been found, 0 otherwise.
>   */
> -int get_matching_microcode(unsigned int csig, int cpf, int rev, void *mc)
> +int get_matching_microcode(unsigned int csig, int cpf, int new_rev, void *mc)

If we're to rename 'rev', maybe calling it 'cpu_rev' would make it more
obvious where this variable is coming from?

>  {
>  	struct microcode_header_intel *mc_hdr = mc;
>  
> -	if (!revision_is_newer(mc_hdr, rev))
> +	if (mc_hdr->rev <= new_rev)
>  		return 0;
>  
> -	return get_matching_sig(csig, cpf, rev, mc);
> +	return get_matching_sig(csig, cpf, new_rev, mc);
>  }
>  EXPORT_SYMBOL_GPL(get_matching_microcode);

Anyway you patch looks good to me!

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