[patch -next] x86, microcode, AMD: signedness bug in generic_load_microcode()

[patch -next] x86, microcode, AMD: signedness bug in generic_load_microcode()

[Dan Carpenter]

install_equiv_cpu_table() returns type int.  It uses negative error
codes so using an unsigned type breaks the error handling.

Signed-off-by: Dan Carpenter <error27@gmail.com>

diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index 9fb8405..c561038 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -246,7 +246,7 @@ generic_load_microcode(int cpu, const u8 *data, size_t size)
 	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
 	struct microcode_header_amd *mc_hdr = NULL;
 	unsigned int mc_size, leftover;
-	unsigned long offset;
+	int offset;
 	const u8 *ucode_ptr = data;
 	void *new_mc = NULL;
 	unsigned int new_rev = uci->cpu_sig.rev;

Re: [patch -next] x86, microcode, AMD: signedness bug in generic_load_microcode()

[Borislav Petkov]

On Fri, Feb 18, 2011 at 04:17:16AM -0500, Dan Carpenter wrote:
> install_equiv_cpu_table() returns type int.  It uses negative error
> codes so using an unsigned type breaks the error handling.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>

Yes, my bad, sorry.

Acked-by: Borislav Petkov <borislav.petkov@amd.com>

Ingo, this should go ontop of tip/x86/microcode please, for it fixes
10de52d6655e ("x86, microcode, AMD: Simplify install_equiv_cpu_table").

Thanks a bunch.

-- 
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

Re: [patch -next] x86, microcode, AMD: signedness bug in generic_load_microcode()

[Ingo Molnar]

* Dan Carpenter <error27@gmail.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?

Thanks,

	Ingo

Re: [patch -next] x86, microcode, AMD: signedness bug in generic_load_microcode()

[Dan Carpenter]

On Sun, Feb 20, 2011 at 02:02:14PM +0100, Ingo Molnar wrote:
> 
> * Dan Carpenter <error27@gmail.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?
> 

It's a smatch warning.

arch/x86/kernel/microcode_amd.c +256 generic_load_microcode(12)
	warn: unsigned 'offset' is never less than zero.

regards,
dan carpenter

Re: [patch -next] x86, microcode, AMD: signedness bug in generic_load_microcode()

[Borislav Petkov]

On Sun, Feb 20, 2011 at 08:02:14AM -0500, Ingo Molnar wrote:
> 
> * Dan Carpenter <error27@gmail.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

Re: [patch -next] x86, microcode, AMD: signedness bug in generic_load_microcode()

[Matthew Wilcox]

On Sun, Feb 20, 2011 at 03:14:52PM +0100, Borislav Petkov wrote:
> 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.

No, that's not what's going on.  GCC _is_ truncating to a byte, 0xa5,
whether it's signed or not.  Then at the time of the call to printf,
the 0xa5 is cast to int.  If the char is signed, 0xa5 is sign-extended;
if unsigned, it's zero-extended.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [patch -next] x86, microcode, AMD: signedness bug in generic_load_microcode()

[Borislav Petkov]

On Sun, Feb 20, 2011 at 10:50:11AM -0700, Matthew Wilcox wrote:
> On Sun, Feb 20, 2011 at 03:14:52PM +0100, Borislav Petkov wrote:
> > 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.
> 
> No, that's not what's going on.  GCC _is_ truncating to a byte, 0xa5,
> whether it's signed or not.  Then at the time of the call to printf,
> the 0xa5 is cast to int.  If the char is signed, 0xa5 is sign-extended;
> if unsigned, it's zero-extended.

Yes, you're right, I missed the fact that printf does convert its
arguments based on the format string. I should've done

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

for chars.

Thanks.

-- 
Regards/Gruss,
    Boris.

Re: [patch -next] x86, microcode, AMD: signedness bug in generic_load_microcode()

[Matthew Wilcox]

On Sun, Feb 20, 2011 at 07:08:45PM +0100, Borislav Petkov wrote:
> On Sun, Feb 20, 2011 at 10:50:11AM -0700, Matthew Wilcox wrote:
> > No, that's not what's going on.  GCC _is_ truncating to a byte, 0xa5,
> > whether it's signed or not.  Then at the time of the call to printf,
> > the 0xa5 is cast to int.  If the char is signed, 0xa5 is sign-extended;
> > if unsigned, it's zero-extended.
> 
> Yes, you're right, I missed the fact that printf does convert its
> arguments based on the format string. I should've done
> 
> 	printf("ret = 0x%hhx\n", ret);

GCC's special treatment of the printf format string is only in the
gneration of warnings.  It doesn't promote differently based on the
format string.

You need to look at 6.5.2.2, parts 6 and 7.  Part 7 says:

	The ellipsis notation in a function prototype declarator causes
	argument type conversion to stop after the last declared
	parameter. The default argument promotions are performed on
	trailing arguments.

And part 6 describes the default argument promotions:

	If the expression that denotes the called function has a type that
	does not include a prototype, the integer promotions are performed
	on each argument, and arguments that have type float are promoted
	to double. These are called the default argument promotions.

So passing a char to printf will cause it to be promoted to int, no
matter what the format string says.  All the format string will do is
change how it's printed.  Probably by casting it back to a char :-)

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [patch -next] x86, microcode, AMD: signedness bug in generic_load_microcode()

[Borislav Petkov]

