linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] x86/microcode: Change checksum to u32
@ 2016-03-01 12:12 Chris Bainbridge
  2016-03-01 14:29 ` Borislav Petkov
  2016-03-08  8:12 ` [tip:x86/microcode] x86/microcode/intel: Change checksum variables " tip-bot for Chris Bainbridge
  0 siblings, 2 replies; 3+ messages in thread
From: Chris Bainbridge @ 2016-03-01 12:12 UTC (permalink / raw)
  To: bp; +Cc: Chris Bainbridge, x86, linux-kernel, hmh

Checksum should be unsigned 32-bit otherwise the calculation overflow
results in undefined behaviour:

[    0.000000] ================================================================================
[    0.000000] UBSAN: Undefined behaviour in arch/x86/kernel/cpu/microcode/intel_lib.c:105:12
[    0.000000] signed integer overflow:
[    0.000000] -1500151068 + -2125470173 cannot be represented in type 'int'
[    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.5.0-rc5+ #495
[    0.000000]  0000000000000086 0000000000000086 0000000000000000 ffffffff83203968
[    0.000000]  ffffffff81b30952 ffffffff834c43b8 ffffffff83203998 ffffffff814fe623
[    0.000000]  ffffffff83203980 ffffffff81bcdf2d ffffffff8339a448 ffffffff83203a08
[    0.000000] Call Trace:
[    0.000000]  [<ffffffff81b30952>] dump_stack+0x4e/0x6c
[    0.000000]  [<ffffffff814fe623>] ? inotify_ioctl+0x43/0x1c0
[    0.000000]  [<ffffffff81bcdf2d>] ubsan_epilogue+0xd/0x40
[    0.000000]  [<ffffffff81bce2fd>] handle_overflow+0xbd/0xe0
[    0.000000]  [<ffffffff81bce32e>] __ubsan_handle_add_overflow+0xe/0x10
[    0.000000]  [<ffffffff81148e45>] microcode_sanity_check+0x405/0x590
[    0.000000]  [<ffffffff8549b01b>] get_matching_model_microcode.isra.2.constprop.8+0xa5/0x345
[    0.000000]  [<ffffffff8548615d>] ? early_idt_handler_common+0x3d/0xae
[    0.000000]  [<ffffffff81b50962>] ? strlcpy+0x52/0xa0
[    0.000000]  [<ffffffff81b30ce1>] ? find_cpio_data+0x371/0x510
[    0.000000]  [<ffffffff8549b461>] load_ucode_intel_bsp+0xa1/0xe5
[    0.000000]  [<ffffffff8549aea8>] load_ucode_bsp+0xdf/0xf3
[    0.000000]  [<ffffffff8549aea8>] ? load_ucode_bsp+0xdf/0xf3
[    0.000000]  [<ffffffff8548671c>] x86_64_start_kernel+0xd1/0xee
[    0.000000] ================================================================================

Link: https://lkml.org/lkml/2016/2/27/79
Signed-off-by: Chris Bainbridge <chris.bainbridge@gmail.com>
---
 arch/x86/kernel/cpu/microcode/intel_lib.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
index b96896bcbdaf..99ca2c935777 100644
--- a/arch/x86/kernel/cpu/microcode/intel_lib.c
+++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
@@ -49,7 +49,7 @@ int microcode_sanity_check(void *mc, int print_err)
 	unsigned long total_size, data_size, ext_table_size;
 	struct microcode_header_intel *mc_header = mc;
 	struct extended_sigtable *ext_header = NULL;
-	int sum, orig_sum, ext_sigcount = 0, i;
+	u32 sum, orig_sum, ext_sigcount = 0, i;
 	struct extended_signature *ext_sig;
 
 	total_size = get_totalsize(mc_header);
@@ -85,8 +85,8 @@ int microcode_sanity_check(void *mc, int print_err)
 
 	/* check extended table checksum */
 	if (ext_table_size) {
-		int ext_table_sum = 0;
-		int *ext_tablep = (int *)ext_header;
+		u32 ext_table_sum = 0;
+		u32 *ext_tablep = (u32 *)ext_header;
 
 		i = ext_table_size / DWSIZE;
 		while (i--)
@@ -102,7 +102,7 @@ int microcode_sanity_check(void *mc, int print_err)
 	orig_sum = 0;
 	i = (MC_HEADER_SIZE + data_size) / DWSIZE;
 	while (i--)
-		orig_sum += ((int *)mc)[i];
+		orig_sum += ((u32 *)mc)[i];
 	if (orig_sum) {
 		if (print_err)
 			pr_err("aborting, bad checksum\n");
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH v2] x86/microcode: Change checksum to u32
  2016-03-01 12:12 [PATCH v2] x86/microcode: Change checksum to u32 Chris Bainbridge
@ 2016-03-01 14:29 ` Borislav Petkov
  2016-03-08  8:12 ` [tip:x86/microcode] x86/microcode/intel: Change checksum variables " tip-bot for Chris Bainbridge
  1 sibling, 0 replies; 3+ messages in thread
From: Borislav Petkov @ 2016-03-01 14:29 UTC (permalink / raw)
  To: Chris Bainbridge; +Cc: x86, linux-kernel, hmh

On Tue, Mar 01, 2016 at 12:12:39PM +0000, Chris Bainbridge wrote:
> Checksum should be unsigned 32-bit otherwise the calculation overflow
> results in undefined behaviour:
> 
> [    0.000000] ================================================================================
> [    0.000000] UBSAN: Undefined behaviour in arch/x86/kernel/cpu/microcode/intel_lib.c:105:12
> [    0.000000] signed integer overflow:
> [    0.000000] -1500151068 + -2125470173 cannot be represented in type 'int'
> [    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.5.0-rc5+ #495
> [    0.000000]  0000000000000086 0000000000000086 0000000000000000 ffffffff83203968
> [    0.000000]  ffffffff81b30952 ffffffff834c43b8 ffffffff83203998 ffffffff814fe623
> [    0.000000]  ffffffff83203980 ffffffff81bcdf2d ffffffff8339a448 ffffffff83203a08
> [    0.000000] Call Trace:
> [    0.000000]  [<ffffffff81b30952>] dump_stack+0x4e/0x6c
> [    0.000000]  [<ffffffff814fe623>] ? inotify_ioctl+0x43/0x1c0
> [    0.000000]  [<ffffffff81bcdf2d>] ubsan_epilogue+0xd/0x40
> [    0.000000]  [<ffffffff81bce2fd>] handle_overflow+0xbd/0xe0
> [    0.000000]  [<ffffffff81bce32e>] __ubsan_handle_add_overflow+0xe/0x10
> [    0.000000]  [<ffffffff81148e45>] microcode_sanity_check+0x405/0x590
> [    0.000000]  [<ffffffff8549b01b>] get_matching_model_microcode.isra.2.constprop.8+0xa5/0x345
> [    0.000000]  [<ffffffff8548615d>] ? early_idt_handler_common+0x3d/0xae
> [    0.000000]  [<ffffffff81b50962>] ? strlcpy+0x52/0xa0
> [    0.000000]  [<ffffffff81b30ce1>] ? find_cpio_data+0x371/0x510
> [    0.000000]  [<ffffffff8549b461>] load_ucode_intel_bsp+0xa1/0xe5
> [    0.000000]  [<ffffffff8549aea8>] load_ucode_bsp+0xdf/0xf3
> [    0.000000]  [<ffffffff8549aea8>] ? load_ucode_bsp+0xdf/0xf3
> [    0.000000]  [<ffffffff8548671c>] x86_64_start_kernel+0xd1/0xee
> [    0.000000] ================================================================================
> 
> Link: https://lkml.org/lkml/2016/2/27/79
> Signed-off-by: Chris Bainbridge <chris.bainbridge@gmail.com>
> ---
>  arch/x86/kernel/cpu/microcode/intel_lib.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Applied, thanks.

-- 
Regards/Gruss,
    Boris.

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

^ permalink raw reply	[flat|nested] 3+ messages in thread
* [tip:x86/microcode] x86/microcode/intel: Change checksum variables to u32
  2016-03-01 12:12 [PATCH v2] x86/microcode: Change checksum to u32 Chris Bainbridge
  2016-03-01 14:29 ` Borislav Petkov
@ 2016-03-08  8:12 ` tip-bot for Chris Bainbridge
  1 sibling, 0 replies; 3+ messages in thread
From: tip-bot for Chris Bainbridge @ 2016-03-08  8:12 UTC (permalink / raw)
  To: linux-tip-commits; +Cc: mingo, bp, tglx, linux-kernel, chris.bainbridge, hpa

Commit-ID:  bc864af13f34d19c911f5691d87bdacc9ce109f5
Gitweb:     http://git.kernel.org/tip/bc864af13f34d19c911f5691d87bdacc9ce109f5
Author:     Chris Bainbridge <chris.bainbridge@gmail.com>
AuthorDate: Mon, 7 Mar 2016 11:10:00 +0100
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 8 Mar 2016 09:08:44 +0100

x86/microcode/intel: Change checksum variables to u32

Microcode checksum verification should be done using unsigned 32-bit
values otherwise the calculation overflow results in undefined
behaviour.

This is also nicely documented in the SDM, section "Microcode Update
Checksum":

  "To check for a corrupt microcode update, software must perform a
  unsigned DWORD (32-bit) checksum of the microcode update. Even though
  some fields are signed, the checksum procedure treats all DWORDs as
  unsigned. Microcode updates with a header version equal to 00000001H
  must sum all DWORDs that comprise the microcode update. A valid
  checksum check will yield a value of 00000000H."

but for some reason the code has been using ints from the very
beginning.

In practice, this bug possibly manifested itself only when doing the
microcode data checksum - apparently, currently shipped Intel microcode
doesn't have an extended signature table for which we do checksum
verification too.

  UBSAN: Undefined behaviour in arch/x86/kernel/cpu/microcode/intel_lib.c:105:12
  signed integer overflow:
  -1500151068 + -2125470173 cannot be represented in type 'int'
  CPU: 0 PID: 0 Comm: swapper Not tainted 4.5.0-rc5+ #495
  ...
  Call Trace:
   dump_stack
   ? inotify_ioctl
   ubsan_epilogue
   handle_overflow
   __ubsan_handle_add_overflow
   microcode_sanity_check
   get_matching_model_microcode.isra.2.constprop.8
   ? early_idt_handler_common
   ? strlcpy
   ? find_cpio_data
   load_ucode_intel_bsp
   load_ucode_bsp
   ? load_ucode_bsp
   x86_64_start_kernel

[ Expand and massage commit message. ]
Signed-off-by: Chris Bainbridge <chris.bainbridge@gmail.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: hmh@hmh.eng.br
Link: http://lkml.kernel.org/r/1456834359-5132-1-git-send-email-chris.bainbridge@gmail.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/cpu/microcode/intel_lib.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
index b96896b..99ca2c9 100644
--- a/arch/x86/kernel/cpu/microcode/intel_lib.c
+++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
@@ -49,7 +49,7 @@ int microcode_sanity_check(void *mc, int print_err)
 	unsigned long total_size, data_size, ext_table_size;
 	struct microcode_header_intel *mc_header = mc;
 	struct extended_sigtable *ext_header = NULL;
-	int sum, orig_sum, ext_sigcount = 0, i;
+	u32 sum, orig_sum, ext_sigcount = 0, i;
 	struct extended_signature *ext_sig;
 
 	total_size = get_totalsize(mc_header);
@@ -85,8 +85,8 @@ int microcode_sanity_check(void *mc, int print_err)
 
 	/* check extended table checksum */
 	if (ext_table_size) {
-		int ext_table_sum = 0;
-		int *ext_tablep = (int *)ext_header;
+		u32 ext_table_sum = 0;
+		u32 *ext_tablep = (u32 *)ext_header;
 
 		i = ext_table_size / DWSIZE;
 		while (i--)
@@ -102,7 +102,7 @@ int microcode_sanity_check(void *mc, int print_err)
 	orig_sum = 0;
 	i = (MC_HEADER_SIZE + data_size) / DWSIZE;
 	while (i--)
-		orig_sum += ((int *)mc)[i];
+		orig_sum += ((u32 *)mc)[i];
 	if (orig_sum) {
 		if (print_err)
 			pr_err("aborting, bad checksum\n");

^ permalink raw reply related	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-03-08  8:13 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-01 12:12 [PATCH v2] x86/microcode: Change checksum to u32 Chris Bainbridge
2016-03-01 14:29 ` Borislav Petkov
2016-03-08  8:12 ` [tip:x86/microcode] x86/microcode/intel: Change checksum variables " tip-bot for Chris Bainbridge

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