linux-kernel - Re: [PATCH] x86/uaccess: fix code generation in put

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: <CALCETrV2rzowdWqhEG7v-hmbUOpNHsBz-hR1RdxBFKuwU6rDTA@mail.gmail.com>
Date:   Fri, 23 Oct 2020 13:42:39 -0700
From:   Andy Lutomirski <luto@...nel.org>
To:     Rasmus Villemoes <linux@...musvillemoes.dk>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        X86 ML <x86@...nel.org>, "H. Peter Anvin" <hpa@...or.com>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Naresh Kamboju <naresh.kamboju@...aro.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] x86/uaccess: fix code generation in put_user()

On Fri, Oct 23, 2020 at 1:32 PM Rasmus Villemoes
<linux@...musvillemoes.dk> wrote:
>
> Quoting
> https://gcc.gnu.org/onlinedocs/gcc/Local-Register-Variables.html:
>
>   You can define a local register variable and associate it with a
>   specified register...
>
>   The only supported use for this feature is to specify registers for
>   input and output operands when calling Extended asm (see Extended
>   Asm). This may be necessary if the constraints for a particular
>   machine don't provide sufficient control to select the desired
>   register.
>
> On 32-bit x86, this is used to ensure that gcc will put an 8-byte
> value into the %edx:%eax pair, while all other cases will just use the
> single register %eax (%rax on x86-64). While the _ASM_AX actually just
> expands to "%eax", note this comment next to get_user() which does
> something very similar:
>
>  * The use of _ASM_DX as the register specifier is a bit of a
>  * simplification, as gcc only cares about it as the starting point
>  * and not size: for a 64-bit value it will use %ecx:%edx on 32 bits
>  * (%ecx being the next register in gcc's x86 register sequence), and
>  * %rdx on 64 bits.
>
> However, getting this to work requires that there is no code between
> the assignment to the local register variable and its use as an input
> to the asm() which can possibly clobber any of the registers involved
> - including evaluation of the expressions making up other inputs.

This looks like the patch is an improvement, but this is still IMO
likely to be very fragile.  Can we just do the size-dependent "a" vs
"A" selection method instead?  Sure, it's a little more code, but it
will be Obviously Correct.  As it stands, I can easily see our code
failing on some gcc or clang version and the compiler authors telling
us that we're relying on unsupportable behavior.