[U-Boot] [PATCH v2 07/13] x86: Fix signed shift overflow in MSR_IA32_APICBASE_BASE

[Eugeniu Rosca]

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

Thanks,
Eugeniu.