[U-Boot] [PATCH v2 07/13] x86: Fix signed shift overflow in MSR_IA32_APICBASE_BASE
Eugeniu Rosca
roscaeugeniu at gmail.com
Sat Sep 1 10:59:09 UTC 2018
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 at 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 at 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.
More information about the U-Boot
mailing list