x86, microcode, AMD: signedness bug in generic_load_microcode()
Subsystems (2)
PATCH 1/1 x86, microcode, AMD: 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.
arch/x86/kernel/microcode_amd.c +1 -1
@@ -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;
Quoted 4 lines · 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 <[email protected]>
Yes, my bad, sorry.
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.
Signature · 8 lines
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
Quoted 2 lines · Dan Carpenter
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
[tip:x86/microcode] x86, microcode, AMD: Fix signedness bug in generic_load_microcode()
In reply to Dan CarpenterCommit-ID: 1396fa9cd2e34669253b7ca8c75f12103481f71c Gitweb: http://git.kernel.org/tip/1396fa9cd2e34669253b7ca8c75f12103481f71c Author: Dan Carpenter <[email protected]> AuthorDate: Fri, 18 Feb 2011 12:17:16 +0300 Committer: Ingo Molnar <[email protected]> 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.
LKML-Reference: <20110218091716.GA4384@bicker>
--- arch/x86/kernel/microcode_amd.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
arch/x86/kernel/microcode_amd.c +1 -1
@@ -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;
Quoted 8 lines · Ingo Molnar
Quoted 2 lines · Dan Carpenter
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
Quoted 7 lines · Ingo Molnar
Quoted 2 lines · Dan Carpenter
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
Signature · 8 lines
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
Quoted 38 lines · Borislav Petkov
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
0x00000000ffffffa5which is cast to the return type of the function. If ret is an unsigned char, then we return a0x00000000000000a5.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."
Quoted 3 lines
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.
Signature · 4 lines
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."
Borislav Petkov
maintainerQuoted 39 lines · Matthew Wilcox
Quoted 38 lines · Borislav Petkov
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
0x00000000ffffffa5which is cast to the return type of the function. If ret is an unsigned char, then we return a0x00000000000000a5.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."
Quoted 8 lines
Quoted 3 lines
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, the0xa5is cast to int. If the char is signed,0xa5is 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.
Signature · 2 lines
Regards/Gruss,
Boris.Quoted 10 lines · Borislav Petkov
Quoted 4 lines · Matthew Wilcox
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, the0xa5is cast to int. If the char is signed,0xa5is 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 :-)
Signature · 4 lines
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."
Borislav Petkov
maintainerQuoted 25 lines · Matthew Wilcox
Quoted 10 lines · Borislav Petkov
Quoted 4 lines · Matthew Wilcox
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, the0xa5is cast to int. If the char is signed,0xa5is 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:
Quoted 8 lines
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! :)
Signature · 2 lines
Regards/Gruss,
Boris.