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

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

[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:

  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@gmail.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

Re: [PATCH] arch/x86/kernel/cpu/microcode/intel: don't store initrd's start

[Borislav Petkov]

On Sun, Jul 24, 2016 at 05:05:49PM +0200, Nicolai Stange wrote:
> 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...

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

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

commit 2a2a745e2d248498a21ba3876d2481e90c7fe0a5
Merge: caaaa222620c eb06158ee145
Author: Ingo Molnar <mingo@kernel.org>
Date:   Sun Jul 24 08:26:41 2016 +0200

    Merge branch 'x86/microcode'

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

This commit does not exist either.

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

This one does.

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

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] arch/x86/kernel/cpu/microcode/intel: don't store initrd's start

[Nicolai Stange]

Borislav Petkov <bp@alien8.de> writes:

> On Sun, Jul 24, 2016 at 05:05:49PM +0200, Nicolai Stange wrote:
>> 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).

>
>> 
>>   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'").
>
> This commit 21ef9a5c3164 does not exist - the current merge commit
> corresponding to this is
>
> commit 2a2a745e2d248498a21ba3876d2481e90c7fe0a5
> Merge: caaaa222620c eb06158ee145
> Author: Ingo Molnar <mingo@kernel.org>
> Date:   Sun Jul 24 08:26:41 2016 +0200
>
>     Merge branch 'x86/microcode'
>
>> Both of its parents, i.e.
>>   commit f5846c92b0a5 ("Merge branch 'x86/headers'")
>
> This commit does not exist either.
>
>> and
>>   commit eb06158ee145 ("x86/microcode: Remove unused symbol exports")
>
> This one does.
>
>> 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.

See above.

> 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

Re: [PATCH] arch/x86/kernel/cpu/microcode/intel: don't store initrd's start

[Borislav Petkov]

On Mon, Jul 25, 2016 at 11:24:31AM +0200, Nicolai Stange wrote:
> 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?

---
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 6515c802346a..b98e016ba5fc 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -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;
 
---

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] arch/x86/kernel/cpu/microcode/intel: don't store initrd's start

[Nicolai Stange]

Borislav Petkov <bp@alien8.de> writes:

> On Mon, Jul 25, 2016 at 11:24:31AM +0200, Nicolai Stange wrote:
>> 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/tip-530dd8d4b9daf77e3e5d145a26210d91ced954c7@git.kernel.org
that I need on by box). But please see below.


>
> ---
> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index 6515c802346a..b98e016ba5fc 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -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);

Am I overlooking something or is this an unrelated cleanup?


> @@ -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;
>

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

Thanks,

Nicolai

Re: [PATCH] arch/x86/kernel/cpu/microcode/intel: don't store initrd's start

[Nicolai Stange]

Nicolai Stange <nicstange@gmail.com> writes:

> Borislav Petkov <bp@alien8.de> writes:
>
>> ---
>> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
>> index 6515c802346a..b98e016ba5fc 100644
>> --- a/arch/x86/kernel/cpu/microcode/intel.c
>> +++ b/arch/x86/kernel/cpu/microcode/intel.c
>> @@ -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);
>
> Am I overlooking something or is this an unrelated cleanup?

I did, nvm.

Re: [PATCH] arch/x86/kernel/cpu/microcode/intel: don't store initrd's start

[Borislav Petkov]

On Mon, Jul 25, 2016 at 02:59:43PM +0200, Nicolai Stange wrote:
> Applied on top of next-20160722 and it boots

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

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

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] arch/x86/kernel/cpu/microcode/intel: don't store initrd's start

[Nicolai Stange]

Borislav Petkov <bp@alien8.de> writes:

> On Mon, Jul 25, 2016 at 02:59:43PM +0200, Nicolai Stange wrote:
>> 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.

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

Re: [PATCH] arch/x86/kernel/cpu/microcode/intel: don't store initrd's start

[Nicolai Stange]

Nicolai Stange <nicstange@gmail.com> writes:
> Borislav Petkov <bp@alien8.de> writes:
>> 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.

Re: [PATCH] arch/x86/kernel/cpu/microcode/intel: don't store initrd's start

[Borislav Petkov]

On Mon, Jul 25, 2016 at 04:16:41PM +0200, Nicolai Stange wrote:
> 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.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] arch/x86/kernel/cpu/microcode/intel: don't store initrd's start

[Borislav Petkov]

On Mon, Jul 25, 2016 at 04:27:55PM +0200, Nicolai Stange wrote:
> 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.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] arch/x86/kernel/cpu/microcode/intel: don't store initrd's start

[Borislav Petkov]

On Mon, Jul 25, 2016 at 04:16:41PM +0200, Nicolai Stange wrote:
> [    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.

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

Which is as it should be.

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

---
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 6515c802346a..c5a7d74a9fa6 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -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;
 

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] arch/x86/kernel/cpu/microcode/intel: don't store initrd's start

[Nicolai Stange]

Borislav Petkov <bp@alien8.de> writes:
> 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).


> 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

Re: [PATCH] arch/x86/kernel/cpu/microcode/intel: don't store initrd's start

[Borislav Petkov]

On Mon, Jul 25, 2016 at 07:44:10PM +0200, Nicolai Stange wrote:
> Boots fine and updates microcode (as always, on top of next-20160722).

Thanks!

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

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

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

> 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?

-- 
Regards/Gruss,
    Boris.

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