From: Eugeniu Rosca <roscaeugeniu@gmail.com>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH v2 07/13] x86: Fix signed shift overflow in MSR_IA32_APICBASE_BASE
Date: Sat, 1 Sep 2018 12:59:09 +0200 [thread overview]
Message-ID: <20180901105909.GA17126@x230> (raw)
In-Reply-To: <20180828064201.GA4030@x230>
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.
next prev parent reply other threads:[~2018-09-01 10:59 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-26 23:13 [U-Boot] [PATCH v2 00/13] Import Undefined Behavior Sanitizer Eugeniu Rosca
2018-08-26 23:13 ` [U-Boot] [PATCH v2 01/13] UBSAN: run-time undefined behavior sanity checker Eugeniu Rosca
2018-08-27 14:13 ` Tom Rini
2018-08-26 23:13 ` [U-Boot] [PATCH v2 02/13] mmc: Fix signed shift overflow Eugeniu Rosca
2018-08-26 23:13 ` [U-Boot] [PATCH v2 03/13] armv8: mmu: " Eugeniu Rosca
2018-08-27 14:13 ` Tom Rini
2018-08-26 23:13 ` [U-Boot] [PATCH v2 04/13] pinctrl: renesas: " Eugeniu Rosca
2018-08-26 23:13 ` [U-Boot] [PATCH v2 05/13] net: phy: " Eugeniu Rosca
2018-08-26 23:13 ` [U-Boot] [PATCH v2 06/13] net: ravb: " Eugeniu Rosca
2018-08-26 23:22 ` Marek Vasut
2018-08-27 20:24 ` Eugeniu Rosca
2018-08-27 23:55 ` Marek Vasut
2018-08-26 23:13 ` [U-Boot] [PATCH v2 07/13] x86: Fix signed shift overflow in MSR_IA32_APICBASE_BASE Eugeniu Rosca
2018-08-28 2:05 ` Bin Meng
2018-08-28 6:42 ` Eugeniu Rosca
2018-09-01 10:59 ` Eugeniu Rosca [this message]
2018-09-04 4:00 ` Bin Meng
2018-09-16 18:46 ` Eugeniu Rosca
2018-09-22 23:10 ` Eugeniu Rosca
2018-09-25 2:06 ` Bin Meng
2018-10-09 0:22 ` Eugeniu Rosca
2018-08-28 8:14 ` Andy Shevchenko
2018-08-26 23:13 ` [U-Boot] [PATCH v2 08/13] disk: part_dos: Fix signed shift overflow Eugeniu Rosca
2018-08-26 23:13 ` [U-Boot] [PATCH v2 09/13] common.h: Fix signed shift overflow in cpumask_next() Eugeniu Rosca
2018-08-26 23:13 ` [U-Boot] [PATCH v2 10/13] mmc: Fix read-past-end-of-array Eugeniu Rosca
2018-08-26 23:13 ` [U-Boot] [PATCH v2 11/13] hashtable: Fix zero-sized array Eugeniu Rosca
2018-08-27 14:13 ` Tom Rini
2018-08-26 23:13 ` [U-Boot] [PATCH v2 12/13] input: " Eugeniu Rosca
2018-08-27 14:13 ` Tom Rini
2018-08-26 23:13 ` [U-Boot] [PATCH v2 13/13] configs: sandbox*: Enable UBSAN Eugeniu Rosca
2018-08-30 2:51 ` Simon Glass
2018-09-17 21:10 ` Eugeniu Rosca
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180901105909.GA17126@x230 \
--to=roscaeugeniu@gmail.com \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox