linux-kernel - Re: [tip:x86/urgent] x86/io: Mark target address as output in 'insb()' asm

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: <CAK8P3a2tpnCZ7-D8SrpnKYs0WFYaG3ozWnp7Z-h0d7T-XrTbDA@mail.gmail.com>
Date:   Wed, 12 Jul 2017 23:47:49 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Andrew Lutomirski <luto@...nel.org>,
        Denys Vlasenko <dvlasenk@...hat.com>,
        Borislav Petkov <bp@...en8.de>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Kalle Valo <kvalo@...eaurora.org>, Peter Anvin <hpa@...or.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Brian Gerst <brgerst@...il.com>,
        "linux-tip-commits@...r.kernel.org" 
        <linux-tip-commits@...r.kernel.org>
Subject: Re: [tip:x86/urgent] x86/io: Mark target address as output in
 'insb()' asm

On Wed, Jul 12, 2017 at 6:57 PM, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
> On Wed, Jul 12, 2017 at 6:10 AM, tip-bot for Arnd Bergmann
> <tipbot@...or.com> wrote:
>>
>> Apparently the assember constraints are slightly off here, as marking the
>> 'addr' argument as a memory output seems appropriate here and gets rid
>> of the warning. For consistency I'm also adding it as input for outsb().
>
> The new constraints look very questionable to me.
>
>>         asm volatile("rep; outs" #bwl                                   \
>> -                    : "+S"(addr), "+c"(count) : "d"(port));            \
>> +                    : "+S"(addr), "+c"(count) : "d"(port), "m" (addr));\
>
>>         asm volatile("rep; ins" #bwl                                    \
>> -                    : "+D"(addr), "+c"(count) : "d"(port));            \
>> +                    : "+D"(addr), "+c"(count), "=m" (addr) : "d"(port));\
>
> That's not how "m" works, afaik. You're passing in an address, but "m"
> takes the _value_.
>
> So as far as I can tell, what you are really doing is say "this
> reads/writes the memory that contains the _pointer_".
>
> So not only does it not do what you think it does, it probably
> actually forces "addr" to be loaded into a stack slot, in order for
> the inline asm to be able to "access" that memory location to get (and
> set) the value of "addr".

Ok, got it, thanks for taking a look!

> So if it hides warnings, it does so by virtue of confusing gcc some
> more about what is actually going on, rather than by fixing the issue.

Right, as far as gcc is concerned, 'addr' might now point to something
that was initialized, so it no longer warns even though it still thinks
that *addr did not get touched.

> I do agree that those inline asm things do lack a memory dependency,
> though. I just think that patch is *completely* wrong.
>
> The real fix is probably to just mark them as "clobbers memory" (ie
> just add "memory" to the clobber list).
>
> If you want to be fancy, you can try to do what <asm/uaccess.h> does,
> which is a disgusting hack, but has traditionally worked;
>
>   struct __large_struct { unsigned long buf[100]; };
>   #define __m(x) (*(struct __large_struct __user *)(x))
>
> and then use your approach with "m" and "=m".

Ok, I'll try both tomorrow and see where I end up.

I guess it's not urgent to fix it since that code has literally
been there since linux-0.1 (first in hd.c, and moved to
asm/io.h in 0.99.17k).

Would you expect that the missing clobber causes actual
runtime bugs and the fix needs to be backported to stable
kernels?

     Arnd