linux-kernel - Re: [PATCH] x86/microcode: Change checksum to u32

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160227182329.GA4947@localhost>
Date:	Sat, 27 Feb 2016 18:23:29 +0000
From:	Chris Bainbridge <chris.bainbridge@...il.com>
To:	Borislav Petkov <bp@...en8.de>
Cc:	x86@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] x86/microcode: Change checksum to u32

On Sat, Feb 27, 2016 at 06:51:18PM +0100, Borislav Petkov wrote:
> On Sat, Feb 27, 2016 at 11:01:47AM +0000, Chris Bainbridge wrote:
> > Checksum should be unsigned 32-bit otherwise the calculation overflows
> > resulting 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'
> 
> So I've been staring at this error message for a while now and I'm
> having hard time understanding what it is telling me. Let's look at all
> three ints:
> 
> * sum is computed here:
> 
>                 sum = orig_sum
>                         - (mc_header->sig + mc_header->pf + mc_header->cksum)
>                         + (ext_sig->sig + ext_sig->pf + ext_sig->cksum);
> 
> and checks mc_header against each extended signature. If it overflows, we abort:
> 
>                 if (sum) {
>                         if (print_err)
>                                 pr_err("aborting, bad checksum\n");
> 
> 
> * orig_sum in the above sum can only be 0:
> 
>         if (orig_sum) {
>                 if (print_err)
>                         pr_err("aborting, bad checksum\n");
>                 return -EINVAL;
> 
> and it adds a bunch of integers which can overflow, sure, but if it
> overflows, we exit early.
> 
> * and
> 
> 	ext_sigcount = ext_header->count;
> 
> so it is some count of extended signatures.
> 
> So what is ubsan complaining about?

        /* calculate the checksum */
        orig_sum = 0;
        i = (MC_HEADER_SIZE + data_size) / DWSIZE;
        while (i--)
                orig_sum += ((int *)mc)[i];

The checksum is the additive sum of all the 32-bit values, but 32-bit
addition overflow is undefined for signed ints. For comparison the
checksum function from iucode-tool uses u32:

intel_ucode_status_t intel_ucode_check_microcode(const void * const uc,
int strict)
{
	...

        uint32_t sum, orig_sum;
        unsigned int i;
        uint32_t *p;

	...

        /* Calculate the checksum.  We exclude the extended table as it
         * also has to have a zero checksum, in order to get better
         * coverage */
        orig_sum = 0;
        i = (INTEL_UC_V1_HEADER_SIZE + data_size) / sizeof(uint32_t);
        p = (uint32_t *)uc;
        while (i--) {
                orig_sum  += *p;
                p++;
        }