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]>
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;
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 <[email protected]>
Yes, my bad, sorry.
Acked-by: Borislav Petkov <[email protected]>
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
* Dan Carpenter <[email protected]> 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
Commit-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.
Signed-off-by: Dan Carpenter <[email protected]>
Acked-by: Borislav Petkov <[email protected]>
Cc: open list:AMD MICROCODE UPD... <[email protected]>
Cc: Andreas Herrmann <[email protected]>
LKML-Reference: <20110218091716.GA4384@bicker>
Signed-off-by: Ingo Molnar <[email protected]>
---
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;
On Sun, Feb 20, 2011 at 02:02:14PM +0100, Ingo Molnar wrote:
>
> * Dan Carpenter <[email protected]> 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
On Sun, Feb 20, 2011 at 08:02:14AM -0500, Ingo Molnar wrote:
>
> * Dan Carpenter <[email protected]> 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
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."
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.
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."
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.