| 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
| ||
|
Message-Id: <20160724150549.2833-1-nicstange@gmail.com>
Date: Sun, 24 Jul 2016 17:05:49 +0200
From: Nicolai Stange <nicstange@...il.com>
To: Borislav Petkov <bp@...en8.de>
Cc: Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>,
"H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
linux-kernel@...r.kernel.org, Nicolai Stange <nicstange@...il.com>
Subject: [PATCH] arch/x86/kernel/cpu/microcode/intel: don't store initrd's start
On x86_64 with CONFIG_RANDOMIZE_MEMORY and CONFIG_MICROCODE_INTEL,
I get the following splat upon booting on an Intel i7-4800MQ:
Call Trace:
[<ffffffffb6054cea>] ? find_microcode_patch+0x4a/0xa0
[<ffffffffb6055487>] load_microcode.isra.1.constprop.12+0x37/0xa0
[...]
[<ffffffffb60557cd>] load_ucode_intel_ap+0x5d/0x80
[<ffffffffb6054924>] load_ucode_ap+0x94/0xa0
[<ffffffffb60481a8>] cpu_init+0x58/0x3e0
[<ffffffffb60709bc>] ? set_pte_vaddr+0x5c/0x90
[<ffffffffb6fac06c>] trap_init+0x2b6/0x328
[<ffffffffb6fa0dba>] start_kernel+0x224/0x47f
[<ffffffffb6fa0120>] ? early_idt_handler_array+0x120/0x120
[<ffffffffb6fa02cf>] x86_64_start_reservations+0x29/0x2b
[<ffffffffb6fa041e>] x86_64_start_kernel+0x14d/0x170
[...]
RIP [<ffffffffb6055af5>] has_newer_microcode+0x5/0x20
[...]
---[ end trace b163fd3960fd46fb ]---
Kernel panic - not syncing: Attempted to kill the idle task!
---[ end Kernel panic - not syncing: Attempted to kill the idle task!
It can be bisected to commit 21ef9a5c3164 ("Merge branch 'x86/microcode'").
Both of its parents, i.e.
commit f5846c92b0a5 ("Merge branch 'x86/headers'")
and
commit eb06158ee145 ("x86/microcode: Remove unused symbol exports")
work fine by themselves.
"Cross-bisecting" between v4.7-rc6..f5846c92b0a5 and v4.7-rc6..eb06158ee145
reveals the conflicting commits:
commit 021182e52fe0 ("x86/mm: Enable KASLR for physical mapping memory
regions")
and
commit 6c5456474e7f ("x86/microcode: Fix loading precedence").
Thus, the above crash can be explained as follows:
1. The __scan_microcode_initrd() invoked from the early
load_ucode_intel_bsp() sets the static blobs.start to the __va of the
initrd's start.
2. kernel_randomize_memory() called from setup_arch() renders the
just set blobs.start meaningless.
3. load_ucode_ap(), invoked indirectly through trap_init(), adds the now
meaningless blobs.start back to ucode offsets in order to get their
addresses.
Note that blobs.start is redundant in the sense that blobs.valid tells us
whether offsets are taken w.r.t to zero or to the initrd's start address.
Thus:
- Get rid of blobs.start.
- Calculate the offset on the fly as needed. Introduce the helper
get_ucode_offset() to facilitate this. In the case of ucode blobs
taken from the initrd, PAGE_OFFSET is added. This takes into account
any KASLR randomization of the physical memory map.
Signed-off-by: Nicolai Stange <nicstange@...il.com>
---
A possible ollow up patch might be to turn the blobs struct into a
'static bool ucode_is_from_initrd'?
Applicable to next-20160722.
On i386, this is compile-tested only.
For x86_64, it boots fine (together with the patch from
http://lkml.kernel.org/g/tip-530dd8d4b9daf77e3e5d145a26210d91ced954c7@git.kernel.org ).
arch/x86/kernel/cpu/microcode/intel.c | 65 ++++++++++++++++++++++++-----------
1 file changed, 44 insertions(+), 21 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 6515c80..3e88077 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -57,10 +57,45 @@ static struct mc_saved_data {
/* Microcode blobs within the initrd. 0 if builtin. */
static struct ucode_blobs {
- unsigned long start;
bool valid;
} blobs;
+#ifdef CONFIG_X86_32
+static unsigned long get_ucode_offset(bool is_from_initrd)
+{
+#ifdef CONFIG_BLK_DEV_INITRD
+ struct boot_params *params;
+
+ if (!is_from_initrd)
+ return 0;
+
+ /* We cannot use initrd_start because it is not set that early yet. */
+ params = (struct boot_params *)__pa_nodebug(&boot_params);
+ return params->hdr.ramdisk_image;
+#else /* CONFIG_BLK_DEV_INITRD */
+ return 0;
+#endif
+}
+#else /* CONFIG_X86_64 */
+static unsigned long get_ucode_offset(bool is_from_initrd)
+{
+#ifdef CONFIG_BLK_DEV_INITRD
+ unsigned long start;
+
+ if (!is_from_initrd)
+ return 0;
+
+ /* We cannot use initrd_start because it is not set that early yet. */
+ start = (u64)boot_params.ext_ramdisk_image << 32;
+ start |= boot_params.hdr.ramdisk_image;
+ return (unsigned long)__va(start);
+ return start + PAGE_OFFSET;
+#else /* CONFIG_BLK_DEV_INITRD */
+ return 0;
+#endif
+}
+#endif /* CONFIG_X86_32 */
+
/* Go through saved patches and find the one suitable for the current CPU. */
static enum ucode_state
find_microcode_patch(struct microcode_intel **saved,
@@ -687,36 +722,23 @@ __scan_microcode_initrd(struct cpio_data *cd, struct ucode_blobs *blbp)
static __initdata char ucode_name[] = "kernel/x86/microcode/GenuineIntel.bin";
char *p = IS_ENABLED(CONFIG_X86_32) ? (char *)__pa_nodebug(ucode_name)
: ucode_name;
-# ifdef CONFIG_X86_32
unsigned long start = 0, size;
+
+# ifdef CONFIG_X86_32
struct boot_params *params;
params = (struct boot_params *)__pa_nodebug(&boot_params);
size = params->hdr.ramdisk_size;
- /*
- * Set start only if we have an initrd image. We cannot use initrd_start
- * because it is not set that early yet.
- */
- start = (size ? params->hdr.ramdisk_image : 0);
-
# else /* CONFIG_X86_64 */
- unsigned long start = 0, size;
-
size = (u64)boot_params.ext_ramdisk_size << 32;
size |= boot_params.hdr.ramdisk_size;
-
- if (size) {
- start = (u64)boot_params.ext_ramdisk_image << 32;
- start |= boot_params.hdr.ramdisk_image;
-
- start += PAGE_OFFSET;
- }
# endif
+ /* Set start only if we have an initrd image. */
+ start = (size ? get_ucode_offset(true) : 0);
*cd = find_cpio_data(p, (void *)start, size, NULL);
if (cd->data) {
- blbp->start = start;
blbp->valid = true;
return UCODE_OK;
@@ -747,7 +769,8 @@ scan_microcode(struct mc_saved_data *mcs, unsigned long *mc_ptrs,
return ret;
}
- return get_matching_model_microcode(blbp->start, cd.data, cd.size,
+ return get_matching_model_microcode(get_ucode_offset(blbp->valid),
+ cd.data, cd.size,
mcs, mc_ptrs, uci);
}
@@ -764,7 +787,7 @@ _load_ucode_intel_bsp(struct mc_saved_data *mcs, unsigned long *mc_ptrs,
if (ret != UCODE_OK)
return;
- ret = load_microcode(mcs, mc_ptrs, blbp->start, &uci);
+ ret = load_microcode(mcs, mc_ptrs, get_ucode_offset(blbp->valid), &uci);
if (ret != UCODE_OK)
return;
@@ -816,7 +839,7 @@ void load_ucode_intel_ap(void)
return;
collect_cpu_info_early(&uci);
- ret = load_microcode(mcs, ptrs, blobs_p->start, &uci);
+ ret = load_microcode(mcs, ptrs, get_ucode_offset(blobs_p->valid), &uci);
if (ret != UCODE_OK)
return;
--
2.9.2
Powered by blists - more mailing lists