arch/x86/kernel/cpu/microcode/intel: don't store initrd's start

PATCH In review Nicolai Stange 2 participants 14 messages Jul 24, 2016 – Jul 25, 2016

Subsystems (2)

x86 x86 microcode update support

lore

Share & tools

lore

Nicolai Stange

author

2016-07-24 15:06 UTC #0 lore raw

PATCH 1/1 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 <[email protected]>

---
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/[email protected] ).

 arch/x86/kernel/cpu/microcode/intel.c | 65 ++++++++++++++++++++++++-----------
 1 file changed, 44 insertions(+), 21 deletions(-)

arch/x86/kernel/cpu/microcode/intel.c +44 ~~-21~~

index 6515c80..3e88077 100644
@@ -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;

Signature · 1 line

2.9.2

Borislav Petkov

maintainer

2016-07-25 07:07 UTC #1 lore raw

Quoted 2 lines · Nicolai Stange

On x86_64 with CONFIG_RANDOMIZE_MEMORY and CONFIG_MICROCODE_INTEL, I get the following splat upon booting on an Intel i7-4800MQ:

Before we dive into this, let's establish which tree you're exactly testing...

Quoted 21 lines

  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!

Quoted 1 line

It can be bisected to commit 21ef9a5c3164 ("Merge branch 'x86/microcode'").

This commit 21ef9a5c3164 does not exist - the current merge commit corresponding to this is

commit 2a2a745e2d248498a21ba3876d2481e90c7fe0a5
Merge: caaaa222620c eb06158ee145
Author: Ingo Molnar <[email protected]>
Date: Sun Jul 24 08:26:41 2016 +0200

Merge branch 'x86/microcode'

Quoted 2 lines

Both of its parents, i.e.
commit f5846c92b0a5 ("Merge branch 'x86/headers'")

This commit does not exist either.

Quoted 2 lines

and
commit eb06158ee145 ("x86/microcode: Remove unused symbol exports")

This one does.

Quoted 7 lines

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").

Those two too.

So it seems you're either testing an older tip/master or some linux-next.

So let's clarify what exactly you're testing before we do anything else.

Thanks.

Signature · 5 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Nicolai Stange

author

2016-07-25 09:24 UTC #2 lore raw

Quoted 6 lines · Borislav Petkov

Quoted 2 lines · Nicolai Stange
On x86_64 with CONFIG_RANDOMIZE_MEMORY and CONFIG_MICROCODE_INTEL, I get the following splat upon booting on an Intel i7-4800MQ:
Before we dive into this, let's establish which tree you're exactly testing...

I tested on linux-next-20160722 (I wrote this below the '---' marker).

Quoted 52 lines

Quoted 22 lines
  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!
Quoted 1 line
It can be bisected to commit 21ef9a5c3164 ("Merge branch 'x86/microcode'").
This commit 21ef9a5c3164 does not exist - the current merge commit corresponding to this is
commit 2a2a745e2d248498a21ba3876d2481e90c7fe0a5
Merge: caaaa222620c eb06158ee145
Author: Ingo Molnar <[email protected]>
Date: Sun Jul 24 08:26:41 2016 +0200
Merge branch 'x86/microcode'
Quoted 2 lines
Both of its parents, i.e.
commit f5846c92b0a5 ("Merge branch 'x86/headers'")
This commit does not exist either.
Quoted 2 lines
and
commit eb06158ee145 ("x86/microcode: Remove unused symbol exports")
This one does.
Quoted 7 lines
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").

Quoted 3 lines

Those two too.
So it seems you're either testing an older tip/master or some linux-next.

See above.

Quoted 1 line

So let's clarify what exactly you're testing before we do anything else.

If you want me to try any particular tree, please just tell me which one.

Thanks,

Nicolai

Borislav Petkov

maintainer

2016-07-25 12:36 UTC #3 lore raw

Quoted 1 line · Nicolai Stange

I tested on linux-next-20160722 (I wrote this below the '---' marker).

Ok, thanks.

So let's try something simpler first. That works in my kvm guest here, can you test it on your box too please?

---

arch/x86/kernel/cpu/microcode/intel.c +8 -2

