x86/microcode: Change checksum to u32
Subsystems (2)
PATCH v2 1/1 x86/microcode: Change checksum to u32
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] ================================================================================
Signed-off-by Chris Bainbridge <[email protected]>
--- arch/x86/kernel/cpu/microcode/intel_lib.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
arch/x86/kernel/cpu/microcode/intel_lib.c +4 -4
@@ -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" );
Signature · 1 line
2.1.4
Quoted 28 lines · Chris Bainbridge
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] ================================================================================
Quoted 5 lines
Signed-off-by Chris Bainbridge <[email protected]>--- arch/x86/kernel/cpu/microcode/intel_lib.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
Applied, thanks.
Signature · 4 lines
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.tip-bot for Chris Bainbridge
merge bot[tip:x86/microcode] x86/microcode/intel: Change checksum variables to u32
In reply to Chris Bainbridgex86/microcode/intel: Change checksum variables to u32
Commit-ID: bc864af13f34d19c911f5691d87bdacc9ce109f5 Gitweb: http://git.kernel.org/tip/bc864af13f34d19c911f5691d87bdacc9ce109f5 Author: Chris Bainbridge <[email protected]> AuthorDate: Mon, 7 Mar 2016 11:10:00 +0100 Committer: Thomas Gleixner <[email protected]> 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 <[email protected]>
Signed-off-by Borislav Petkov <[email protected]>
Signed-off-by Thomas Gleixner <[email protected]>
--- arch/x86/kernel/cpu/microcode/intel_lib.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
arch/x86/kernel/cpu/microcode/intel_lib.c +4 -4
@@ -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" );