From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S2992565AbcB0SXe (ORCPT <rfc822;w@1wt.eu>);
	Sat, 27 Feb 2016 13:23:34 -0500
Received: from mail-wm0-f54.google.com ([74.125.82.54]:34737 "EHLO
	mail-wm0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1756566AbcB0SXd (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Sat, 27 Feb 2016 13:23:33 -0500
Date: Sat, 27 Feb 2016 18:23:29 +0000
From: Chris Bainbridge <chris.bainbridge@gmail.com>
To: Borislav Petkov <bp@alien8.de>
Cc: x86@kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] x86/microcode: Change checksum to u32
Message-ID: <20160227182329.GA4947@localhost>
References: <1456570907-5344-1-git-send-email-chris.bainbridge@gmail.com>
 <20160227175118.GC5261@pd.tnic>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20160227175118.GC5261@pd.tnic>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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++;
        }