Hi there,
On Tue, Aug 28, 2018 at 08:42:01AM +0200, Eugeniu Rosca wrote:
Hi Bin,
cc: Masahiro, Andrey
On Tue, Aug 28, 2018 at 10:05:51AM +0800, Bin Meng wrote:
Hi Eugeniu,
On Mon, Aug 27, 2018 at 7:19 AM Eugeniu Rosca roscaeugeniu@gmail.com wrote:
Fix the following UBSAN report:
UBSAN: Undefined behaviour in arch/x86/cpu/lapic.c:73:14 left shift of 1048575 by 12 places cannot be represented in type 'int' ======================================================================
Steps to reproduce the above:
- echo CONFIG_UBSAN=y >> configs/qemu-x86_defconfig
- make ARCH=x86 qemu-x86_defconfig all
- qemu-system-i386 --version QEMU emulator version 2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.31)
- qemu-system-i386 --nographic -bios u-boot.rom
Fixes: 98568f0fa96b ("x86: Import MSR/MTRR code from Linux") Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com
Changes in v2:
- None. Newly pushed.
arch/x86/include/asm/msr-index.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h index 9c1dbe61d596..d8b7b8013c74 100644 --- a/arch/x86/include/asm/msr-index.h +++ b/arch/x86/include/asm/msr-index.h @@ -370,7 +370,7 @@ #define MSR_IA32_APICBASE 0x0000001b #define MSR_IA32_APICBASE_BSP (1<<8) #define MSR_IA32_APICBASE_ENABLE (1<<11) -#define MSR_IA32_APICBASE_BASE (0xfffff<<12) +#define MSR_IA32_APICBASE_BASE (0xfffffUL << 12)
I don't understand why such warnings is emitted: "left shift of 1048575 by 12 places cannot be represented in type 'int'"
Compilers don't complain this code and Linux kernel has the same definition here.
I wrote a basic kernel module printing the result of "(0xfffff << 12)" and kernel UBSAN doesn't complain indeed.
I started to compare the compiler flags between Linux and U-Boot and nailed down empirically that Linux UBSAN warning is inhibited by the -fno-strict-overflow gcc option, introduced in Linux commit [1]. The latter actually replaces another gcc option -fwrapv, introduced in [2].
Any of the two flags makes the UBSAN error vanish in the kernel. Neither of the two flags is used in U-Boot.
I am in the process of browsing some documentation related to -fwrapv and -fno-strict-overflow (e.g. [3]). Please, feel free to share any thoughts and/or cc anybody who might have dealt with these topics in the past. I will come back with more feedback later.
[1] v2.6.31 commit a137802ee839 ("Don't use '-fwrapv' compiler option: it's buggy in gcc-4.1.x") [2] v2.6.29 commit 68df3755e383 ("Add '-fwrapv' to gcc CFLAGS") [3] https://www.airs.com/blog/archives/120
Regards, Bin
Just wanted to let you know that coreboot folks are going through similar discussions in [1]. Also, experimenting with various gcc versions and flags in my spare time, I collected some evidence [2] showing that the behavior of GCC UBSAN (-fsanitize=undefined & friends) may differ a lot depending on the gcc version and below flags (none used by U-Boot, but some used in Linux kernel): -fwrapv -fstrict-overflow -fno-strict-overflow
Checking how -fno-strict-overflow and -fwrapv compare to each other (since they seem to accomplish similar goals according to many sources), I've used the sample app from [3] to see how gcc handles signed integer wraparound depending on gcc version, flags, optimization level and on whether UBSAN is enabled or not. The variance/inconsistency of the results [4] is very high in my opinion.
One clear conclusion of [4] is that questions like why gcc UBSAN complains in U-Boot but not in the Kernel require knowing at least the parameters tracked in [4] (and maybe more).
[1] https://mail.coreboot.org/pipermail/coreboot/2018-February/086146.html [2] UBSAN behavior (printing 1 << 31) is highly dependent on gcc version and flags
+----------------------+-------------+-----+ | gcc flags | gcc version | UB? | |----------------------|-------------|-----| | | gcc-4.9.4 | - | | -fsanitize=undefined | gcc-5.5.0 | y | | | gcc-7.3.0 | y | | | gcc-8.1.0 | y | +------------------------------------------+ | | gcc-4.9.4 | - | | -fsanitize=undefined | gcc-5.5.0 | y | | -fstrict-overflow | gcc-7.3.0 | y | | | gcc-8.1.0 | y | +------------------------------------------+ | | gcc-4.9.4 | - | | -fsanitize=undefined | gcc-5.5.0 | y | | -fno-strict-overflow | gcc-7.3.0 | y | | | gcc-8.1.0 | - | +------------------------------------------+ | | gcc-4.9.4 | - | | -fsanitize=undefined | gcc-5.5.0 | y | | -fwrapv | gcc-7.3.0 | - | | | gcc-8.1.0 | - | +----------------------+-------------+-----+
[3] http://thiemonagel.de/2010/01/signed-integer-overflow/
[4] Wraparound [3] dependency on gcc version, flags, optimization level and -fsanitize=undefined
| gcc flags | gcc | Wrapped? (UB!) | |-------------------------|-------|------|-----|-----|-----|-----| | | | -O0 | -O1 | -O2 | -O3 | -Os | | | 4.9.4 | y/y! | y/y | n/n | n/n | n/n | | none | 5.5.0 | y/y! | y/y | n/y | n/y | n/y | | (/-fsanitize=undefined) | 7.3.0 | y/y! | y/y | n/y | n/y | n/y | | | 8.1.0 | n/n | n/n | n/n | n/n | n/n | +----------------------------------------------------------------+ | | 4.9.4 | n/n | n/n | n/n | n/n | n/n | | -fstrict-overflow | 5.5.0 | n/y! | n/y | n/y | n/y | n/y | | (/-fsanitize=undefined) | 7.3.0 | n/y! | n/y | n/y | n/y | n/y | | | 8.1.0 | n/n | n/n | n/n | n/n | n/n | +----------------------------------------------------------------+ | | 4.9.4 | y/y! | y/y | y/y | y/y | y/y | | -fno-strict-overflow | 5.5.0 | y/y! | y/y | y/y | y/y | y/y | | (/-fsanitize=undefined) | 7.3.0 | y/y! | y/y | y/y | y/y | y/y | | | 8.1.0 | y/y | y/y | y/y | y/y | y/y | +----------------------------------------------------------------+ | | 4.9.4 | y/y | y/y | y/y | y/y | y/y | | -fwrapv | 5.5.0 | y/y | y/y | y/y | y/y | y/y | | (/-fsanitize=undefined) | 7.3.0 | y/y | y/y | y/y | y/y | y/y | | | 8.1.0 | y/y | y/y | y/y | y/y | y/y | +----------------------------------------------------------------+
Comments/suggestions appreciated.
Best regards, Eugeniu.