* [PATCH] x86/microcode/intel: Use correct buffer size for saving microcode data
@ 2017-01-05 1:02 Junichi Nomura
2017-01-05 4:45 ` Junichi Nomura
0 siblings, 1 reply; 6+ messages in thread
From: Junichi Nomura @ 2017-01-05 1:02 UTC (permalink / raw)
To: bp, x86, linux-kernel; +Cc: tglx, mingo, hpa
In generic_load_microcode(), curr_mc_size is the size of the last
allocated buffer and not always the size of the buffer pointed to by
"new_mc".
Without this fix, we could get oops like this:
BUG: unable to handle kernel paging request at ffffc9000e30f000
IP: __memcpy+0x12/0x20
...
Call Trace:
? kmemdup+0x43/0x60
__alloc_microcode_buf+0x44/0x70
save_microcode_patch+0xd4/0x150
generic_load_microcode+0x1b8/0x260
request_microcode_user+0x15/0x20
microcode_write+0x91/0x100
__vfs_write+0x34/0x120
vfs_write+0xc1/0x130
SyS_write+0x56/0xc0
do_syscall_64+0x6c/0x160
entry_SYSCALL64_slow_path+0x25/0x25
Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Fixes: 06b8534cb728 ("x86/microcode: Rework microcode loading")
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index faec8fa..aee3cb5 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -823,7 +823,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
u8 *ucode_ptr = data, *new_mc = NULL, *mc = NULL;
int new_rev = uci->cpu_sig.rev;
unsigned int leftover = size;
- unsigned int curr_mc_size = 0;
+ unsigned int curr_mc_size = 0, new_mc_size = 0;
unsigned int csig, cpf;
while (leftover) {
@@ -864,6 +864,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
vfree(new_mc);
new_rev = mc_header.rev;
new_mc = mc;
+ new_mc_size = curr_mc_size;
mc = NULL; /* trigger new vmalloc */
}
@@ -889,7 +890,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
* permanent memory. So it will be loaded early when a CPU is hot added
* or resumes.
*/
- save_mc_for_early(new_mc, curr_mc_size);
+ save_mc_for_early(new_mc, new_mc_size);
pr_debug("CPU%d found a matching microcode update with version 0x%x (current=0x%x)\n",
cpu, new_rev, uci->cpu_sig.rev);
--
Jun'ichi Nomura, NEC Corporation / NEC Solution Innovators, Ltd.
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/microcode/intel: Use correct buffer size for saving microcode data
2017-01-05 1:02 [PATCH] x86/microcode/intel: Use correct buffer size for saving microcode data Junichi Nomura
@ 2017-01-05 4:45 ` Junichi Nomura
2017-01-05 10:17 ` Borislav Petkov
2017-01-09 22:18 ` [tip:x86/urgent] " tip-bot for Junichi Nomura
0 siblings, 2 replies; 6+ messages in thread
From: Junichi Nomura @ 2017-01-05 4:45 UTC (permalink / raw)
To: bp, x86, linux-kernel; +Cc: tglx, mingo, hpa
On 01/05/17 10:02, Junichi Nomura wrote:
> In generic_load_microcode(), curr_mc_size is the size of the last
> allocated buffer and not always the size of the buffer pointed to by
> "new_mc".
...
> @@ -864,6 +864,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
> vfree(new_mc);
> new_rev = mc_header.rev;
> new_mc = mc;
> + new_mc_size = curr_mc_size;
> mc = NULL; /* trigger new vmalloc */
Oops, sorry. It should save "mc_size", not "curr_mc_size".
-------------------
In generic_load_microcode(), curr_mc_size is the size of the last
allocated buffer and could be smaller than the actual size of the
buffer pointed to by "new_mc".
Without this fix, we could get oops like this:
BUG: unable to handle kernel paging request at ffffc9000e30f000
IP: __memcpy+0x12/0x20
...
Call Trace:
? kmemdup+0x43/0x60
__alloc_microcode_buf+0x44/0x70
save_microcode_patch+0xd4/0x150
generic_load_microcode+0x1b8/0x260
request_microcode_user+0x15/0x20
microcode_write+0x91/0x100
__vfs_write+0x34/0x120
vfs_write+0xc1/0x130
SyS_write+0x56/0xc0
do_syscall_64+0x6c/0x160
entry_SYSCALL64_slow_path+0x25/0x25
Save "mc_size" and use it for save_mc_for_early().
Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Fixes: 06b8534cb728 ("x86/microcode: Rework microcode loading")
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index faec8fa..aee3cb5 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -823,7 +823,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
u8 *ucode_ptr = data, *new_mc = NULL, *mc = NULL;
int new_rev = uci->cpu_sig.rev;
unsigned int leftover = size;
- unsigned int curr_mc_size = 0;
+ unsigned int curr_mc_size = 0, new_mc_size = 0;
unsigned int csig, cpf;
while (leftover) {
@@ -864,6 +864,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
vfree(new_mc);
new_rev = mc_header.rev;
new_mc = mc;
+ new_mc_size = mc_size;
mc = NULL; /* trigger new vmalloc */
}
@@ -889,7 +890,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
* permanent memory. So it will be loaded early when a CPU is hot added
* or resumes.
*/
- save_mc_for_early(new_mc, curr_mc_size);
+ save_mc_for_early(new_mc, new_mc_size);
pr_debug("CPU%d found a matching microcode update with version 0x%x (current=0x%x)\n",
cpu, new_rev, uci->cpu_sig.rev);
--
Jun'ichi Nomura, NEC Corporation / NEC Solution Innovators, Ltd.
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/microcode/intel: Use correct buffer size for saving microcode data
2017-01-05 4:45 ` Junichi Nomura
@ 2017-01-05 10:17 ` Borislav Petkov
2017-01-05 23:47 ` Junichi Nomura
2017-01-09 22:18 ` [tip:x86/urgent] " tip-bot for Junichi Nomura
1 sibling, 1 reply; 6+ messages in thread
From: Borislav Petkov @ 2017-01-05 10:17 UTC (permalink / raw)
To: Junichi Nomura; +Cc: x86, linux-kernel, tglx, mingo, hpa
On Thu, Jan 05, 2017 at 04:45:18AM +0000, Junichi Nomura wrote:
> On 01/05/17 10:02, Junichi Nomura wrote:
> > In generic_load_microcode(), curr_mc_size is the size of the last
> > allocated buffer and not always the size of the buffer pointed to by
> > "new_mc".
> ...
> > @@ -864,6 +864,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
> > vfree(new_mc);
> > new_rev = mc_header.rev;
> > new_mc = mc;
> > + new_mc_size = curr_mc_size;
> > mc = NULL; /* trigger new vmalloc */
>
> Oops, sorry. It should save "mc_size", not "curr_mc_size".
>
> -------------------
> In generic_load_microcode(), curr_mc_size is the size of the last
> allocated buffer and could be smaller than the actual size of the
> buffer pointed to by "new_mc".
>
> Without this fix, we could get oops like this:
>
> BUG: unable to handle kernel paging request at ffffc9000e30f000
> IP: __memcpy+0x12/0x20
> ...
> Call Trace:
> ? kmemdup+0x43/0x60
> __alloc_microcode_buf+0x44/0x70
> save_microcode_patch+0xd4/0x150
> generic_load_microcode+0x1b8/0x260
> request_microcode_user+0x15/0x20
I see you're using the old interface but how exactly do you trigger
this, i.e., can you give me the blob you're using and the exact steps
you're performing? I'd like to reproduce it here.
Thanks.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/microcode/intel: Use correct buffer size for saving microcode data
2017-01-05 10:17 ` Borislav Petkov
@ 2017-01-05 23:47 ` Junichi Nomura
2017-01-06 12:39 ` Borislav Petkov
0 siblings, 1 reply; 6+ messages in thread
From: Junichi Nomura @ 2017-01-05 23:47 UTC (permalink / raw)
To: Borislav Petkov; +Cc: x86, linux-kernel, tglx, mingo, hpa
On 01/05/17 19:17, Borislav Petkov wrote:
> On Thu, Jan 05, 2017 at 04:45:18AM +0000, Junichi Nomura wrote:
>> In generic_load_microcode(), curr_mc_size is the size of the last
>> allocated buffer and could be smaller than the actual size of the
>> buffer pointed to by "new_mc".
>>
>> Without this fix, we could get oops like this:
>>
>> BUG: unable to handle kernel paging request at ffffc9000e30f000
>> IP: __memcpy+0x12/0x20
>> ...
>> Call Trace:
>> ? kmemdup+0x43/0x60
>> __alloc_microcode_buf+0x44/0x70
>> save_microcode_patch+0xd4/0x150
>> generic_load_microcode+0x1b8/0x260
>> request_microcode_user+0x15/0x20
>
> I see you're using the old interface but how exactly do you trigger
> this, i.e., can you give me the blob you're using and the exact steps
> you're performing? I'd like to reproduce it here.
I'm seeing this on RHEL6-based userspace, where 'microcode_ctl -Qu' is
run from udev automatically when microcode driver is loaded.
The version of microcode_ctl is 1.17.
The problem can be reproduced either with the one in RHEL6 (
microcode-20151106.dat) or with the latest blob from Intel,
i.e. microcode-20161104.tgz.
--
Jun'ichi Nomura, NEC Corporation / NEC Solution Innovators, Ltd.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/microcode/intel: Use correct buffer size for saving microcode data
2017-01-05 23:47 ` Junichi Nomura
@ 2017-01-06 12:39 ` Borislav Petkov
0 siblings, 0 replies; 6+ messages in thread
From: Borislav Petkov @ 2017-01-06 12:39 UTC (permalink / raw)
To: Junichi Nomura; +Cc: x86, linux-kernel, tglx, mingo, hpa
On Thu, Jan 05, 2017 at 11:47:56PM +0000, Junichi Nomura wrote:
> The problem can be reproduced either with the one in RHEL6 (
> microcode-20151106.dat) or with the latest blob from Intel,
> i.e. microcode-20161104.tgz.
Ok, thanks, I was able to reproduce and now I see what's going on. I've
expanded your commit message to explain the situation better, see below.
Thanks again for that good catch!
---
From: Junichi Nomura <j-nomura@ce.jp.nec.com>
Date: Thu, 5 Jan 2017 04:45:18 +0000
Subject: [PATCH] x86/microcode/intel: Use correct buffer size for saving
microcode data
In generic_load_microcode(), curr_mc_size is the size of the last
allocated buffer and since we have this performance "optimization"
there to vmalloc a new buffer only when the current one is bigger,
curr_mc_size ends up becoming the size of the biggest buffer we've seen
so far.
However, we end up saving the microcode patch which matches our CPU
and its size is not curr_mc_size but the respective mc_size during the
iteration while we're staring at it.
So save that mc_size into a separate variable and use it to store the
previously found microcode buffer.
Without this fix, we could get oops like this:
BUG: unable to handle kernel paging request at ffffc9000e30f000
IP: __memcpy+0x12/0x20
...
Call Trace:
? kmemdup+0x43/0x60
__alloc_microcode_buf+0x44/0x70
save_microcode_patch+0xd4/0x150
generic_load_microcode+0x1b8/0x260
request_microcode_user+0x15/0x20
microcode_write+0x91/0x100
__vfs_write+0x34/0x120
vfs_write+0xc1/0x130
SyS_write+0x56/0xc0
do_syscall_64+0x6c/0x160
entry_SYSCALL64_slow_path+0x25/0x25
Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Link: http://lkml.kernel.org/r/4f33cbfd-44f2-9bed-3b66-7446cd14256f@ce.jp.nec.com
Fixes: 06b8534cb728 ("x86/microcode: Rework microcode loading")
Signed-off-by: Borislav Petkov <bp@suse.de>
---
arch/x86/kernel/cpu/microcode/intel.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 943486589757..3f329b74e040 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -823,7 +823,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
u8 *ucode_ptr = data, *new_mc = NULL, *mc = NULL;
int new_rev = uci->cpu_sig.rev;
unsigned int leftover = size;
- unsigned int curr_mc_size = 0;
+ unsigned int curr_mc_size = 0, new_mc_size = 0;
unsigned int csig, cpf;
while (leftover) {
@@ -864,6 +864,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
vfree(new_mc);
new_rev = mc_header.rev;
new_mc = mc;
+ new_mc_size = mc_size;
mc = NULL; /* trigger new vmalloc */
}
@@ -889,7 +890,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
* permanent memory. So it will be loaded early when a CPU is hot added
* or resumes.
*/
- save_mc_for_early(new_mc, curr_mc_size);
+ save_mc_for_early(new_mc, new_mc_size);
pr_debug("CPU%d found a matching microcode update with version 0x%x (current=0x%x)\n",
cpu, new_rev, uci->cpu_sig.rev);
--
2.11.0
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [tip:x86/urgent] x86/microcode/intel: Use correct buffer size for saving microcode data
2017-01-05 4:45 ` Junichi Nomura
2017-01-05 10:17 ` Borislav Petkov
@ 2017-01-09 22:18 ` tip-bot for Junichi Nomura
1 sibling, 0 replies; 6+ messages in thread
From: tip-bot for Junichi Nomura @ 2017-01-09 22:18 UTC (permalink / raw)
To: linux-tip-commits; +Cc: j-nomura, mingo, tglx, bp, hpa, linux-kernel
Commit-ID: 2e86222c67bb5d942da68e8415749b32db208534
Gitweb: http://git.kernel.org/tip/2e86222c67bb5d942da68e8415749b32db208534
Author: Junichi Nomura <j-nomura@ce.jp.nec.com>
AuthorDate: Mon, 9 Jan 2017 12:41:47 +0100
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Mon, 9 Jan 2017 23:11:15 +0100
x86/microcode/intel: Use correct buffer size for saving microcode data
In generic_load_microcode(), curr_mc_size is the size of the last
allocated buffer and since we have this performance "optimization"
there to vmalloc a new buffer only when the current one is bigger,
curr_mc_size ends up becoming the size of the biggest buffer we've seen
so far.
However, we end up saving the microcode patch which matches our CPU
and its size is not curr_mc_size but the respective mc_size during the
iteration while we're staring at it.
So save that mc_size into a separate variable and use it to store the
previously found microcode buffer.
Without this fix, we could get oops like this:
BUG: unable to handle kernel paging request at ffffc9000e30f000
IP: __memcpy+0x12/0x20
...
Call Trace:
? kmemdup+0x43/0x60
__alloc_microcode_buf+0x44/0x70
save_microcode_patch+0xd4/0x150
generic_load_microcode+0x1b8/0x260
request_microcode_user+0x15/0x20
microcode_write+0x91/0x100
__vfs_write+0x34/0x120
vfs_write+0xc1/0x130
SyS_write+0x56/0xc0
do_syscall_64+0x6c/0x160
entry_SYSCALL64_slow_path+0x25/0x25
Fixes: 06b8534cb728 ("x86/microcode: Rework microcode loading")
Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Link: http://lkml.kernel.org/r/4f33cbfd-44f2-9bed-3b66-7446cd14256f@ce.jp.nec.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
arch/x86/kernel/cpu/microcode/intel.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 9434865..3f329b7 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -823,7 +823,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
u8 *ucode_ptr = data, *new_mc = NULL, *mc = NULL;
int new_rev = uci->cpu_sig.rev;
unsigned int leftover = size;
- unsigned int curr_mc_size = 0;
+ unsigned int curr_mc_size = 0, new_mc_size = 0;
unsigned int csig, cpf;
while (leftover) {
@@ -864,6 +864,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
vfree(new_mc);
new_rev = mc_header.rev;
new_mc = mc;
+ new_mc_size = mc_size;
mc = NULL; /* trigger new vmalloc */
}
@@ -889,7 +890,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
* permanent memory. So it will be loaded early when a CPU is hot added
* or resumes.
*/
- save_mc_for_early(new_mc, curr_mc_size);
+ save_mc_for_early(new_mc, new_mc_size);
pr_debug("CPU%d found a matching microcode update with version 0x%x (current=0x%x)\n",
cpu, new_rev, uci->cpu_sig.rev);
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-01-09 22:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-05 1:02 [PATCH] x86/microcode/intel: Use correct buffer size for saving microcode data Junichi Nomura
2017-01-05 4:45 ` Junichi Nomura
2017-01-05 10:17 ` Borislav Petkov
2017-01-05 23:47 ` Junichi Nomura
2017-01-06 12:39 ` Borislav Petkov
2017-01-09 22:18 ` [tip:x86/urgent] " tip-bot for Junichi Nomura
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).