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: <AD6F6BCA-B614-4218-9C0C-D2B8A1D7F338@zytor.com>
Date:   Fri, 23 Oct 2020 13:54:39 -0700
From:   hpa@...or.com
To:     Andy Lutomirski <luto@...nel.org>,
        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>,
        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 October 23, 2020 1:42:39 PM PDT, Andy Lutomirski <luto@...nel.org> wrote:
>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.

Yeah, the reason get_user hacks it is because there is no equivalent to "A" for other register pairs, but not using it for dx:ax is just silly.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.