On Tue, Jan 21, 2014 at 10:39:21AM -0500, Boris Ostrovsky wrote:
> I was thinking this could have been introduced by last night's merge
> of your tree.
Ok, I need more info:
* kernel .config
* are you building an initrd with the microcode? If so, how exactly?
Which microcode blob?
* what machine, family?
* anything else I'd need to reproduce here?
Btw, why the h*ll are you still using a 32-bit kernel? :)
Thanks.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On 01/21/2014 11:14 AM, Borislav Petkov wrote:
> On Tue, Jan 21, 2014 at 10:39:21AM -0500, Boris Ostrovsky wrote:
>> I was thinking this could have been introduced by last night's merge
>> of your tree.
> Ok, I need more info:
>
> * kernel .config
Below, but probably not particularly important because...
>
> * are you building an initrd with the microcode? If so, how exactly?
> Which microcode blob?
cat $(AMD_UCODE_PATH)/* >
ucode_initrd/kernel/x86/microcode/AuthenticAMD.bin
(cd ucode_initrd;find . | cpio -o -H newc >
../$(DISTDIR)/common/ucode_initrd.cpio)
...
cat $(DISTDIR)/common/ucode_initrd.cpio \
$(DISTDIR)/common/_initramfs.cpio.gz > \
$(DISTDIR)/common/initramfs.cpio.gz
and as I just discovered, with a little twist: apparently AMD_UCODE_PATH
points to Intel microcode.
So we clearly screwed up on our end but I'd think that we shouldn't
crash when this happens. We haven't until today (and evidently we've had
this problem for a long time).
>
> * what machine, family?
All of our AMD boxes. E.g.:
CPU0: AMD E-350 Processor (fam: 14, model: 01, stepping: 00)
CPU0: Quad-Core AMD Opteron(tm) Processor 1352 (fam: 10, model: 02,
stepping: 03)
CPU0: AMD Engineering Sample (fam: 10, model: 04, stepping: 01)
CPU0: AMD Athlon(tm) II X2 245 Processor (fam: 10, model: 06, stepping: 02)
CPU0: AMD A8-3850 APU with Radeon(tm) HD Graphics (fam: 12, model: 01,
stepping: 00)
>
> * anything else I'd need to reproduce here?
>
> Btw, why the h*ll are you still using a 32-bit kernel? :)
It's part of Xen testing and 32-bit dom0 is still being used. And
because we build it we test bare-metal as well.
-boris
On Tue, Jan 21, 2014 at 12:55:53PM -0500, Boris Ostrovsky wrote:
> cat $(AMD_UCODE_PATH)/* >
> ucode_initrd/kernel/x86/microcode/AuthenticAMD.bin
> (cd ucode_initrd;find . | cpio -o -H newc >
> ../$(DISTDIR)/common/ucode_initrd.cpio)
> ...
> cat $(DISTDIR)/common/ucode_initrd.cpio \
> $(DISTDIR)/common/_initramfs.cpio.gz > \
> $(DISTDIR)/common/initramfs.cpio.gz
>
> and as I just discovered, with a little twist: apparently
> AMD_UCODE_PATH points to Intel microcode.
LOL!
@hpa: in case you were wondering whether Intel ucode works on AMD - it
doesn't! :-)
> So we clearly screwed up on our end but I'd think that we shouldn't
> crash when this happens.
Yes, we shouldn't. I'll try to reproduce it here.
Thanks.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On 01/21/2014 01:25 PM, Borislav Petkov wrote:
> On Tue, Jan 21, 2014 at 12:55:53PM -0500, Boris Ostrovsky wrote:
>> cat $(AMD_UCODE_PATH)/* >
>> ucode_initrd/kernel/x86/microcode/AuthenticAMD.bin
>> (cd ucode_initrd;find . | cpio -o -H newc >
>> ../$(DISTDIR)/common/ucode_initrd.cpio)
>> ...
>> cat $(DISTDIR)/common/ucode_initrd.cpio \
>> $(DISTDIR)/common/_initramfs.cpio.gz > \
>> $(DISTDIR)/common/initramfs.cpio.gz
>>
>> and as I just discovered, with a little twist: apparently
>> AMD_UCODE_PATH points to Intel microcode.
> LOL!
>
> @hpa: in case you were wondering whether Intel ucode works on AMD - it
> doesn't! :-)
>
>> So we clearly screwed up on our end but I'd think that we shouldn't
>> crash when this happens.
> Yes, we shouldn't. I'll try to reproduce it here.
So I tried this on a "good" system and I am pretty sure this is broken.
I don't
have any microcode in initrd and I am still dying in load_microcode_amd().
I didn't dig any further but if you can't reproduce this I can look at
what is
a NULL and why. (I suspect it's the 'container' variable).
-boris
On Thursday 23 January 2014, Boris Ostrovsky wrote:
>On 01/21/2014 01:25 PM, Borislav Petkov wrote:
>> On Tue, Jan 21, 2014 at 12:55:53PM -0500, Boris Ostrovsky wrote:
>>> cat $(AMD_UCODE_PATH)/* >
>>>
>>> ucode_initrd/kernel/x86/microcode/AuthenticAMD.bin
>>>
>>> (cd ucode_initrd;find . | cpio -o -H newc >
>>>
>>> ../$(DISTDIR)/common/ucode_initrd.cpio)
>>>
>>> ...
>>> cat $(DISTDIR)/common/ucode_initrd.cpio \
>>>
>>> $(DISTDIR)/common/_initramfs.cpio.gz > \
>>> $(DISTDIR)/common/initramfs.cpio.gz
>>>
>>> and as I just discovered, with a little twist: apparently
>>> AMD_UCODE_PATH points to Intel microcode.
>>
>> LOL!
>>
>> @hpa: in case you were wondering whether Intel ucode works on AMD - it
>> doesn't! :-)
>>
>>> So we clearly screwed up on our end but I'd think that we shouldn't
>>> crash when this happens.
>>
>> Yes, we shouldn't. I'll try to reproduce it here.
>
>So I tried this on a "good" system and I am pretty sure this is broken.
>I don't
>have any microcode in initrd and I am still dying in
>load_microcode_amd().
You can find the code on AMD's web site if you look carefully, or at least
I was able to some years ago when I built this box. There s/b an installer
that lives in /etc/init.d, and the code itself normally lives in the
/lib/firmware/amd-ucode/ directory.
If its working correctly you should see something resembling this in your
dmsg with:
gene@coyote$ grep microcode /var/log/dmesg
[ 22.572212] microcode: CPU0: patch_level=0x01000065
[ 22.573242] microcode: CPU0: new patch_level=0x01000083
[ 22.573256] microcode: CPU1: patch_level=0x01000065
[ 22.573263] microcode: CPU1: new patch_level=0x01000083
[ 22.573273] microcode: CPU2: patch_level=0x01000065
[ 22.573281] microcode: CPU2: new patch_level=0x01000083
[ 22.573293] microcode: CPU3: patch_level=0x01000065
[ 22.573301] microcode: CPU3: new patch_level=0x01000083
[ 22.573377] microcode: Microcode Update Driver: v2.00
<[email protected]>, Peter Oruba
That was from a bootup to 3.12.6, 32 bit PAE, on a quad core phenom.
Cheers, Gene
--
"There are four boxes to be used in defense of liberty:
soap, ballot, jury, and ammo. Please use in that order."
-Ed Howdershelt (Author)
Genes Web page <http://geneslinuxbox.net:6309/gene>
NOTICE: Will pay 100 USD for an HP-4815A defective but
complete probe assembly.
If you continually give you will continually have.
A pen in the hand of this president is far more
dangerous than 200 million guns in the hands of
law-abiding citizens.
On 01/23/2014 01:54 PM, Gene Heskett wrote:
> On Thursday 23 January 2014, Boris Ostrovsky wrote:
>> On 01/21/2014 01:25 PM, Borislav Petkov wrote:
>>> On Tue, Jan 21, 2014 at 12:55:53PM -0500, Boris Ostrovsky wrote:
>>>> cat $(AMD_UCODE_PATH)/* >
>>>>
>>>> ucode_initrd/kernel/x86/microcode/AuthenticAMD.bin
>>>>
>>>> (cd ucode_initrd;find . | cpio -o -H newc >
>>>>
>>>> ../$(DISTDIR)/common/ucode_initrd.cpio)
>>>>
>>>> ...
>>>> cat $(DISTDIR)/common/ucode_initrd.cpio \
>>>>
>>>> $(DISTDIR)/common/_initramfs.cpio.gz > \
>>>> $(DISTDIR)/common/initramfs.cpio.gz
>>>>
>>>> and as I just discovered, with a little twist: apparently
>>>> AMD_UCODE_PATH points to Intel microcode.
>>> LOL!
>>>
>>> @hpa: in case you were wondering whether Intel ucode works on AMD - it
>>> doesn't! :-)
>>>
>>>> So we clearly screwed up on our end but I'd think that we shouldn't
>>>> crash when this happens.
>>> Yes, we shouldn't. I'll try to reproduce it here.
>> So I tried this on a "good" system and I am pretty sure this is broken.
>> I don't
>> have any microcode in initrd and I am still dying in
>> load_microcode_amd().
> You can find the code on AMD's web site if you look carefully, or at least
> I was able to some years ago when I built this box. There s/b an installer
> that lives in /etc/init.d, and the code itself normally lives in the
> /lib/firmware/amd-ucode/ directory.
>
> If its working correctly you should see something resembling this in your
> dmsg with:
> gene@coyote$ grep microcode /var/log/dmesg
> [ 22.572212] microcode: CPU0: patch_level=0x01000065
> [ 22.573242] microcode: CPU0: new patch_level=0x01000083
> [ 22.573256] microcode: CPU1: patch_level=0x01000065
> [ 22.573263] microcode: CPU1: new patch_level=0x01000083
> [ 22.573273] microcode: CPU2: patch_level=0x01000065
> [ 22.573281] microcode: CPU2: new patch_level=0x01000083
> [ 22.573293] microcode: CPU3: patch_level=0x01000065
> [ 22.573301] microcode: CPU3: new patch_level=0x01000083
> [ 22.573377] microcode: Microcode Update Driver: v2.00
> <[email protected]>, Peter Oruba
>
> That was from a bootup to 3.12.6, 32 bit PAE, on a quad core phenom.
Right. But the (suspected) regression is from this Monday's merge
of early microcode loading code.
-boris
On Thu, Jan 23, 2014 at 01:08:46PM -0500, Boris Ostrovsky wrote:
> So I tried this on a "good" system and I am pretty sure this is
> broken. I don't have any microcode in initrd and I am still dying in
> load_microcode_amd().
32-bit still or 64-bit?
> I didn't dig any further but if you can't reproduce this I can look at
> what is a NULL and why. (I suspect it's the 'container' variable).
I will have a look soon - currently busy with other stuff.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Thursday 23 January 2014, Boris Ostrovsky wrote:
>Right. But the (suspected) regression is from this Monday's merge
>of early microcode loading code.
>
>-boris
Ahh, I missed that in the OP then. I'll get me coat.
Cheers, Gene
--
"There are four boxes to be used in defense of liberty:
soap, ballot, jury, and ammo. Please use in that order."
-Ed Howdershelt (Author)
Genes Web page <http://geneslinuxbox.net:6309/gene>
NOTICE: Will pay 100 USD for an HP-4815A defective but
complete probe assembly.
It is very difficult to prophesy, especially when it pertains to the
future.
A pen in the hand of this president is far more
dangerous than 200 million guns in the hands of
law-abiding citizens.
On 01/23/2014 02:29 PM, Borislav Petkov wrote:
> On Thu, Jan 23, 2014 at 01:08:46PM -0500, Boris Ostrovsky wrote:
>> So I tried this on a "good" system and I am pretty sure this is
>> broken. I don't have any microcode in initrd and I am still dying in
>> load_microcode_amd().
> 32-bit still or 64-bit?
32-bit only.
-boris
On Thu, Jan 23, 2014 at 02:41:59PM -0500, Boris Ostrovsky wrote:
> 32-bit only.
Ok, I think I know what happens. Can you try this one (I missed doing
this on 32-bit and 64-bit does it, which would explain why it didn't
explode there):
---
diff --git a/arch/x86/kernel/cpu/microcode/amd_early.c b/arch/x86/kernel/cpu/microcode/amd_early.c
index 8384c0fa206f..03b759401bc1 100644
--- a/arch/x86/kernel/cpu/microcode/amd_early.c
+++ b/arch/x86/kernel/cpu/microcode/amd_early.c
@@ -359,7 +359,7 @@ int __init save_microcode_in_initrd_amd(void)
pr_info("microcode: updated early to new patch_level=0x%08x\n",
ucode_new_rev);
- if (!container)
+ if (!container || !ucode_cpio.data)
return -EINVAL;
eax = cpuid_eax(0x00000001);
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On 01/28/2014 11:24 AM, Borislav Petkov wrote:
> On Thu, Jan 23, 2014 at 02:41:59PM -0500, Boris Ostrovsky wrote:
>> 32-bit only.
> Ok, I think I know what happens. Can you try this one (I missed doing
> this on 32-bit and 64-bit does it, which would explain why it didn't
> explode there):
>
> ---
> diff --git a/arch/x86/kernel/cpu/microcode/amd_early.c b/arch/x86/kernel/cpu/microcode/amd_early.c
> index 8384c0fa206f..03b759401bc1 100644
> --- a/arch/x86/kernel/cpu/microcode/amd_early.c
> +++ b/arch/x86/kernel/cpu/microcode/amd_early.c
> @@ -359,7 +359,7 @@ int __init save_microcode_in_initrd_amd(void)
> pr_info("microcode: updated early to new patch_level=0x%08x\n",
> ucode_new_rev);
>
> - if (!container)
> + if (!container || !ucode_cpio.data)
> return -EINVAL;
>
> eax = cpuid_eax(0x00000001);
It fixes the case when there is no microcode in initrd but when
microcode is corrupted (as was the case when we were pointing to Intel
binary) we still die. Neither container nor ucode_cpio.data is NULL in
this case.
-boris
On Tue, Jan 28, 2014 at 03:43:57PM -0500, Boris Ostrovsky wrote:
> It fixes the case when there is no microcode in initrd but when
> microcode is corrupted (as was the case when we were pointing to Intel
> binary) we still die. Neither container nor ucode_cpio.data is NULL in
> this case.
How is that possible? Did you generate a path in the cpio by the name of
"kernel/x86/microcode/AuthenticAMD.bin"
but with Intel microcode in it?
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On 01/28/2014 03:52 PM, Borislav Petkov wrote:
> On Tue, Jan 28, 2014 at 03:43:57PM -0500, Boris Ostrovsky wrote:
>> It fixes the case when there is no microcode in initrd but when
>> microcode is corrupted (as was the case when we were pointing to Intel
>> binary) we still die. Neither container nor ucode_cpio.data is NULL in
>> this case.
> How is that possible? Did you generate a path in the cpio by the name of
>
> "kernel/x86/microcode/AuthenticAMD.bin"
>
> but with Intel microcode in it?
>
Yes. This was a bug in our build environment. It is fixed now but I put this
bug back when I was testing your patch.
-boris
On Tue, Jan 28, 2014 at 04:05:20PM -0500, Boris Ostrovsky wrote:
> Yes. This was a bug in our build environment. It is fixed now but I
> put this bug back when I was testing your patch.
Right, that was the deal. Oh well, we'd need to verify the container
too, I guess:
---
diff --git a/arch/x86/kernel/cpu/microcode/amd_early.c b/arch/x86/kernel/cpu/microcode/amd_early.c
index 8384c0fa206f..c568b6c9af95 100644
--- a/arch/x86/kernel/cpu/microcode/amd_early.c
+++ b/arch/x86/kernel/cpu/microcode/amd_early.c
@@ -37,10 +37,12 @@ static __initdata char ucode_path[] = "kernel/x86/microcode/AuthenticAMD.bin";
static struct cpio_data __init find_ucode_in_initrd(void)
{
+ struct cpio_data ret;
long offset = 0;
char *path;
void *start;
size_t size;
+ u32 *hdr;
#ifdef CONFIG_X86_32
struct boot_params *p;
@@ -59,7 +61,18 @@ static struct cpio_data __init find_ucode_in_initrd(void)
size = boot_params.hdr.ramdisk_size;
#endif
- return find_cpio_data(path, start, size, &offset);
+ ret = find_cpio_data(path, start, size, &offset);
+
+ /* Verify we didn't get some garbage from the initrd. */
+ hdr = (u32 *)ret.data;
+
+ if (hdr[0] != UCODE_MAGIC ||
+ hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
+ hdr[2] == 0) {
+ ret.data = NULL;
+ ret.size = 0;
+ }
+ return ret;
}
static size_t compute_container_size(u8 *data, u32 total_size)
@@ -359,7 +372,7 @@ int __init save_microcode_in_initrd_amd(void)
pr_info("microcode: updated early to new patch_level=0x%08x\n",
ucode_new_rev);
- if (!container)
+ if (!container || !ucode_cpio.data)
return -EINVAL;
eax = cpuid_eax(0x00000001);
--
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On 01/28/2014 04:30 PM, Borislav Petkov wrote:
> On Tue, Jan 28, 2014 at 04:05:20PM -0500, Boris Ostrovsky wrote:
>> Yes. This was a bug in our build environment. It is fixed now but I
>> put this bug back when I was testing your patch.
> Right, that was the deal. Oh well, we'd need to verify the container
> too, I guess:
I'll give it a spin. Apparently the problem is currently in
install_equiv_cpu_table()
when we are trying to index into buf and buf (which is container)
doesn't appear
to be there.
Are you sure that
container = (u8 *)(__va((u32)relocated_ramdisk) +
((u32)container -
boot_params.hdr.ramdisk_image));
in save_microcode_in_initrd_amd() always results in a valid pointer? It
is non-NULL but it
points to address that looks to be not mapped.
-boris
>
> ---
> diff --git a/arch/x86/kernel/cpu/microcode/amd_early.c b/arch/x86/kernel/cpu/microcode/amd_early.c
> index 8384c0fa206f..c568b6c9af95 100644
> --- a/arch/x86/kernel/cpu/microcode/amd_early.c
> +++ b/arch/x86/kernel/cpu/microcode/amd_early.c
> @@ -37,10 +37,12 @@ static __initdata char ucode_path[] = "kernel/x86/microcode/AuthenticAMD.bin";
>
> static struct cpio_data __init find_ucode_in_initrd(void)
> {
> + struct cpio_data ret;
> long offset = 0;
> char *path;
> void *start;
> size_t size;
> + u32 *hdr;
>
> #ifdef CONFIG_X86_32
> struct boot_params *p;
> @@ -59,7 +61,18 @@ static struct cpio_data __init find_ucode_in_initrd(void)
> size = boot_params.hdr.ramdisk_size;
> #endif
>
> - return find_cpio_data(path, start, size, &offset);
> + ret = find_cpio_data(path, start, size, &offset);
> +
> + /* Verify we didn't get some garbage from the initrd. */
> + hdr = (u32 *)ret.data;
> +
> + if (hdr[0] != UCODE_MAGIC ||
> + hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
> + hdr[2] == 0) {
> + ret.data = NULL;
> + ret.size = 0;
> + }
> + return ret;
> }
>
> static size_t compute_container_size(u8 *data, u32 total_size)
> @@ -359,7 +372,7 @@ int __init save_microcode_in_initrd_amd(void)
> pr_info("microcode: updated early to new patch_level=0x%08x\n",
> ucode_new_rev);
>
> - if (!container)
> + if (!container || !ucode_cpio.data)
> return -EINVAL;
>
> eax = cpuid_eax(0x00000001);
> --
>
On 01/28/2014 04:37 PM, Boris Ostrovsky wrote:
> On 01/28/2014 04:30 PM, Borislav Petkov wrote:
>> On Tue, Jan 28, 2014 at 04:05:20PM -0500, Boris Ostrovsky wrote:
>>> Yes. This was a bug in our build environment. It is fixed now but I
>>> put this bug back when I was testing your patch.
>> Right, that was the deal. Oh well, we'd need to verify the container
>> too, I guess:
The patch seems to have fixed the problem.
-boris
>
> I'll give it a spin. Apparently the problem is currently in
> install_equiv_cpu_table()
> when we are trying to index into buf and buf (which is container)
> doesn't appear
> to be there.
>
> Are you sure that
>
> container = (u8 *)(__va((u32)relocated_ramdisk) +
> ((u32)container -
> boot_params.hdr.ramdisk_image));
>
> in save_microcode_in_initrd_amd() always results in a valid pointer?
> It is non-NULL but it
> points to address that looks to be not mapped.
>
> -boris
>
>>
>> ---
>> diff --git a/arch/x86/kernel/cpu/microcode/amd_early.c
>> b/arch/x86/kernel/cpu/microcode/amd_early.c
>> index 8384c0fa206f..c568b6c9af95 100644
>> --- a/arch/x86/kernel/cpu/microcode/amd_early.c
>> +++ b/arch/x86/kernel/cpu/microcode/amd_early.c
>> @@ -37,10 +37,12 @@ static __initdata char ucode_path[] =
>> "kernel/x86/microcode/AuthenticAMD.bin";
>> static struct cpio_data __init find_ucode_in_initrd(void)
>> {
>> + struct cpio_data ret;
>> long offset = 0;
>> char *path;
>> void *start;
>> size_t size;
>> + u32 *hdr;
>> #ifdef CONFIG_X86_32
>> struct boot_params *p;
>> @@ -59,7 +61,18 @@ static struct cpio_data __init
>> find_ucode_in_initrd(void)
>> size = boot_params.hdr.ramdisk_size;
>> #endif
>> - return find_cpio_data(path, start, size, &offset);
>> + ret = find_cpio_data(path, start, size, &offset);
>> +
>> + /* Verify we didn't get some garbage from the initrd. */
>> + hdr = (u32 *)ret.data;
>> +
>> + if (hdr[0] != UCODE_MAGIC ||
>> + hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
>> + hdr[2] == 0) {
>> + ret.data = NULL;
>> + ret.size = 0;
>> + }
>> + return ret;
>> }
>> static size_t compute_container_size(u8 *data, u32 total_size)
>> @@ -359,7 +372,7 @@ int __init save_microcode_in_initrd_amd(void)
>> pr_info("microcode: updated early to new
>> patch_level=0x%08x\n",
>> ucode_new_rev);
>> - if (!container)
>> + if (!container || !ucode_cpio.data)
>> return -EINVAL;
>> eax = cpuid_eax(0x00000001);
>> --
>>
>
On Tue, Jan 28, 2014 at 06:10:59PM -0500, Boris Ostrovsky wrote:
> The patch seems to have fixed the problem.
Thanks, I'll add your Reported-and-tested-by.
>> I'll give it a spin. Apparently the problem is currently in
>> install_equiv_cpu_table() when we are trying to index into buf and
>> buf (which is container) doesn't appear to be there.
Yes.
> >Are you sure that
> >
> > container = (u8 *)(__va((u32)relocated_ramdisk) +
> > ((u32)container -
> >boot_params.hdr.ramdisk_image));
> >
> >in save_microcode_in_initrd_amd() always results in a valid
> >pointer? It is non-NULL but it
> >points to address that looks to be not mapped.
Well, we unconditionally relocate the ramdisk to direct-mapped memory,
see relocate_initrd(). I'll take a look at this tomorrow though, it is
late here.
The good thing is, I can reproduce the initial crash you reported in
qemu+kvm which makes everything very easy to play with. :)
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Wed, Jan 29, 2014 at 12:22:19AM +0100, Borislav Petkov wrote:
> > >Are you sure that
> > >
> > > container = (u8 *)(__va((u32)relocated_ramdisk) +
> > > ((u32)container -
> > >boot_params.hdr.ramdisk_image));
> > >
> > >in save_microcode_in_initrd_amd() always results in a valid
> > >pointer? It is non-NULL but it
> > >points to address that looks to be not mapped.
Well, here's what I see here:
* initrd *without* microcode:
[ 0.000000] RAMDISK: [mem 0x7fbfc000-0x7ffeffff]
[ 0.000000] Allocated new RAMDISK: [mem 0x3680a000-0x36bfd099]
[ 0.000000] Move RAMDISK from [mem 0x7fbfc000-0x7ffef099] to [mem 0x3680a000-0x36bfd099]
...
[ 0.710771] save_microcode_in_initrd_amd:
relocated_ramdisk: 0x3680a000,
__va(relocated_ramdisk): 0xf680a000,
container: 0x76c0e000,
boot_params.hdr.ramdisk_image: 0x7fbfc000
...
[ 0.717080] BUG: unable to handle kernel paging request at 76c0e008
* initrd + microcode:
[ 0.000000] RAMDISK: [mem 0x7fbf7000-0x7ffeffff]
[ 0.000000] Allocated new RAMDISK: [mem 0x36805000-0x36bfd499]
[ 0.000000] Move RAMDISK from [mem 0x7fbf7000-0x7ffef499] to [mem 0x36805000-0x36bfd499]
[ 0.781948] save_microcode_in_initrd_amd:
relocated_ramdisk: 0x36805000,
__va(relocated_ramdisk): 0xf6805000,
container: 0xf680527c,
boot_params.hdr.ramdisk_image: 0x7fbf7000
Now guess what happens arithmetically if container is 0 (first case
above without microcode):
0xf680a000 + 0 - 0x7fbfc000 = 0x76c0e000.
So the fix of looking at ucode_cpio.data is correct.
An equivalent fix would be this hunk below but the initial version I
sent you aligns more with what we do on 64-bit.
---
diff --git a/arch/x86/kernel/cpu/microcode/amd_early.c b/arch/x86/kernel/cpu/microcode/amd_early.c
index 8384c0fa206f..7199953fdbf2 100644
--- a/arch/x86/kernel/cpu/microcode/amd_early.c
+++ b/arch/x86/kernel/cpu/microcode/amd_early.c
@@ -347,6 +347,9 @@ int __init save_microcode_in_initrd_amd(void)
if (!uci->cpu_sig.sig)
smp_call_function_single(bsp, collect_cpu_sig_on_bsp, NULL, 1);
+ if (!container)
+ return -EINVAL;
+
/*
* Take into account the fact that the ramdisk might get relocated
* and therefore we need to recompute the container's position in
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On 01/30/2014 10:13 AM, Borislav Petkov wrote:
> On Wed, Jan 29, 2014 at 12:22:19AM +0100, Borislav Petkov wrote:
>>>> Are you sure that
>>>>
>>>> container = (u8 *)(__va((u32)relocated_ramdisk) +
>>>> ((u32)container -
>>>> boot_params.hdr.ramdisk_image));
>>>>
>>>> in save_microcode_in_initrd_amd() always results in a valid
>>>> pointer? It is non-NULL but it
>>>> points to address that looks to be not mapped.
> Well, here's what I see here:
>
> * initrd *without* microcode:
>
> [ 0.000000] RAMDISK: [mem 0x7fbfc000-0x7ffeffff]
> [ 0.000000] Allocated new RAMDISK: [mem 0x3680a000-0x36bfd099]
> [ 0.000000] Move RAMDISK from [mem 0x7fbfc000-0x7ffef099] to [mem 0x3680a000-0x36bfd099]
>
> ...
>
> [ 0.710771] save_microcode_in_initrd_amd:
>
> relocated_ramdisk: 0x3680a000,
> __va(relocated_ramdisk): 0xf680a000,
> container: 0x76c0e000,
> boot_params.hdr.ramdisk_image: 0x7fbfc000
>
> ...
>
> [ 0.717080] BUG: unable to handle kernel paging request at 76c0e008
>
>
> * initrd + microcode:
>
> [ 0.000000] RAMDISK: [mem 0x7fbf7000-0x7ffeffff]
> [ 0.000000] Allocated new RAMDISK: [mem 0x36805000-0x36bfd499]
> [ 0.000000] Move RAMDISK from [mem 0x7fbf7000-0x7ffef499] to [mem 0x36805000-0x36bfd499]
>
> [ 0.781948] save_microcode_in_initrd_amd:
>
> relocated_ramdisk: 0x36805000,
> __va(relocated_ramdisk): 0xf6805000,
> container: 0xf680527c,
> boot_params.hdr.ramdisk_image: 0x7fbf7000
>
> Now guess what happens arithmetically if container is 0 (first case
> above without microcode):
>
> 0xf680a000 + 0 - 0x7fbfc000 = 0x76c0e000.
>
> So the fix of looking at ucode_cpio.data is correct.
That's not sufficient though, you'll need the other piece in
find_ucode_in_initrd() where you check the header.
>
> An equivalent fix would be this hunk below but the initial version I
> sent you aligns more with what we do on 64-bit.
>
> ---
> diff --git a/arch/x86/kernel/cpu/microcode/amd_early.c b/arch/x86/kernel/cpu/microcode/amd_early.c
> index 8384c0fa206f..7199953fdbf2 100644
> --- a/arch/x86/kernel/cpu/microcode/amd_early.c
> +++ b/arch/x86/kernel/cpu/microcode/amd_early.c
> @@ -347,6 +347,9 @@ int __init save_microcode_in_initrd_amd(void)
> if (!uci->cpu_sig.sig)
> smp_call_function_single(bsp, collect_cpu_sig_on_bsp, NULL, 1);
>
> + if (!container)
> + return -EINVAL;
> +
> /*
> * Take into account the fact that the ramdisk might get relocated
> * and therefore we need to recompute the container's position in
I like this one better. You can move this check above "#ifdef
CONFIG_X86_32" and have it as the first statement in the routine, common
between 32- and 64-bit.
-boris
On Thu, Jan 30, 2014 at 02:41:09PM -0500, Boris Ostrovsky wrote:
> That's not sufficient though, you'll need the other piece in
> find_ucode_in_initrd() where you check the header.
I meant wrt to looking at ucode_cpio.data. The other piece remains.
> I like this one better. You can move this check above "#ifdef
> CONFIG_X86_32" and have it as the first statement in the routine,
> common between 32- and 64-bit.
Yeah, it is simpler. Ok, will change.
Thanks.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Thu, Jan 30, 2014 at 08:54:13PM +0100, Borislav Petkov wrote:
> Yeah, it is simpler. Ok, will change.
Ok, let's have another run at it. This one takes care of the relocated
ramdisk on 64-bit too, as we do it there also, not only on 32-bit. It
boots fine here on the 32/64-bit guests.
---
From: Borislav Petkov <[email protected]>
Subject: [PATCH] x86, microcode, AMD: Sanity-check initrd image
For additional coverage, BorisO and friends did swap AMD microcode with
Intel microcode blobs in order to see what happens. What did happen on
32-bit was
[ 5.722656] BUG: unable to handle kernel paging request at be3a6008
[ 5.722693] IP: [<c106d6b4>] load_microcode_amd+0x24/0x3f0
[ 5.722716] *pdpt = 0000000000000000 *pde = 0000000000000000
because there was a valid initrd there but without valid microcode in
it. In order to fix that, we need to check the container signature from
the initrd before we continue loading it.
Also, take care of the ramdisk relocation on both bitness.
Reported-by: Boris Ostrovsky <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/kernel/cpu/microcode/amd_early.c | 60 +++++++++++++++++++++++--------
1 file changed, 45 insertions(+), 15 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd_early.c b/arch/x86/kernel/cpu/microcode/amd_early.c
index 8384c0fa206f..41537ae7c31d 100644
--- a/arch/x86/kernel/cpu/microcode/amd_early.c
+++ b/arch/x86/kernel/cpu/microcode/amd_early.c
@@ -37,10 +37,12 @@ static __initdata char ucode_path[] = "kernel/x86/microcode/AuthenticAMD.bin";
static struct cpio_data __init find_ucode_in_initrd(void)
{
+ struct cpio_data ret;
long offset = 0;
char *path;
void *start;
size_t size;
+ u32 *hdr;
#ifdef CONFIG_X86_32
struct boot_params *p;
@@ -59,7 +61,20 @@ static struct cpio_data __init find_ucode_in_initrd(void)
size = boot_params.hdr.ramdisk_size;
#endif
- return find_cpio_data(path, start, size, &offset);
+ ret = find_cpio_data(path, start, size, &offset);
+ if (!ret.data)
+ return ret;
+
+ /* Verify we didn't get some garbage from the initrd. */
+ hdr = (u32 *)ret.data;
+
+ if (hdr[0] != UCODE_MAGIC ||
+ hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
+ hdr[2] == 0) {
+ ret.data = NULL;
+ ret.size = 0;
+ }
+ return ret;
}
static size_t compute_container_size(u8 *data, u32 total_size)
@@ -285,6 +300,15 @@ static void __init collect_cpu_sig_on_bsp(void *arg)
uci->cpu_sig.sig = cpuid_eax(0x00000001);
}
+
+static void __init get_bsp_sig(void)
+{
+ unsigned int bsp = boot_cpu_data.cpu_index;
+ struct ucode_cpu_info *uci = ucode_cpu_info + bsp;
+
+ if (!uci->cpu_sig.sig)
+ smp_call_function_single(bsp, collect_cpu_sig_on_bsp, NULL, 1);
+}
#else
void load_ucode_amd_ap(void)
{
@@ -337,31 +361,37 @@ void load_ucode_amd_ap(void)
int __init save_microcode_in_initrd_amd(void)
{
+ unsigned long cont;
enum ucode_state ret;
u32 eax;
-#ifdef CONFIG_X86_32
- unsigned int bsp = boot_cpu_data.cpu_index;
- struct ucode_cpu_info *uci = ucode_cpu_info + bsp;
-
- if (!uci->cpu_sig.sig)
- smp_call_function_single(bsp, collect_cpu_sig_on_bsp, NULL, 1);
+ if (!container)
+ return -EINVAL;
+#ifdef CONFIG_X86_32
+ get_bsp_sig();
+ cont = (unsigned long)container;
+#else
/*
- * Take into account the fact that the ramdisk might get relocated
- * and therefore we need to recompute the container's position in
- * virtual memory space.
+ * We need the physical address of the container for both bitness since
+ * boot_params.hdr.ramdisk_image is a physical address.
*/
- container = (u8 *)(__va((u32)relocated_ramdisk) +
- ((u32)container - boot_params.hdr.ramdisk_image));
+ cont = __pa(container);
#endif
+
+ /*
+ * Take into account the fact that the ramdisk might get relocated and
+ * therefore we need to recompute the container's position in virtual
+ * memory space.
+ */
+ if (relocated_ramdisk)
+ container = (u8 *)(__va(relocated_ramdisk) +
+ (cont - boot_params.hdr.ramdisk_image));
+
if (ucode_new_rev)
pr_info("microcode: updated early to new patch_level=0x%08x\n",
ucode_new_rev);
- if (!container)
- return -EINVAL;
-
eax = cpuid_eax(0x00000001);
eax = ((eax >> 8) & 0xf) + ((eax >> 20) & 0xff);
--
1.8.5.2.192.g7794a68
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On 02/03/2014 12:55 PM, Borislav Petkov wrote:
> On Thu, Jan 30, 2014 at 08:54:13PM +0100, Borislav Petkov wrote:
>> Yeah, it is simpler. Ok, will change.
> Ok, let's have another run at it. This one takes care of the relocated
> ramdisk on 64-bit too, as we do it there also, not only on 32-bit. It
> boots fine here on the 32/64-bit guests.
>
> ---
> From: Borislav Petkov <[email protected]>
> Subject: [PATCH] x86, microcode, AMD: Sanity-check initrd image
>
> For additional coverage, BorisO and friends did swap AMD microcode with
> Intel microcode blobs in order to see what happens. What did happen on
> 32-bit was
>
> [ 5.722656] BUG: unable to handle kernel paging request at be3a6008
> [ 5.722693] IP: [<c106d6b4>] load_microcode_amd+0x24/0x3f0
> [ 5.722716] *pdpt = 0000000000000000 *pde = 0000000000000000
>
> because there was a valid initrd there but without valid microcode in
> it. In order to fix that, we need to check the container signature from
> the initrd before we continue loading it.
I thought that it may be sufficient to check for !container in
save_microcode_in_initrd_amd() before performing relocation. If the
signature was wrong, we would have found out about it in
load_ucode_bsp() -> apply_ucode_in_initrd() and returned right away, from
/* find equiv cpu table */
if (header[0] != UCODE_MAGIC ||
header[1] != UCODE_EQUIV_CPU_TABLE_TYPE || /* type */
header[2] == 0) /* size */
return;
and container would have stayed zero (which I think it is. And if not,
you can set it to zero, just like you do below if you couldn't find
equivalence ID and eq_id is zero).
You only need the additional signature verification that the patch below
is adding if you check for !container after relocation, the way it was
in the original patch.
In other words, the whole fix would be moving !container check up.
(64-bit relocation is a separate issue).
No?
-boris
>
> Also, take care of the ramdisk relocation on both bitness.
>
> Reported-by: Boris Ostrovsky <[email protected]>
> Signed-off-by: Borislav Petkov <[email protected]>
> ---
> arch/x86/kernel/cpu/microcode/amd_early.c | 60 +++++++++++++++++++++++--------
> 1 file changed, 45 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/microcode/amd_early.c b/arch/x86/kernel/cpu/microcode/amd_early.c
> index 8384c0fa206f..41537ae7c31d 100644
> --- a/arch/x86/kernel/cpu/microcode/amd_early.c
> +++ b/arch/x86/kernel/cpu/microcode/amd_early.c
> @@ -37,10 +37,12 @@ static __initdata char ucode_path[] = "kernel/x86/microcode/AuthenticAMD.bin";
>
> static struct cpio_data __init find_ucode_in_initrd(void)
> {
> + struct cpio_data ret;
> long offset = 0;
> char *path;
> void *start;
> size_t size;
> + u32 *hdr;
>
> #ifdef CONFIG_X86_32
> struct boot_params *p;
> @@ -59,7 +61,20 @@ static struct cpio_data __init find_ucode_in_initrd(void)
> size = boot_params.hdr.ramdisk_size;
> #endif
>
> - return find_cpio_data(path, start, size, &offset);
> + ret = find_cpio_data(path, start, size, &offset);
> + if (!ret.data)
> + return ret;
> +
> + /* Verify we didn't get some garbage from the initrd. */
> + hdr = (u32 *)ret.data;
> +
> + if (hdr[0] != UCODE_MAGIC ||
> + hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
> + hdr[2] == 0) {
> + ret.data = NULL;
> + ret.size = 0;
> + }
> + return ret;
> }
>
> static size_t compute_container_size(u8 *data, u32 total_size)
> @@ -285,6 +300,15 @@ static void __init collect_cpu_sig_on_bsp(void *arg)
>
> uci->cpu_sig.sig = cpuid_eax(0x00000001);
> }
> +
> +static void __init get_bsp_sig(void)
> +{
> + unsigned int bsp = boot_cpu_data.cpu_index;
> + struct ucode_cpu_info *uci = ucode_cpu_info + bsp;
> +
> + if (!uci->cpu_sig.sig)
> + smp_call_function_single(bsp, collect_cpu_sig_on_bsp, NULL, 1);
> +}
> #else
> void load_ucode_amd_ap(void)
> {
> @@ -337,31 +361,37 @@ void load_ucode_amd_ap(void)
>
> int __init save_microcode_in_initrd_amd(void)
> {
> + unsigned long cont;
> enum ucode_state ret;
> u32 eax;
>
> -#ifdef CONFIG_X86_32
> - unsigned int bsp = boot_cpu_data.cpu_index;
> - struct ucode_cpu_info *uci = ucode_cpu_info + bsp;
> -
> - if (!uci->cpu_sig.sig)
> - smp_call_function_single(bsp, collect_cpu_sig_on_bsp, NULL, 1);
> + if (!container)
> + return -EINVAL;
>
> +#ifdef CONFIG_X86_32
> + get_bsp_sig();
> + cont = (unsigned long)container;
> +#else
> /*
> - * Take into account the fact that the ramdisk might get relocated
> - * and therefore we need to recompute the container's position in
> - * virtual memory space.
> + * We need the physical address of the container for both bitness since
> + * boot_params.hdr.ramdisk_image is a physical address.
> */
> - container = (u8 *)(__va((u32)relocated_ramdisk) +
> - ((u32)container - boot_params.hdr.ramdisk_image));
> + cont = __pa(container);
> #endif
> +
> + /*
> + * Take into account the fact that the ramdisk might get relocated and
> + * therefore we need to recompute the container's position in virtual
> + * memory space.
> + */
> + if (relocated_ramdisk)
> + container = (u8 *)(__va(relocated_ramdisk) +
> + (cont - boot_params.hdr.ramdisk_image));
> +
> if (ucode_new_rev)
> pr_info("microcode: updated early to new patch_level=0x%08x\n",
> ucode_new_rev);
>
> - if (!container)
> - return -EINVAL;
> -
> eax = cpuid_eax(0x00000001);
> eax = ((eax >> 8) & 0xf) + ((eax >> 20) & 0xff);
>
On Mon, Feb 03, 2014 at 02:13:27PM -0500, Boris Ostrovsky wrote:
> I thought that it may be sufficient to check for !container in
> save_microcode_in_initrd_amd() before performing relocation. If the
> signature was wrong, we would have found out about it in
> load_ucode_bsp() -> apply_ucode_in_initrd() and returned right away,
Your original test case which exploded had exactly that scenario - it
was pointing to Intel ucode so container wasn't NULL. Thus we need to
check the sig in find_ucode_in_initrd().
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On 02/03/2014 02:30 PM, Borislav Petkov wrote:
> On Mon, Feb 03, 2014 at 02:13:27PM -0500, Boris Ostrovsky wrote:
>> I thought that it may be sufficient to check for !container in
>> save_microcode_in_initrd_amd() before performing relocation. If the
>> signature was wrong, we would have found out about it in
>> load_ucode_bsp() -> apply_ucode_in_initrd() and returned right away,
> Your original test case which exploded had exactly that scenario - it
> was pointing to Intel ucode so container wasn't NULL. Thus we need to
> check the sig in find_ucode_in_initrd().
>
It exploded when 'if (!container)' check was done *after* relocation,
which made container non-zero. If you do the check *before* then I think
you will catch the fact that container is empty.
load_ucode_bsp() -> apply_ucode_in_initrd() path does not include
save_microcode_in_initrd_amd() and (if I understand the code correctly)
we already verify signature in apply_ucode_in_initrd().
I am pretty sure I tested this scenario but I can verify it again.
-boris
On Mon, Feb 03, 2014 at 02:37:34PM -0500, Boris Ostrovsky wrote:
> It exploded when 'if (!container)' check was done *after* relocation,
> which made container non-zero. If you do the check *before* then I
> think you will catch the fact that container is empty.
Ah, right, this was it.
Ok, just remove the signature check then and give it a try pls:
--
diff --git a/arch/x86/kernel/cpu/microcode/amd_early.c b/arch/x86/kernel/cpu/microcode/amd_early.c
index 41537ae7c31d..362994511be1 100644
--- a/arch/x86/kernel/cpu/microcode/amd_early.c
+++ b/arch/x86/kernel/cpu/microcode/amd_early.c
@@ -37,12 +37,10 @@ static __initdata char ucode_path[] = "kernel/x86/microcode/AuthenticAMD.bin";
static struct cpio_data __init find_ucode_in_initrd(void)
{
- struct cpio_data ret;
long offset = 0;
char *path;
void *start;
size_t size;
- u32 *hdr;
#ifdef CONFIG_X86_32
struct boot_params *p;
@@ -60,21 +58,7 @@ static struct cpio_data __init find_ucode_in_initrd(void)
start = (void *)(boot_params.hdr.ramdisk_image + PAGE_OFFSET);
size = boot_params.hdr.ramdisk_size;
#endif
-
- ret = find_cpio_data(path, start, size, &offset);
- if (!ret.data)
- return ret;
-
- /* Verify we didn't get some garbage from the initrd. */
- hdr = (u32 *)ret.data;
-
- if (hdr[0] != UCODE_MAGIC ||
- hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
- hdr[2] == 0) {
- ret.data = NULL;
- ret.size = 0;
- }
- return ret;
+ return find_cpio_data(path, start, size, &offset);
}
static size_t compute_container_size(u8 *data, u32 total_size)
--
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On 02/03/2014 02:52 PM, Borislav Petkov wrote:
> On Mon, Feb 03, 2014 at 02:37:34PM -0500, Boris Ostrovsky wrote:
>> It exploded when 'if (!container)' check was done *after* relocation,
>> which made container non-zero. If you do the check *before* then I
>> think you will catch the fact that container is empty.
> Ah, right, this was it.
>
> Ok, just remove the signature check then and give it a try pls:
Yes, this fixes the bug. And it loads microcode for a good binary.
For these two patches:
Tested-by: Boris Ostrovsky <[email protected]>
(You might want to rename the final patch to something else since you
are not checking the blob anymore).
Thanks.
-boris
>
> --
> diff --git a/arch/x86/kernel/cpu/microcode/amd_early.c b/arch/x86/kernel/cpu/microcode/amd_early.c
> index 41537ae7c31d..362994511be1 100644
> --- a/arch/x86/kernel/cpu/microcode/amd_early.c
> +++ b/arch/x86/kernel/cpu/microcode/amd_early.c
> @@ -37,12 +37,10 @@ static __initdata char ucode_path[] = "kernel/x86/microcode/AuthenticAMD.bin";
>
> static struct cpio_data __init find_ucode_in_initrd(void)
> {
> - struct cpio_data ret;
> long offset = 0;
> char *path;
> void *start;
> size_t size;
> - u32 *hdr;
>
> #ifdef CONFIG_X86_32
> struct boot_params *p;
> @@ -60,21 +58,7 @@ static struct cpio_data __init find_ucode_in_initrd(void)
> start = (void *)(boot_params.hdr.ramdisk_image + PAGE_OFFSET);
> size = boot_params.hdr.ramdisk_size;
> #endif
> -
> - ret = find_cpio_data(path, start, size, &offset);
> - if (!ret.data)
> - return ret;
> -
> - /* Verify we didn't get some garbage from the initrd. */
> - hdr = (u32 *)ret.data;
> -
> - if (hdr[0] != UCODE_MAGIC ||
> - hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
> - hdr[2] == 0) {
> - ret.data = NULL;
> - ret.size = 0;
> - }
> - return ret;
> + return find_cpio_data(path, start, size, &offset);
> }
>
> static size_t compute_container_size(u8 *data, u32 total_size)
> --
>
On Mon, Feb 03, 2014 at 03:28:51PM -0500, Boris Ostrovsky wrote:
> On 02/03/2014 02:52 PM, Borislav Petkov wrote:
> >On Mon, Feb 03, 2014 at 02:37:34PM -0500, Boris Ostrovsky wrote:
> >>It exploded when 'if (!container)' check was done *after* relocation,
> >>which made container non-zero. If you do the check *before* then I
> >>think you will catch the fact that container is empty.
> >Ah, right, this was it.
> >
> >Ok, just remove the signature check then and give it a try pls:
>
> Yes, this fixes the bug. And it loads microcode for a good binary.
>
> For these two patches:
> Tested-by: Boris Ostrovsky <[email protected]>
>
> (You might want to rename the final patch to something else since
> you are not checking the blob anymore).
Yes, will do. It'll be one patch though - it is still small enough. I'll
send it as a reply to this message.
Thanks for your help!
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
From: Borislav Petkov <[email protected]>
For additional coverage, BorisO and friends unknowlingly did swap AMD
microcode with Intel microcode blobs in order to see what happens. What
did happen on 32-bit was
[ 5.722656] BUG: unable to handle kernel paging request at be3a6008
[ 5.722693] IP: [<c106d6b4>] load_microcode_amd+0x24/0x3f0
[ 5.722716] *pdpt = 0000000000000000 *pde = 0000000000000000
because there was a valid initrd there but without valid microcode in it
and the container check happened *after* the relocated ramdisk handling
on 32-bit, which was clearly wrong.
While at it, take care of the ramdisk relocation on both 32- and 64-bit
as it is done on both. Also, comment what we're doing because this code
is a bit tricky.
Reported-and-tested-by: Boris Ostrovsky <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/kernel/cpu/microcode/amd_early.c | 43 +++++++++++++++++++++----------
1 file changed, 29 insertions(+), 14 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd_early.c b/arch/x86/kernel/cpu/microcode/amd_early.c
index 8384c0fa206f..617a9e284245 100644
--- a/arch/x86/kernel/cpu/microcode/amd_early.c
+++ b/arch/x86/kernel/cpu/microcode/amd_early.c
@@ -285,6 +285,15 @@ static void __init collect_cpu_sig_on_bsp(void *arg)
uci->cpu_sig.sig = cpuid_eax(0x00000001);
}
+
+static void __init get_bsp_sig(void)
+{
+ unsigned int bsp = boot_cpu_data.cpu_index;
+ struct ucode_cpu_info *uci = ucode_cpu_info + bsp;
+
+ if (!uci->cpu_sig.sig)
+ smp_call_function_single(bsp, collect_cpu_sig_on_bsp, NULL, 1);
+}
#else
void load_ucode_amd_ap(void)
{
@@ -337,31 +346,37 @@ void load_ucode_amd_ap(void)
int __init save_microcode_in_initrd_amd(void)
{
+ unsigned long cont;
enum ucode_state ret;
u32 eax;
-#ifdef CONFIG_X86_32
- unsigned int bsp = boot_cpu_data.cpu_index;
- struct ucode_cpu_info *uci = ucode_cpu_info + bsp;
-
- if (!uci->cpu_sig.sig)
- smp_call_function_single(bsp, collect_cpu_sig_on_bsp, NULL, 1);
+ if (!container)
+ return -EINVAL;
+#ifdef CONFIG_X86_32
+ get_bsp_sig();
+ cont = (unsigned long)container;
+#else
/*
- * Take into account the fact that the ramdisk might get relocated
- * and therefore we need to recompute the container's position in
- * virtual memory space.
+ * We need the physical address of the container for both bitness since
+ * boot_params.hdr.ramdisk_image is a physical address.
*/
- container = (u8 *)(__va((u32)relocated_ramdisk) +
- ((u32)container - boot_params.hdr.ramdisk_image));
+ cont = __pa(container);
#endif
+
+ /*
+ * Take into account the fact that the ramdisk might get relocated and
+ * therefore we need to recompute the container's position in virtual
+ * memory space.
+ */
+ if (relocated_ramdisk)
+ container = (u8 *)(__va(relocated_ramdisk) +
+ (cont - boot_params.hdr.ramdisk_image));
+
if (ucode_new_rev)
pr_info("microcode: updated early to new patch_level=0x%08x\n",
ucode_new_rev);
- if (!container)
- return -EINVAL;
-
eax = cpuid_eax(0x00000001);
eax = ((eax >> 8) & 0xf) + ((eax >> 20) & 0xff);
--
1.8.5.2.192.g7794a68
Commit-ID: 75a1ba5b2c529db60ca49626bcaf0bddf4548438
Gitweb: http://git.kernel.org/tip/75a1ba5b2c529db60ca49626bcaf0bddf4548438
Author: Borislav Petkov <[email protected]>
AuthorDate: Mon, 3 Feb 2014 21:41:44 +0100
Committer: H. Peter Anvin <[email protected]>
CommitDate: Thu, 6 Feb 2014 11:11:19 -0800
x86, microcode, AMD: Unify valid container checks
For additional coverage, BorisO and friends unknowlingly did swap AMD
microcode with Intel microcode blobs in order to see what happens. What
did happen on 32-bit was
[ 5.722656] BUG: unable to handle kernel paging request at be3a6008
[ 5.722693] IP: [<c106d6b4>] load_microcode_amd+0x24/0x3f0
[ 5.722716] *pdpt = 0000000000000000 *pde = 0000000000000000
because there was a valid initrd there but without valid microcode in it
and the container check happened *after* the relocated ramdisk handling
on 32-bit, which was clearly wrong.
While at it, take care of the ramdisk relocation on both 32- and 64-bit
as it is done on both. Also, comment what we're doing because this code
is a bit tricky.
Reported-and-tested-by: Boris Ostrovsky <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: H. Peter Anvin <[email protected]>
---
arch/x86/kernel/cpu/microcode/amd_early.c | 43 +++++++++++++++++++++----------
1 file changed, 29 insertions(+), 14 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd_early.c b/arch/x86/kernel/cpu/microcode/amd_early.c
index 8384c0f..617a9e2 100644
--- a/arch/x86/kernel/cpu/microcode/amd_early.c
+++ b/arch/x86/kernel/cpu/microcode/amd_early.c
@@ -285,6 +285,15 @@ static void __init collect_cpu_sig_on_bsp(void *arg)
uci->cpu_sig.sig = cpuid_eax(0x00000001);
}
+
+static void __init get_bsp_sig(void)
+{
+ unsigned int bsp = boot_cpu_data.cpu_index;
+ struct ucode_cpu_info *uci = ucode_cpu_info + bsp;
+
+ if (!uci->cpu_sig.sig)
+ smp_call_function_single(bsp, collect_cpu_sig_on_bsp, NULL, 1);
+}
#else
void load_ucode_amd_ap(void)
{
@@ -337,31 +346,37 @@ void load_ucode_amd_ap(void)
int __init save_microcode_in_initrd_amd(void)
{
+ unsigned long cont;
enum ucode_state ret;
u32 eax;
-#ifdef CONFIG_X86_32
- unsigned int bsp = boot_cpu_data.cpu_index;
- struct ucode_cpu_info *uci = ucode_cpu_info + bsp;
-
- if (!uci->cpu_sig.sig)
- smp_call_function_single(bsp, collect_cpu_sig_on_bsp, NULL, 1);
+ if (!container)
+ return -EINVAL;
+#ifdef CONFIG_X86_32
+ get_bsp_sig();
+ cont = (unsigned long)container;
+#else
/*
- * Take into account the fact that the ramdisk might get relocated
- * and therefore we need to recompute the container's position in
- * virtual memory space.
+ * We need the physical address of the container for both bitness since
+ * boot_params.hdr.ramdisk_image is a physical address.
*/
- container = (u8 *)(__va((u32)relocated_ramdisk) +
- ((u32)container - boot_params.hdr.ramdisk_image));
+ cont = __pa(container);
#endif
+
+ /*
+ * Take into account the fact that the ramdisk might get relocated and
+ * therefore we need to recompute the container's position in virtual
+ * memory space.
+ */
+ if (relocated_ramdisk)
+ container = (u8 *)(__va(relocated_ramdisk) +
+ (cont - boot_params.hdr.ramdisk_image));
+
if (ucode_new_rev)
pr_info("microcode: updated early to new patch_level=0x%08x\n",
ucode_new_rev);
- if (!container)
- return -EINVAL;
-
eax = cpuid_eax(0x00000001);
eax = ((eax >> 8) & 0xf) + ((eax >> 20) & 0xff);