linux-kernel - Re: [GIT PULL] x86/asm updates for v5.10

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: <CAHk-=wiVoLoZS1v9SCQzH4mkpz6akE3pPrHPxM2hamOXPcaW9w@mail.gmail.com>
Date:   Mon, 12 Oct 2020 11:11:35 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Borislav Petkov <bp@...e.de>, Uros Bizjak <ubizjak@...il.com>
Cc:     x86-ml <x86@...nel.org>, lkml <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] x86/asm updates for v5.10

On Mon, Oct 12, 2020 at 4:06 AM Borislav Petkov <bp@...e.de> wrote:
>
> * Use XORL instead of XORQ to avoid a REX prefix and save some bytes in
> the .fixup section, by Uros Bizjak.

I think this one is actually buggy.

For the 1-byte case, it does this:

     __get_user_asm(x_u8__, ptr, retval, "b", "=q");

and ends up doing "xorl" on a register that we told the compiler is a
byte register (with that "=q")

Yes, it uses "%k[output]" to turn that byte register into the word
version of the register, but there's no fundamental reason why the
register might not be something like "%ah".

Does the "xorl" work? Does it build? Yes, and yes.

But maybe %al contains SOMETHING ELSE, and it now clears that too,
because the asm is basically doing something completely different than
what we told the compiler it would do.

Now, afaik, gcc (and presumably clang) basically almost never use the
high byte registers. But I still think this patch is fundamentally
wrong and conceptually completely buggy, even if it might work in
practice.

Also, I'm going to uninline this nasty __get_user() function anyway
for 5.10, so the patch ends up being not just wrong, but pointless.
This is not some kind of hot code that should be optimized, and the
extra byte is not a lot to worry about.

Annoying. Because the other patch in this pull request is fine, and
people want it.

But I'm going to skip this pull request, because I really think it's
dangerously and subtly buggy even if there might not be any case that
matters in reality.

                   Linus