x86/microcode/intel: Microcode verification cleanups
Subsystems (2)
Borislav Petkov
maintainercover x86/microcode/intel: Microcode verification cleanups
From: Borislav Petkov <[email protected]>
Hi,
here's a small set which makes sure the microcode data checksum verification is done using u32s, as the SDM says and not what we did. The interesting part is that Chris Bainbridge caught this with UBSAN even though the SDM has been proclaiming using unsigned dwords already.
The rest are some trivial cleanups which sprang out after me staring at microcode_sanity_check(). FWIW, it could use some more but that's for later.
Patches ontop of tip/x86/microcode.
Borislav Petkov (4):
x86/microcode/intel: Get rid of DWSIZE x86/microcode/intel: Merge two consecutive if-statements x86/microcode/intel: Improve microcode sanity-checking error messages x86/microcode/intel: Drop orig_sum from ext signature checksum
Chris Bainbridge (1):
x86/microcode/intel: Change checksum variables to u32
arch/x86/include/asm/microcode_intel.h | 1 - arch/x86/kernel/cpu/microcode/intel_lib.c | 58 ++++++++++++++++++++----------- 2 files changed, 37 insertions(+), 22 deletions(-)
Signature · 1 line
2.3.5
Borislav Petkov
maintainerPATCH 1/5 x86/microcode/intel: Change checksum variables to u32
From: Chris Bainbridge <[email protected]>
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
Signed-off-by Chris Bainbridge <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc x86-ml <[email protected]>
[ Expand and massage commit message. ]
Signed-off-by Borislav Petkov <[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.3.5
Borislav Petkov
maintainerPATCH 2/5 x86/microcode/intel: Get rid of DWSIZE
In reply to Borislav PetkovFrom: Borislav Petkov <[email protected]>
sizeof(u32) is perfectly clear as it is.
Signed-off-by Borislav Petkov <[email protected]>
--- arch/x86/include/asm/microcode_intel.h | 1 - arch/x86/kernel/cpu/microcode/intel_lib.c | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-)
arch/x86/include/asm/microcode_intel.h +0 -1
@@ -40,7 +40,6 @@ struct extended_sigtable {#define DEFAULT_UCODE_TOTALSIZE (DEFAULT_UCODE_DATASIZE + MC_HEADER_SIZE)#define EXT_HEADER_SIZE (sizeof(struct extended_sigtable))#define EXT_SIGNATURE_SIZE (sizeof(struct extended_signature)) -#define DWSIZE (sizeof(u32))#define get_totalsize (mc) \ (((struct microcode_intel *)mc)->hdr.datasize ? \
arch/x86/kernel/cpu/microcode/intel_lib.c +2 -2
@@ -88,7 +88,7 @@ int microcode_sanity_check(void *mc, int print_err)u32 ext_table_sum =0 ;u32 *ext_tablep = (u32 *)ext_header; - i = ext_table_size /DWSIZE ; + i = ext_table_size /sizeof (u32);while (i--) ext_table_sum += ext_tablep[i];if (ext_table_sum) {
@@ -100,7 +100,7 @@ int microcode_sanity_check(void *mc, int print_err)/* calculate the checksum */ orig_sum =0 ; - i = (MC_HEADER_SIZE + data_size) /DWSIZE ; + i = (MC_HEADER_SIZE + data_size) /sizeof (u32);while (i--) orig_sum += ((u32 *)mc)[i];if (orig_sum) {
Signature · 1 line
2.3.5
tip-bot for Borislav Petkov
merge bot[tip:x86/microcode] x86/microcode/intel: Get rid of DWSIZE
x86/microcode/intel: Get rid of DWSIZE
Commit-ID: c0414622177ae4739a49ca7dad4306a681e2878b Gitweb: http://git.kernel.org/tip/c0414622177ae4739a49ca7dad4306a681e2878b Author: Borislav Petkov <[email protected]> AuthorDate: Mon, 7 Mar 2016 11:10:01 +0100 Committer: Thomas Gleixner <[email protected]> CommitDate: Tue, 8 Mar 2016 09:08:44 +0100
x86/microcode/intel: Get rid of DWSIZE
sizeof(u32) is perfectly clear as it is.
Signed-off-by Borislav Petkov <[email protected]>
Signed-off-by Thomas Gleixner <[email protected]>
--- arch/x86/include/asm/microcode_intel.h | 1 - arch/x86/kernel/cpu/microcode/intel_lib.c | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-)
arch/x86/include/asm/microcode_intel.h +0 -1
@@ -40,7 +40,6 @@ struct extended_sigtable {#define DEFAULT_UCODE_TOTALSIZE (DEFAULT_UCODE_DATASIZE + MC_HEADER_SIZE)#define EXT_HEADER_SIZE (sizeof(struct extended_sigtable))#define EXT_SIGNATURE_SIZE (sizeof(struct extended_signature)) -#define DWSIZE (sizeof(u32))#define get_totalsize (mc) \ (((struct microcode_intel *)mc)->hdr.datasize ? \
arch/x86/kernel/cpu/microcode/intel_lib.c +2 -2
@@ -88,7 +88,7 @@ int microcode_sanity_check(void *mc, int print_err)u32 ext_table_sum =0 ;u32 *ext_tablep = (u32 *)ext_header; - i = ext_table_size /DWSIZE ; + i = ext_table_size /sizeof (u32);while (i--) ext_table_sum += ext_tablep[i];if (ext_table_sum) {
@@ -100,7 +100,7 @@ int microcode_sanity_check(void *mc, int print_err)/* calculate the checksum */ orig_sum =0 ; - i = (MC_HEADER_SIZE + data_size) /DWSIZE ; + i = (MC_HEADER_SIZE + data_size) /sizeof (u32);while (i--) orig_sum += ((u32 *)mc)[i];if (orig_sum) {
Borislav Petkov
maintainerPATCH 3/5 x86/microcode/intel: Merge two consecutive if-statements
In reply to Borislav PetkovFrom: Borislav Petkov <[email protected]>
Merge the two consecutive "if (ext_table_size)".
No functionality change.
Signed-off-by Borislav Petkov <[email protected]>
--- arch/x86/kernel/cpu/microcode/intel_lib.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
arch/x86/kernel/cpu/microcode/intel_lib.c +5 -5
@@ -68,6 +68,9 @@ int microcode_sanity_check(void *mc, int print_err) } ext_table_size = total_size - (MC_HEADER_SIZE + data_size);if (ext_table_size) { +u32 ext_table_sum =0 ; +u32 *ext_tablep; +if ((ext_table_size <EXT_HEADER_SIZE ) || ((ext_table_size -EXT_HEADER_SIZE ) %EXT_SIGNATURE_SIZE )) {if (print_err)
@@ -81,12 +84,9 @@ int microcode_sanity_check(void *mc, int print_err)return -EFAULT ; } ext_sigcount = ext_header->count; - } -/* check extended table checksum */ -if (ext_table_size) { -u32 ext_table_sum =0 ; -u32 *ext_tablep = (u32 *)ext_header; +/* check extended table checksum */ + ext_tablep = (u32 *)ext_header; i = ext_table_size /sizeof (u32);while (i--)
Signature · 1 line
2.3.5
tip-bot for Borislav Petkov
merge bot[tip:x86/microcode] x86/microcode/intel: Merge two consecutive if-statements
x86/microcode/intel: Merge two consecutive if-statements
Commit-ID: 7d0161569a3b66aaa01520002c3e5fd7126d071f Gitweb: http://git.kernel.org/tip/7d0161569a3b66aaa01520002c3e5fd7126d071f Author: Borislav Petkov <[email protected]> AuthorDate: Mon, 7 Mar 2016 11:10:02 +0100 Committer: Thomas Gleixner <[email protected]> CommitDate: Tue, 8 Mar 2016 09:08:45 +0100
x86/microcode/intel: Merge two consecutive if-statements
Merge the two consecutive "if (ext_table_size)". No functional change.
Signed-off-by Borislav Petkov <[email protected]>
Signed-off-by Thomas Gleixner <[email protected]>
--- arch/x86/kernel/cpu/microcode/intel_lib.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
arch/x86/kernel/cpu/microcode/intel_lib.c +5 -5
@@ -68,6 +68,9 @@ int microcode_sanity_check(void *mc, int print_err) } ext_table_size = total_size - (MC_HEADER_SIZE + data_size);if (ext_table_size) { +u32 ext_table_sum =0 ; +u32 *ext_tablep; +if ((ext_table_size <EXT_HEADER_SIZE ) || ((ext_table_size -EXT_HEADER_SIZE ) %EXT_SIGNATURE_SIZE )) {if (print_err)
@@ -81,12 +84,9 @@ int microcode_sanity_check(void *mc, int print_err)return -EFAULT ; } ext_sigcount = ext_header->count; - } -/* check extended table checksum */ -if (ext_table_size) { -u32 ext_table_sum =0 ; -u32 *ext_tablep = (u32 *)ext_header; +/* check extended table checksum */ + ext_tablep = (u32 *)ext_header; i = ext_table_size /sizeof (u32);while (i--)
Borislav Petkov
maintainerPATCH 4/5 x86/microcode/intel: Improve microcode sanity-checking error messages
In reply to Borislav PetkovFrom: Borislav Petkov <[email protected]>
Turn them into proper sentences. Add comments tomicrocode_sanity_check() to explain what it does.
Signed-off-by Borislav Petkov <[email protected]>
--- arch/x86/kernel/cpu/microcode/intel_lib.c | 36 ++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 10 deletions(-)
arch/x86/kernel/cpu/microcode/intel_lib.c +26 -10
@@ -57,15 +57,16 @@ int microcode_sanity_check(void *mc, int print_err)if (data_size +MC_HEADER_SIZE > total_size) {if (print_err) -pr_err ("error! Bad data size in microcode data file\n" ); +pr_err ("Error: bad microcode data file size.\n" );return -EINVAL ; }if (mc_header->ldrver !=1 || mc_header->hdrver !=1 ) {if (print_err) -pr_err ("error! Unknown microcode update format\n" ); +pr_err ("Error: invalid/unknown microcode update format.\n" );return -EINVAL ; } + ext_table_size = total_size - (MC_HEADER_SIZE + data_size);if (ext_table_size) {u32 ext_table_sum =0 ;
@@ -74,43 +75,58 @@ int microcode_sanity_check(void *mc, int print_err)if ((ext_table_size <EXT_HEADER_SIZE ) || ((ext_table_size -EXT_HEADER_SIZE ) %EXT_SIGNATURE_SIZE )) {if (print_err) -pr_err ("error! Small exttable size in microcode data file\n" ); +pr_err ("Error: truncated extended signature table.\n" );return -EINVAL ; } + ext_header = mc +MC_HEADER_SIZE + data_size;if (ext_table_size !=exttable_size (ext_header)) {if (print_err) -pr_err ("error! Bad exttable size in microcode data file\n" ); +pr_err ("Error: extended signature table size mismatch.\n" );return -EFAULT ; } + ext_sigcount = ext_header->count; -/* check extended table checksum */ +/* +* Check extended table checksum: the sum of all dwords that +* comprise a valid table must be 0. +*/ ext_tablep = (u32 *)ext_header; i = ext_table_size /sizeof (u32);while (i--) ext_table_sum += ext_tablep[i]; +if (ext_table_sum) {if (print_err) -pr_warn ("aborting, bad extended signature table checksum\n" ); +pr_warn ("Bad extended signature table checksum, aborting.\n" );return -EINVAL ; } } -/* calculate the checksum */ +/* +* Calculate the checksum of update data and header. The checksum of +* valid update data and header including the extended signature table +* must be 0. +*/ orig_sum =0 ; i = (MC_HEADER_SIZE + data_size) /sizeof (u32);while (i--) orig_sum += ((u32 *)mc)[i]; +if (orig_sum) {if (print_err) -pr_err ("aborting, bad checksum\n" ); +pr_err ("Bad microcode data checksum, aborting.\n" );return -EINVAL ; } +if (!ext_table_size)return 0 ; -/* check extended signature checksum */ + +/* +* Check extended signature checksum: 0 => valid. +*/ for (i =0 ; i < ext_sigcount; i++) { ext_sig = (void *)ext_header +EXT_HEADER_SIZE +EXT_SIGNATURE_SIZE * i;
@@ -119,7 +135,7 @@ int microcode_sanity_check(void *mc, int print_err) + (ext_sig->sig + ext_sig->pf + ext_sig->cksum);if (sum) {if (print_err) -pr_err ("aborting, bad checksum\n" ); +pr_err ("Bad extended signature checksum, aborting.\n" );return -EINVAL ; } }
Signature · 1 line
2.3.5
tip-bot for Borislav Petkov
merge bot[tip:x86/microcode] x86/microcode/intel: Improve microcode sanity-checking error messages
x86/microcode/intel: Improve microcode sanity-checking error messages
Commit-ID: 5b46b5e003724547f0c83041cada15f9f496590d Gitweb: http://git.kernel.org/tip/5b46b5e003724547f0c83041cada15f9f496590d Author: Borislav Petkov <[email protected]> AuthorDate: Mon, 7 Mar 2016 11:10:03 +0100 Committer: Thomas Gleixner <[email protected]> CommitDate: Tue, 8 Mar 2016 09:08:45 +0100
x86/microcode/intel: Improve microcode sanity-checking error messages
Turn them into proper sentences. Add comments to microcode_sanity_check() to explain what it does.
Signed-off-by Borislav Petkov <[email protected]>
Signed-off-by Thomas Gleixner <[email protected]>
--- arch/x86/kernel/cpu/microcode/intel_lib.c | 36 ++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 10 deletions(-)
arch/x86/kernel/cpu/microcode/intel_lib.c +26 -10
@@ -57,15 +57,16 @@ int microcode_sanity_check(void *mc, int print_err)if (data_size +MC_HEADER_SIZE > total_size) {if (print_err) -pr_err ("error! Bad data size in microcode data file\n" ); +pr_err ("Error: bad microcode data file size.\n" );return -EINVAL ; }if (mc_header->ldrver !=1 || mc_header->hdrver !=1 ) {if (print_err) -pr_err ("error! Unknown microcode update format\n" ); +pr_err ("Error: invalid/unknown microcode update format.\n" );return -EINVAL ; } + ext_table_size = total_size - (MC_HEADER_SIZE + data_size);if (ext_table_size) {u32 ext_table_sum =0 ;
@@ -74,43 +75,58 @@ int microcode_sanity_check(void *mc, int print_err)if ((ext_table_size <EXT_HEADER_SIZE ) || ((ext_table_size -EXT_HEADER_SIZE ) %EXT_SIGNATURE_SIZE )) {if (print_err) -pr_err ("error! Small exttable size in microcode data file\n" ); +pr_err ("Error: truncated extended signature table.\n" );return -EINVAL ; } + ext_header = mc +MC_HEADER_SIZE + data_size;if (ext_table_size !=exttable_size (ext_header)) {if (print_err) -pr_err ("error! Bad exttable size in microcode data file\n" ); +pr_err ("Error: extended signature table size mismatch.\n" );return -EFAULT ; } + ext_sigcount = ext_header->count; -/* check extended table checksum */ +/* +* Check extended table checksum: the sum of all dwords that +* comprise a valid table must be 0. +*/ ext_tablep = (u32 *)ext_header; i = ext_table_size /sizeof (u32);while (i--) ext_table_sum += ext_tablep[i]; +if (ext_table_sum) {if (print_err) -pr_warn ("aborting, bad extended signature table checksum\n" ); +pr_warn ("Bad extended signature table checksum, aborting.\n" );return -EINVAL ; } } -/* calculate the checksum */ +/* +* Calculate the checksum of update data and header. The checksum of +* valid update data and header including the extended signature table +* must be 0. +*/ orig_sum =0 ; i = (MC_HEADER_SIZE + data_size) /sizeof (u32);while (i--) orig_sum += ((u32 *)mc)[i]; +if (orig_sum) {if (print_err) -pr_err ("aborting, bad checksum\n" ); +pr_err ("Bad microcode data checksum, aborting.\n" );return -EINVAL ; } +if (!ext_table_size)return 0 ; -/* check extended signature checksum */ + +/* +* Check extended signature checksum: 0 => valid. +*/ for (i =0 ; i < ext_sigcount; i++) { ext_sig = (void *)ext_header +EXT_HEADER_SIZE +EXT_SIGNATURE_SIZE * i;
@@ -119,7 +135,7 @@ int microcode_sanity_check(void *mc, int print_err) + (ext_sig->sig + ext_sig->pf + ext_sig->cksum);if (sum) {if (print_err) -pr_err ("aborting, bad checksum\n" ); +pr_err ("Bad extended signature checksum, aborting.\n" );return -EINVAL ; } }
Borislav Petkov
maintainerPATCH 5/5 x86/microcode/intel: Drop orig_sum from ext signature checksum
In reply to Borislav PetkovFrom: Borislav Petkov <[email protected]>
It is 0 because for !0 values we would have exited already.
Signed-off-by Borislav Petkov <[email protected]>
--- arch/x86/kernel/cpu/microcode/intel_lib.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
arch/x86/kernel/cpu/microcode/intel_lib.c +3 -3
@@ -130,9 +130,9 @@ int microcode_sanity_check(void *mc, int print_err)for (i =0 ; i < ext_sigcount; i++) { ext_sig = (void *)ext_header +EXT_HEADER_SIZE +EXT_SIGNATURE_SIZE * i; - sum = orig_sum - - (mc_header->sig + mc_header->pf + mc_header->cksum) - + (ext_sig->sig + ext_sig->pf + ext_sig->cksum); + + sum = (mc_header->sig + mc_header->pf + mc_header->cksum) - + (ext_sig->sig + ext_sig->pf + ext_sig->cksum);if (sum) {if (print_err)pr_err ("Bad extended signature checksum, aborting.\n" );
Signature · 1 line
2.3.5
tip-bot for Borislav Petkov
merge bot[tip:x86/microcode] x86/microcode/intel: Drop orig_sum from ext signature checksum
x86/microcode/intel: Drop orig_sum from ext signature checksum
Commit-ID: 4ace2e7a48ab426eaa9745ace4c50c6a7adb3992 Gitweb: http://git.kernel.org/tip/4ace2e7a48ab426eaa9745ace4c50c6a7adb3992 Author: Borislav Petkov <[email protected]> AuthorDate: Mon, 7 Mar 2016 11:10:04 +0100 Committer: Thomas Gleixner <[email protected]> CommitDate: Tue, 8 Mar 2016 09:08:45 +0100
x86/microcode/intel: Drop orig_sum from ext signature checksum
It is 0 because for !0 values we would have exited already.
Signed-off-by Borislav Petkov <[email protected]>
Signed-off-by Thomas Gleixner <[email protected]>
--- arch/x86/kernel/cpu/microcode/intel_lib.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
arch/x86/kernel/cpu/microcode/intel_lib.c +3 -3
@@ -130,9 +130,9 @@ int microcode_sanity_check(void *mc, int print_err)for (i =0 ; i < ext_sigcount; i++) { ext_sig = (void *)ext_header +EXT_HEADER_SIZE +EXT_SIGNATURE_SIZE * i; - sum = orig_sum - - (mc_header->sig + mc_header->pf + mc_header->cksum) - + (ext_sig->sig + ext_sig->pf + ext_sig->cksum); + + sum = (mc_header->sig + mc_header->pf + mc_header->cksum) - + (ext_sig->sig + ext_sig->pf + ext_sig->cksum);if (sum) {if (print_err)pr_err ("Bad extended signature checksum, aborting.\n" );