On Sun, Feb 20, 2011 at 11:42:17AM -0700, Matthew Wilcox wrote:
> On Sun, Feb 20, 2011 at 07:08:45PM +0100, Borislav Petkov wrote:
> > On Sun, Feb 20, 2011 at 10:50:11AM -0700, Matthew Wilcox wrote:
> > > No, that's not what's going on.  GCC _is_ truncating to a byte, 0xa5,
> > > whether it's signed or not.  Then at the time of the call to printf,
> > > the 0xa5 is cast to int.  If the char is signed, 0xa5 is sign-extended;
> > > if unsigned, it's zero-extended.
> > 
> > Yes, you're right, I missed the fact that printf does convert its
> > arguments based on the format string. I should've done
> > 
> > 	printf("ret = 0x%hhx\n", ret);
> 
> GCC's special treatment of the printf format string is only in the
> gneration of warnings.  It doesn't promote differently based on the
> format string.
> 
> You need to look at 6.5.2.2, parts 6 and 7.  Part 7 says:
> 
> 	The ellipsis notation in a function prototype declarator causes
> 	argument type conversion to stop after the last declared
> 	parameter. The default argument promotions are performed on
> 	trailing arguments.
> 
> And part 6 describes the default argument promotions:
> 
> 	If the expression that denotes the called function has a type that
> 	does not include a prototype, the integer promotions are performed
> 	on each argument, and arguments that have type float are promoted
> 	to double. These are called the default argument promotions.
> 
> So passing a char to printf will cause it to be promoted to int, no
> matter what the format string says.  All the format string will do is
> change how it's printed.  Probably by casting it back to a char :-)

Ha, I see, maybe I should've seen this earlier if I would've looked at
the asm, as grandma always taught me:

	char ret = f();
	...
	printf("ret = 0x%hhx\n", ret);

translates to:

00000000004004e4 <f>:
  4004e4:       55                      push   %rbp
  4004e5:       48 89 e5                mov    %rsp,%rbp
  4004e8:       b8 a5 a5 a5 a5          mov    $0xa5a5a5a5,%eax
  4004ed:       c9                      leaveq 
  4004ee:       c3                      retq   

00000000004004ef <main>:
  4004ef:       55                      push   %rbp
  4004f0:       48 89 e5                mov    %rsp,%rbp
  4004f3:       48 83 ec 10             sub    $0x10,%rsp
  4004f7:       b8 00 00 00 00          mov    $0x0,%eax
  4004fc:       e8 e3 ff ff ff          callq  4004e4 <f>
  400501:       88 45 ff                mov    %al,-0x1(%rbp)
  400504:       0f be 55 ff             movsbl -0x1(%rbp),%edx		<--- mov 8-bit reg/mem with sign extension to a 32-bit reg
  400508:       b8 1c 06 40 00          mov    $0x40061c,%eax
  40050d:       89 d6                   mov    %edx,%esi
  40050f:       48 89 c7                mov    %rax,%rdi
  400512:       b8 00 00 00 00          mov    $0x0,%eax
  400517:       e8 c4 fe ff ff          callq  4003e0 <printf@plt>


vs the unsigned char case

	unsigned char ret = f();
	...
	printf("ret = 0x%hhx\n", ret);

=>
  ...

  400501:       88 45 ff                mov    %al,-0x1(%rbp)
  400504:       0f b6 55 ff             movzbl -0x1(%rbp),%edx		<--- mov 8-bit reg/mem with zero-extension to a 32-bit reg
  400508:       b8 1c 06 40 00          mov    $0x40061c,%eax
  40050d:       89 d6                   mov    %edx,%esi
  40050f:       48 89 c7                mov    %rax,%rdi
  400512:       b8 00 00 00 00          mov    $0x0,%eax
  400517:       e8 c4 fe ff ff          callq  4003e0 <printf@plt>


Thanks for enlightening me! :)

-- 
Regards/Gruss,
    Boris.

[tip:x86/microcode] x86, microcode, AMD: Fix signedness bug in generic_load_microcode()

[tip-bot for Dan Carpenter]

Commit-ID:  1396fa9cd2e34669253b7ca8c75f12103481f71c
Gitweb:     http://git.kernel.org/tip/1396fa9cd2e34669253b7ca8c75f12103481f71c
Author:     Dan Carpenter <error27@gmail.com>
AuthorDate: Fri, 18 Feb 2011 12:17:16 +0300
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Sun, 20 Feb 2011 14:01:32 +0100

x86, microcode, AMD: Fix signedness bug in generic_load_microcode()

install_equiv_cpu_table() returns type int.  It uses negative
error codes so using an unsigned type breaks the error handling.

Signed-off-by: Dan Carpenter <error27@gmail.com>
Acked-by: Borislav Petkov <borislav.petkov@amd.com>
Cc: open list:AMD MICROCODE UPD... <amd64-microcode@amd64.org>
Cc: Andreas Herrmann <andreas.herrmann3@amd.com>
LKML-Reference: <20110218091716.GA4384@bicker>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/microcode_amd.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index 9fb8405..c561038 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -246,7 +246,7 @@ generic_load_microcode(int cpu, const u8 *data, size_t size)
 	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
 	struct microcode_header_amd *mc_hdr = NULL;
 	unsigned int mc_size, leftover;
-	unsigned long offset;
+	int offset;
 	const u8 *ucode_ptr = data;
 	void *new_mc = NULL;
 	unsigned int new_rev = uci->cpu_sig.rev;