linux-kernel - Re: [clang] stack protector and f1f029c7bf

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: <195ec03a-0ccb-43f2-e455-c61b91aaf9eb@redhat.com>
Date:   Thu, 24 May 2018 12:49:56 -0700
From:   Tom Stellard <tstellar@...hat.com>
To:     hpa@...or.com, Nick Desaulniers <ndesaulniers@...gle.com>
Cc:     Alistair Strachan <astrachan@...gle.com>,
        Manoj Gupta <manojgupta@...gle.com>,
        Matthias Kaehlcke <mka@...gle.com>,
        Greg Hackmann <ghackmann@...gle.com>, sedat.dilek@...il.com,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [clang] stack protector and f1f029c7bf

On 05/24/2018 11:19 AM, hpa@...or.com wrote:
> On May 23, 2018 3:08:19 PM PDT, Nick Desaulniers <ndesaulniers@...gle.com> wrote:
>> H. Peter,
>>
>> It was reported [0] that compiling the Linux kernel with Clang +
>> CC_STACKPROTECTOR_STRONG was causing a crash in native_save_fl(), due
>> to
>> how GCC does not emit a stack guard for static inline functions (see
>> Alistair's excellent report in [1]) but Clang does.
>>
>> When working with the LLVM release maintainers, Tom had suggested [2]
>> changing the inline assembly constraint in native_save_fl() from '=rm'
>> to
>> '=r', and Alistair had verified the disassembly:
>>
>> (good) code generated w/o -fstack-protector-strong:
>>
>> native_save_fl:
>>          pushfq
>>          popq    -8(%rsp)
>>          movq    -8(%rsp), %rax
>>          retq
>>
>> (good) code generated w/ =r input constraint:
>>
>> native_save_fl:
>>          pushfq
>>          popq    %rax
>>          retq
>>
>> (bad) code generated with -fstack-protector-strong:
>>
>> native_save_fl:
>>          subq    $24, %rsp
>>          movq    %fs:40, %rax
>>          movq    %rax, 16(%rsp)
>>          pushfq
>>          popq    8(%rsp)
>>          movq    8(%rsp), %rax
>>          movq    %fs:40, %rcx
>>          cmpq    16(%rsp), %rcx
>>          jne     .LBB0_2
>>          addq    $24, %rsp
>>          retq
>> .LBB0_2:
>>          callq   __stack_chk_fail
>>
>> It looks like the sugguestion is actually a revert of your commit:
>> ab94fcf528d127fcb490175512a8910f37e5b346:
>> x86: allow "=rm" in native_save_fl()
>>
>> It seemed like there was a question internally about why worry about
>> pop
>> adjusting the stack if the stack could be avoided altogether.
>>
>> I think Sedat can retest this, but I was curious as well about the
>> commit
>> message in ab94fcf528d: "[ Impact: performance ]", but Alistair's
>> analysis
>> of the disassembly seems to indicate there is no performance impact (in
>> fact, looks better as there's one less mov).
>>
>> Is there a reason we should not revert ab94fcf528d12, or maybe a better
>> approach?
>>
>> [0] https://lkml.org/lkml/2018/5/7/534
>> [1] https://bugs.llvm.org/show_bug.cgi?id=37512#c15
>> [2] https://bugs.llvm.org/show_bug.cgi?id=37512#c22
> 
> A stack canary on an *inlined* function? That's bound to break things elsewhere too sooner or later.
> 
> It feels like *once again* clang is asking for the Linux kernel to change to paper over technical or compatibility problems in clang/LLVM. This is not exactly helping the feeling that we should just rip out any and all clang hacks and call it a loss.
> 

In this case this fix is working-around a bug in the kernel.  The problem
here is that the caller of native_save_fl() assumes that it won't
clobber any of the general purpose register even though the function
is defined as a normal c function which tells the compiler that it's
OK to clobber the registers.

This is something that really needs to be fixed in the kernel.  Relying
on heuristics of internal compiler algorithms in order for code to work
correctly is always going to lead to problems like this.

-Tom