index 6515c802346a..b98e016ba5fc 100644
@@ -793,10 +793,10 @@ void __init load_ucode_intel_bsp(void)
 void load_ucode_intel_ap(void)
 {
 	struct ucode_blobs *blobs_p;
+	unsigned long *ptrs, start;
 	struct mc_saved_data *mcs;
 	struct ucode_cpu_info uci;
 	enum ucode_state ret;
-	unsigned long *ptrs;
 
 #ifdef CONFIG_X86_32
 	mcs	= (struct mc_saved_data *)__pa_nodebug(&mc_saved_data);

@@ -815,8 +815,14 @@ void load_ucode_intel_ap(void)
 	if (!mcs->num_saved)
 		return;
 
+	/*
+	 * Pay attention to CONFIG_RANDOMIZE_MEMORY as it shuffles physmem
+	 * mapping too.
+	 */
+	start = blobs_p->start + (PAGE_OFFSET - __PAGE_OFFSET_BASE);
+
 	collect_cpu_info_early(&uci);
-	ret = load_microcode(mcs, ptrs, blobs_p->start, &uci);
+	ret = load_microcode(mcs, ptrs, start, &uci);
 	if (ret != UCODE_OK)
 		return;

---

Signature · 5 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Nicolai Stange

author

2016-07-25 12:59 UTC #4 lore raw

Quoted 7 lines · Borislav Petkov

Quoted 1 line · Nicolai Stange
I tested on linux-next-20160722 (I wrote this below the '---' marker).
Ok, thanks.
So let's try something simpler first. That works in my kvm guest here, can you test it on your box too please?

Applied on top of next-20160722 and it boots (again, with the additional
http://lkml.kernel.org/g/[email protected]
that I need on by box). But please see below.

Quoted 8 lines

---

arch/x86/kernel/cpu/microcode/intel.c +0 -0

index 6515c802346a..b98e016ba5fc 100644
@@ -793,10 +793,10 @@ void __init load_ucode_intel_bsp(void)
 void load_ucode_intel_ap(void)

arch/x86/kernel/cpu/microcode/intel.c void __init load_ucode_intel_bsp(void) · line 802

Quoted 10 lines

arch/x86/kernel/cpu/microcode/intel.c +1 -1

 {
 	struct ucode_blobs *blobs_p;
+	unsigned long *ptrs, start;
 	struct mc_saved_data *mcs;
 	struct ucode_cpu_info uci;
 	enum ucode_state ret;
-	unsigned long *ptrs;
 
 #ifdef CONFIG_X86_32
 	mcs	= (struct mc_saved_data *)__pa_nodebug(&mc_saved_data);

Am I overlooking something or is this an unrelated cleanup?

Quoted 6 lines

arch/x86/kernel/cpu/microcode/intel.c +2 -0

@@ -815,8 +815,14 @@ void load_ucode_intel_ap(void)
 	if (!mcs->num_saved)
 		return;
 
+	/*
+	 * Pay attention to CONFIG_RANDOMIZE_MEMORY as it shuffles physmem

arch/x86/kernel/cpu/microcode/intel.c void load_ucode_intel_ap(void) · line 828

Quoted 10 lines

arch/x86/kernel/cpu/microcode/intel.c +5 -1

+	 * mapping too.
+	 */
+	start = blobs_p->start + (PAGE_OFFSET - __PAGE_OFFSET_BASE);
+
 	collect_cpu_info_early(&uci);
-	ret = load_microcode(mcs, ptrs, blobs_p->start, &uci);
+	ret = load_microcode(mcs, ptrs, start, &uci);
 	if (ret != UCODE_OK)
 		return;

Doesn't this break the builtin-ucode case (!blobs.valid) where blobs.start is supposed to be zero?

Thanks,

Nicolai

Nicolai Stange

author

2016-07-25 13:06 UTC #5 lore raw

Quoted 17 lines · Nicolai Stange

Quoted 15 lines · Borislav Petkov

---

arch/x86/kernel/cpu/microcode/intel.c +1 -1

index 6515c802346a..b98e016ba5fc 100644
@@ -793,10 +793,10 @@ void __init load_ucode_intel_bsp(void)
 void load_ucode_intel_ap(void)
 {
 	struct ucode_blobs *blobs_p;
+	unsigned long *ptrs, start;
 	struct mc_saved_data *mcs;
 	struct ucode_cpu_info uci;
 	enum ucode_state ret;
-	unsigned long *ptrs;

Quoted 4 lines

arch/x86/kernel/cpu/microcode/intel.c void __init load_ucode_intel_bsp(void) · line 802
Quoted 2 lines
arch/x86/kernel/cpu/microcode/intel.c +0 -0
 #ifdef CONFIG_X86_32
 	mcs	= (struct mc_saved_data *)__pa_nodebug(&mc_saved_data);
Am I overlooking something or is this an unrelated cleanup?

I did, nvm.

Borislav Petkov

maintainer

2016-07-25 13:45 UTC #6 lore raw

In reply to Nicolai Stange

Quoted 1 line · Nicolai Stange

Applied on top of next-20160722 and it boots

Does it apply the microcode too? Or your box doesn't need microcode?

Quoted 2 lines

Doesn't this break the builtin-ucode case (!blobs.valid) where blobs.start is supposed to be zero?

Good point.

Well, it shouldn't because in the builtin case start should simply contain the ASLR offset the physmem mapping was moved to and this offset is exactly where the builtin images should be... Let me poke at it to see whether I'm seeing things correctly.

Thanks.

Signature · 5 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Nicolai Stange

author

2016-07-25 14:17 UTC #7 lore raw

Quoted 4 lines · Borislav Petkov

Quoted 1 line · Nicolai Stange
Applied on top of next-20160722 and it boots
Does it apply the microcode too? Or your box doesn't need microcode?

[    0.000000] microcode: microcode updated early to revision 0x20, date = 2016-03-16
[    2.929972] microcode: sig=0x306c3, pf=0x10, revision=0x20

Is this a "yes"?

/proc/cpuinfo shows a "microcode" value of 0x20 for all logical cores at least.

Quoted 9 lines

Quoted 2 lines
Doesn't this break the builtin-ucode case (!blobs.valid) where blobs.start is supposed to be zero?
Good point.
Well, it shouldn't because in the builtin case start should simply contain the ASLR offset the physmem mapping was moved to and this offset is exactly where the builtin images should be... Let me poke at it to see whether I'm seeing things correctly.

Hmm. From what I've seen, I've concluded that the builtin images' addresses are __va ones (assuming x86_64). I might be wrong though.

Another point: does PAGE_OFFSET_BASE exist on ARCH=i386?

And a third one, more of a sidenote: I've seen a comment somewhere that blobs.start is set to 0 in the builtin case. However, I was unable to find this initialization in the original code. This very last point would have been fixed by my patch as a sideeffect. I just forgot about this remark.

Thanks,

Nicolai

Nicolai Stange

author

2016-07-25 14:28 UTC #8 lore raw

Quoted 8 lines · Nicolai Stange

Quoted 4 lines · Borislav Petkov
Well, it shouldn't because in the builtin case start should simply contain the ASLR offset the physmem mapping was moved to and this offset is exactly where the builtin images should be... Let me poke at it to see whether I'm seeing things correctly.
Hmm. From what I've seen, I've concluded that the builtin images' addresses are __va ones (assuming x86_64). I might be wrong though.

Clarification: "__va ones" that don't point into the physmem mapping but somewhere else.

Borislav Petkov

maintainer

2016-07-25 16:41 UTC #9 lore raw

Quoted 2 lines · Nicolai Stange

Clarification: "__va ones" that don't point into the physmem mapping but somewhere else.

That would be the kernel text mapping. See Documentation/x86/x86_64/mm.txt for more info.

Signature · 5 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Borislav Petkov

maintainer

2016-07-25 15:07 UTC #10 lore raw

In reply to Nicolai Stange

Quoted 3 lines · Nicolai Stange

And a third one, more of a sidenote: I've seen a comment somewhere that blobs.start is set to 0 in the builtin case. However, I was unable to find this initialization in the original code.

Hint: struct ucode_blobs blobs is declared static.

Signature · 5 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Borislav Petkov

maintainer

2016-07-25 16:46 UTC #11 lore raw

In reply to Nicolai Stange

Quoted 4 lines · Nicolai Stange

[    0.000000] microcode: microcode updated early to revision 0x20, date = 2016-03-16
[    2.929972] microcode: sig=0x306c3, pf=0x10, revision=0x20

Is this a "yes"?

Yap, the "updated early" line says your microcode got updated to rev 0x20 from what was there before.

Quoted 2 lines

/proc/cpuinfo shows a "microcode" value of 0x20 for all logical cores at least.

Which is as it should be.

Quoted 1 line

Another point: does PAGE_OFFSET_BASE exist on ARCH=i386?

Yeah, we should tie this to CONFIG_RANDOMIZE_MEMORY. IOW, here's another version, I'll do some more hammering on it tomorrow.

It should take care of the builtin case too as there we have the microcode in the kernel text mapping which is already relocated when we go search for microcode blobs so no need for adjusting start then.

---

arch/x86/kernel/cpu/microcode/intel.c +14 -2

index 6515c802346a..c5a7d74a9fa6 100644
@@ -793,10 +793,10 @@ void __init load_ucode_intel_bsp(void)
 void load_ucode_intel_ap(void)
 {
 	struct ucode_blobs *blobs_p;
+	unsigned long *ptrs, start = 0;
 	struct mc_saved_data *mcs;
 	struct ucode_cpu_info uci;
 	enum ucode_state ret;
-	unsigned long *ptrs;
 
 #ifdef CONFIG_X86_32
 	mcs	= (struct mc_saved_data *)__pa_nodebug(&mc_saved_data);

@@ -815,8 +815,20 @@ void load_ucode_intel_ap(void)
 	if (!mcs->num_saved)
 		return;
 
+	if (blobs_p->valid) {
+		start = blobs_p->start;
+
+#ifdef CONFIG_RANDOMIZE_MEMORY
+		/*
+		 * Pay attention to CONFIG_RANDOMIZE_MEMORY as it shuffles
+		 * physmem mapping too and there we have the initrd.
+		 */
+		start += (PAGE_OFFSET - __PAGE_OFFSET_BASE);
+#endif
+	}
+
 	collect_cpu_info_early(&uci);
-	ret = load_microcode(mcs, ptrs, blobs_p->start, &uci);
+	ret = load_microcode(mcs, ptrs, start, &uci);
 	if (ret != UCODE_OK)
 		return;

Signature · 5 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Nicolai Stange

author

2016-07-25 17:44 UTC #12 lore raw

Quoted 2 lines · Borislav Petkov

Yeah, we should tie this to CONFIG_RANDOMIZE_MEMORY. IOW, here's another version, I'll do some more hammering on it tomorrow.

Boots fine and updates microcode (as always, on top of next-20160722).

Quoted 3 lines

It should take care of the builtin case too as there we have the microcode in the kernel text mapping which is already relocated when we go search for microcode blobs so no need for adjusting start then.

Now that you've got basically the same #ifdefery and if(->valid)'s in your final result as I did, may I ask whether you would still consider it as being simpler? In particular, what's the point of having that ->start if it's redundant and has to be corrected in a more or less hackish way anyway?

Thanks,

Nicolai

Borislav Petkov

maintainer

2016-07-25 18:19 UTC #13 lore raw

Quoted 1 line · Nicolai Stange

Boots fine and updates microcode (as always, on top of next-20160722).

Thanks!

Quoted 1 line

Now that you've got basically the same #ifdefery

Are you saying one #ifdef in my version is the same as at least 7 #if* lines in yours?

Jeez.

Quoted 1 line

and if(->valid)'s in your final result as I did,

Your get_ucode_offset() thing is called three times. That function is also ugly as it gets a bool just to test it and return early if it is false.

I have the valid test only once for the builtin case.

Quoted 1 line

may I ask whether you would still consider it as being simpler?

You're joking, right?

Your diffstat:

1 file changed, 44 insertions(+), 21 deletions(-)

My diffstat:

1 file changed, 14 insertions(+), 2 deletions(-)

Along with the reasons above.

Quoted 3 lines

In particular, what's the point of having that ->start if it's redundant and has to be corrected in a more or less hackish way anyway?

You must be kidding right? Yours returns start too. Let me break it to
ya: we need start.

And now let me break to you the ultimate reason why: the less intrusive/big/complex the patch, the less problems I have. Because next time the microcode loader breaks, it won't be *you* picking up the pieces.

Got that?

Signature · 5 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--