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

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))
	->
	if (!((mc_hdr->rev <= new_rev) ? 0 : 1))
	->
	if (!((mc_hdr->rev <= mc_hdr->rev) ? 0 : 1))
	->
	if (!0)
	->
	if (1)
		continue;

So basically @new_rev was wrong to use there in the first place. And it
is there since it got committed in 3.13. If anything, it should've been
old_rev FAIK, or

	if (!revision_is_newer(mc_saved_hdr, new_rev))

... whateva...

And to confirm this and so I can stop rubbing my eyes, let's look at the
asm:

 *
 * Returns: The updated number @num_saved of saved microcode patches.
 */
static unsigned int _save_mc(struct microcode_intel **mc_saved,
                             u8 *ucode_ptr, unsigned int num_saved)
{
ffffffff81033f25:       4c 89 65 e0             mov    %r12,-0x20(%rbp)
ffffffff81033f29:       4c 89 6d e8             mov    %r13,-0x18(%rbp)
ffffffff81033f2d:       49 89 f4                mov    %rsi,%r12


ucode_ptr lands in %r12

...

		new_rev	     = mc_hdr->rev;
ffffffff81033f4f:	45 8b 74 24 04       	mov    0x4(%r12),%r14d


new_rev is the second unsigned int in the struct thus new_rev = %r14d = *(%r12 + 4)

...

		if (!revision_is_newer(mc_hdr, new_rev))
ffffffff81033f70:	45 3b 74 24 04       	cmp    0x4(%r12),%r14d
ffffffff81033f75:	73 21                	jae    ffffffff81033f98 <_save_mc+0x88>


So we practically end up doing

	cmpl 0x4(%r12), 0x4(%r12)

and gcc doesn't optimize it away even.


Oh well, let's kill this function completely:


---
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()

It is a one-liner for checking microcode header revisions. On top of
that, it can be used wrong as it was the case in _save_mc(). Get rid of
it.

Signed-off-by: Borislav Petkov <bp@...e.de>
---
 arch/x86/include/asm/microcode_intel.h      | 6 ------
 arch/x86/kernel/cpu/microcode/intel_early.c | 2 +-
 arch/x86/kernel/cpu/microcode/intel_lib.c   | 6 +++---
 3 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index 2b9209c46ca9..a4df6d292228 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -60,12 +60,6 @@ extern int get_matching_microcode(unsigned int csig, int cpf, int rev, void *mc)
 extern int microcode_sanity_check(void *mc, int print_err);
 extern int get_matching_sig(unsigned int csig, int cpf, int rev, void *mc);
 
-static inline int
-revision_is_newer(struct microcode_header_intel *mc_header, int rev)
-{
-	return (mc_header->rev <= rev) ? 0 : 1;
-}
-
 #ifdef CONFIG_MICROCODE_INTEL_EARLY
 extern void __init load_ucode_intel_bsp(void);
 extern void load_ucode_intel_ap(void);
diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index 98d320c25dff..edae46ebdf32 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -262,7 +262,7 @@ static unsigned int _save_mc(struct microcode_intel **mc_saved,
 
 		found = 1;
 
-		if (!revision_is_newer(mc_hdr, new_rev))
+		if (mc_hdr->rev <= mc_saved_hdr->rev)
 			continue;
 
 		/*
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)
 {
 	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);
-- 
2.3.5

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
--
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/