linux-kernel - Re: gcc 4.5.1 / as 2.20.51.0.11 miscompiling drivers/char/i8k.c ?

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 for Android: free password hash cracker in your pocket

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

Message-ID: <AANLkTim48YSGCdtPLqpPyELA8w-Jo=zMGkDN3omJW1ec@mail.gmail.com>
Date:	Mon, 15 Nov 2010 11:04:49 -0800
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Jeff Law <law@...hat.com>
Cc:	Richard Guenther <richard.guenther@...il.com>,
	Andi Kleen <andi@...stfloor.org>,
	Andreas Schwab <schwab@...ux-m68k.org>, Jim <jim876@...all.nl>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	gcc@....gnu.org
Subject: Re: gcc 4.5.1 / as 2.20.51.0.11 miscompiling drivers/char/i8k.c ?

On Mon, Nov 15, 2010 at 10:45 AM, Jeff Law <law@...hat.com> wrote:
>
> A memory clobber should clobber anything in memory, including autos in
> memory; if it doesn't, then that seems like a major problem.  I'd like to
> see the rationale behind not clobbering autos in memory.

Yes. It turns out that the "asm optimized away" was entirely wrong (we
never saw that, it was just a report on another mailing list).

Looking at the asm posted, it seems to me that gcc actually compiles
the asm itself 100% correctly, and the "memory" clobber is working
fine inside that function. So the code generated for i8k_smm() itself
is all good.

But _while_ generating the good code, gcc doesn't seem to realize that
it writes to anything, so it decides to mark the function
"__attribute__((const))", which is obviously wrong (a memory clobber
definitely implies that it's not const). And as a result, the callers
will be mis-optimized, because they do things like

    static int i8k_get_bios_version(void)
    {
        struct smm_regs regs = { .eax = I8K_SMM_BIOS_VERSION, };

        return i8k_smm(&regs) ? : regs.eax;
    }

and since gcc has (incorrectly) decided that "i8k_smm()" is a const
function, it thinks that "regs.eax" hasn't changed, so it doesn't
bother to reload it: it "knows" that it is still I8K_SMM_BIOS_VERSION
that it initialized it with. So it will basically have rewritten that
final return statement as

        return i8k_smm(&regs) ? : I8K_SMM_BIOS_VERSION;

which obviously doesn't really work.

This also explains why adding "volatile" worked. The "asm volatile"
triggered "this is not a const function".

Similarly, the "+m" works, because it also makes clear that the asm is
writing to memory, and isn't a const function.

Now, the "memory" clobber should clearly also have done that, but I'd
be willing to bet that some version of gcc (possibly extra slackware
patches) had forgotten the trivial logic to say "a memory clobber also
makes the user function non-const".

                 Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/