linux-kernel - Re: [patch -next] x86, microcode, AMD: signedness bug in generic_load

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]

Date:	Sun, 20 Feb 2011 15:14:52 +0100
From:	Borislav Petkov <bp@...64.org>
To:	Ingo Molnar <mingo@...e.hu>
Cc:	Dan Carpenter <error27@...il.com>,
	"Herrmann3, Andreas" <Andreas.Herrmann3@....com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	"maintainer:X86 ARCHITECTURE..." <x86@...nel.org>,
	"open list:AMD MICROCODE UPD..." <amd64-microcode@...64.org>,
	open list <linux-kernel@...r.kernel.org>,
	"kernel-janitors@...r.kernel.org" <kernel-janitors@...r.kernel.org>
Subject: Re: [patch -next] x86, microcode, AMD: signedness bug in
 generic_load_microcode()

On Sun, Feb 20, 2011 at 08:02:14AM -0500, Ingo Molnar wrote:
> 
> * Dan Carpenter <error27@...il.com> wrote:
> 
> > install_equiv_cpu_table() returns type int.  It uses negative error
> > codes so using an unsigned type breaks the error handling.
> 
> How did you notice this btw - did GCC throw a warning?

Was wondering about the same thing too, I didn't see any warning during
my testing. Can GCC even check whether return types of functions are
"compatible" when assigned to variables?

--
#include <stdio.h>

int f() {
        return 0xa5a5a5a5;
}

int main()
{

        char ret = f();

        printf("ret = 0x%016x\n", ret);

        return 0;
} 
--

doesn't cause a warning and prints a sign extended 0x00000000ffffffa5
which is cast to the return type of the function. If ret is an unsigned
char, then we return a 0x00000000000000a5.

I found something about it in the C99 standard¹, section "6.5.16.1 Simple
assignment":

4.  EXAMPLE 1       In the program fragment

           int f(void);
           char c;
           /* ... */
           if ((c = f()) == -1)
                    /* ... */

the int value returned by the function may be truncated when stored in
the char, and then converted back to int width prior to the comparison.
In an implementation in which ‘‘plain’’ char has the same range
of values as unsigned char (and char is narrower than int), the result
of the conversion cannot be negative, so the operands of the comparison
can never compare equal. Therefore, for full portability, the variable c
should be declared as int."

so the whole "... may be truncated.. " could mean a lot of things. From
my example above, gcc does truncate the int return type to a byte-sized
char only when they differ in signedness.

In the original case where we assign an int return type of a function
(smaller size) to an unsigned long (greater size), the first gets
converted to an unsigned long without a warning because the unsigned
long is large enough to contain the int and so it is assumed the user
knows what he/she's doing.

However, the unsigned long type is later checked for < 0 which could
never hit so I guess this could be warned for but I'm not sure whether
this would make sense in all cases.

Wait a minute, there _actually_ is a gcc '-Wconversion' option which is
_very_ noisy but does catch it:

arch/x86/kernel/microcode_amd.c: In function ‘generic_load_microcode’:
arch/x86/kernel/microcode_amd.c:255: warning: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result

Come to think of it, it might make sense to be able to enable it when doing
debug builds as a way to do some more checking on your code when prepping
patches, maybe something like this:

make W=1 arch/x86/kernel/microcode_amd.o

which enables all gcc warnings for that specific file only so that
you could verify whether the warnings are valid and fix them if so.
Something similar to perf's EXTRA_WARNINGS.

Let me see whether this can be easily done...

Thanks.

¹ http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1336.pdf

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/