linux-kernel - Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170302230558.bhmo4q62v7tqjl2n@treble>
Date:   Thu, 2 Mar 2017 17:05:58 -0600
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Denys Vlasenko <dvlasenk@...hat.com>
Subject: Re: [PATCH] [RFC] x86: avoid -mtune=atom for objtool warnings

On Thu, Mar 02, 2017 at 11:49:49PM +0100, Arnd Bergmann wrote:
> On Thu, Mar 2, 2017 at 2:03 AM, Josh Poimboeuf <jpoimboe@...hat.com> wrote:
> > On Wed, Mar 01, 2017 at 11:42:54PM +0100, Arnd Bergmann wrote:
> >> On Wed, Mar 1, 2017 at 5:53 PM, Josh Poimboeuf <jpoimboe@...hat.com> wrote:
> >> > On Wed, Mar 01, 2017 at 04:27:29PM +0100, Arnd Bergmann wrote:
> >>
> >> > I see no apparent reason for the ud2.
> >>
> >> It's the possible division by zero. This change would avoid the ud2:
> >>
> >> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> >> index db8e8b40569d..a2b09c518225 100644
> >> --- a/drivers/i2c/busses/i2c-img-scb.c
> >> +++ b/drivers/i2c/busses/i2c-img-scb.c
> >> @@ -1196,6 +1196,8 @@ static int img_i2c_init(struct img_i2c *i2c)
> >>         clk_khz /= prescale;
> >>
> >>         /* Setup the clock increment value */
> >> +       if (clk_khz < 1)
> >> +               clk_khz = 1;
> >>         inc = (256 * 16 * bitrate_khz) / clk_khz;
> >>
> >>         /*
> >
> > Ok, I see what gcc is doing.
> >
> >         clk_khz = clk_get_rate(i2c->scb_clk) / 1000;
> >         ...
> >         inc = (256 * 16 * bitrate_khz) / clk_khz;
> >
> > Because CONFIG_HAVE_CLK isn't set, clk_get_rate() returns 0, which means
> > clk_khz is always zero, so the last statement *always* results in a
> > divide-by-zero.  So that looks like a bug in the code.
> >
> > However, I'm baffled by how gcc handles it.  Instead of:
> >
> >   a) reporting a compile-time warning/error; or
> >
> >   b) letting the #DE (divide error) exception happen;
> >
> > it inserts a 'ud2', resulting in a #UD (invalid opcode).  Why?!?
> 
> Just FYI, I found another one like this:
> 
> 0000000000000000 <hibvt_pwm_get_state>:
>    0:   e8 00 00 00 00          callq  5 <hibvt_pwm_get_state+0x5>
>                         1: R_X86_64_PC32        __fentry__-0x4
>    5:   8b 46 10                mov    0x10(%rsi),%eax
>    8:   55                      push   %rbp
>    9:   48 89 e5                mov    %rsp,%rbp
>    c:   c1 e0 05                shl    $0x5,%eax
>    f:   48 03 47 48             add    0x48(%rdi),%rax
>   13:   8b 00                   mov    (%rax),%eax
>   15:   0f 0b                   ud2
>   17:   66 0f 1f 84 00 00 00    nopw   0x0(%rax,%rax,1)
>   1e:   00 00
> 
> static inline unsigned long clk_get_rate(struct clk *clk)
> {
>         return 0;
> }
> 
> static void hibvt_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>                                 struct pwm_state *state)
> {
>         struct hibvt_pwm_chip *hi_pwm_chip = to_hibvt_pwm_chip(chip);
>         void __iomem *base;
>         u32 freq, value;
> 
>         freq = div_u64(clk_get_rate(hi_pwm_chip->clk), 1000000);
>         base = hi_pwm_chip->base;
> 
>         value = readl(base + PWM_CFG0_ADDR(pwm->hwpwm));
>         state->period = div_u64(value * 1000, freq);
> 
>         value = readl(base + PWM_CFG1_ADDR(pwm->hwpwm));
>         state->duty_cycle = div_u64(value * 1000, freq);
> 
>         value = readl(base + PWM_CTRL_ADDR(pwm->hwpwm));
>         state->enabled = (PWM_ENABLE_MASK & value);
> }

I assume '-Wdiv-by-zero' is enabled and gcc isn't showing the "division
by zero" warning for either of these?  The 'ud2' is guaranteed to
trigger since the function has no branches.  Surely at least the missing
warning is a gcc bug.

The good news is objtool is flushing these out, albeit with a confusing
message.

-- 
Josh