linux-kernel - Re: [PATCH] x86/microcode: Do not access the initrd after it has been freed

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: <20170130093527.syjz7yldgndml7qb@pd.tnic>
Date:   Mon, 30 Jan 2017 10:35:27 +0100
From:   Borislav Petkov <bp@...en8.de>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     Andrey Ryabinin <aryabinin@...tuozzo.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        LKML <linux-kernel@...r.kernel.org>,
        "H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH] x86/microcode: Do not access the initrd after it has
 been freed

On Mon, Jan 30, 2017 at 09:46:32AM +0100, Ingo Molnar wrote:
> Ok, I have applied this to tip:x86/urgent.
> 
> Note that there are new conflicts with your pending work in tip:x86/microcode, and 
> I fixed them up in:
> 
>   7c5b4112040e Merge branch 'x86/urgent' into x86/microcode, to resolve conflicts
> 
> Could you please double-check my conflict resolution?

Almost, this part is wrong:

--------------------- arch/x86/kernel/cpu/microcode/amd.c ---------------------
index 7889ae492af0,079e81733a58..73082365ed1c
@@@ -268,20 -316,43 +268,20 @@@ void __load_ucode_amd(unsigned int cpui
  		use_pa	= false;
  	}
  
- 	if (!get_builtin_microcode(&cp, x86_family(cpuid_1_eax)))
 -	if (!get_builtin_microcode(&cp, family))
++	if (!get_builtin_microcode(&cp, x86_family(cpuid_1_eax)) && !initrd_gone)
  		cp = find_microcode_in_initrd(path, use_pa);

--

Btw, I did experiment with the merging because I knew it'll cause
trouble due to the urgent fix and here's what I did:

You're merging tip/x86/urgent into tip/x86/microcode so I checked out
the microcode branch and did:

$ git checkout -b tip-microcode tip/x86/microcode
$ git merge -s recursive -X ours tip/x86/urgent

This way I'm favouring our changes in the conflicting files. It merges
cleanly and the resulting diff is below.

The logic behind it is is that tip/x86/microcode does away with a bunch
of code and the urgent change touches some of that code but that's only
for 4.10.

It goes away in 4.11 and that's why we should prefer "ours" as the merge
option.

	 [ Btw, I'll send a patch for 4.11 later to make initrd_gone static as
	   it is going to be used only in microcode/core.c after the cleanup. ]

However, I still haven't figured out how to say "prefer ours but only
for specific files or subtree" because the diff has that hunk in
arch/x86/kernel/fpu/core.c too which should definitely not be "ours" as
it is a fix and there the urgent version should be the one going in.

Hmmm.

---

$ git diff HEAD~1..
diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 90b22bbdfce9..daadeeea00b1 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -139,6 +139,7 @@ extern void __init load_ucode_bsp(void);
 extern void load_ucode_ap(void);
 void reload_early_microcode(void);
 extern bool get_builtin_firmware(struct cpio_data *cd, const char *name);
+extern bool initrd_gone;
 #else
 static inline int __init microcode_init(void)			{ return 0; };
 static inline void __init load_ucode_bsp(void)			{ }
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 3b74d2f315d3..b4a4cd39b358 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -46,6 +46,8 @@
 static struct microcode_ops	*microcode_ops;
 static bool dis_ucode_ldr = true;
 
+bool initrd_gone;
+
 LIST_HEAD(microcode_cache);
 
 /*
@@ -219,11 +221,12 @@ void load_ucode_ap(void)
 static int __init save_microcode_in_initrd(void)
 {
 	struct cpuinfo_x86 *c = &boot_cpu_data;
+	int ret = -EINVAL;
 
 	switch (c->x86_vendor) {
 	case X86_VENDOR_INTEL:
 		if (c->x86 >= 6)
-			return save_microcode_in_initrd_intel();
+			ret = save_microcode_in_initrd_intel();
 		break;
 	case X86_VENDOR_AMD:
 		if (c->x86 >= 0x10)
@@ -233,7 +236,9 @@ static int __init save_microcode_in_initrd(void)
 		break;
 	}
 
-	return -EINVAL;
+	initrd_gone = true;
+
+	return ret;
 }
 
 struct cpio_data find_microcode_in_initrd(const char *path, bool use_pa)
@@ -276,9 +281,16 @@ struct cpio_data find_microcode_in_initrd(const char *path, bool use_pa)
 	 * has the virtual address of the beginning of the initrd. It also
 	 * possibly relocates the ramdisk. In either case, initrd_start contains
 	 * the updated address so use that instead.
+	 *
+	 * initrd_gone is for the hotplug case where we've thrown out initrd
+	 * already.
 	 */
-	if (!use_pa && initrd_start)
-		start = initrd_start;
+	if (!use_pa) {
+		if (initrd_gone)
+			return (struct cpio_data){ NULL, 0, "" };
+		if (initrd_start)
+			start = initrd_start;
+	}
 
 	return find_cpio_data(path, (void *)start, size, NULL);
 #else /* !CONFIG_BLK_DEV_INITRD */
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index e4e97a5355ce..de7234401275 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -9,6 +9,7 @@
 #include <asm/fpu/regset.h>
 #include <asm/fpu/signal.h>
 #include <asm/fpu/types.h>
+#include <asm/fpu/xstate.h>
 #include <asm/traps.h>
 
 #include <linux/hardirq.h>
@@ -183,7 +184,8 @@ void fpstate_init(union fpregs_state *state)
 	 * it will #GP. Make sure it is replaced after the memset().
 	 */
 	if (static_cpu_has(X86_FEATURE_XSAVES))
-		state->xsave.header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT;
+		state->xsave.header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT |
+					       xfeatures_mask;
 
 	if (static_cpu_has(X86_FEATURE_FXSR))
 		fpstate_init_fxstate(&state->fxsave);

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.