[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