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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <D1A1643C-17FE-450B-872B-C2F736FAFA68@zytor.com>
Date:   Thu, 24 May 2018 14:33:45 -0700
From:   hpa@...or.com
To:     tstellar@...hat.com, Tom Stellard <tstellar@...hat.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 May 24, 2018 12:49:56 PM PDT, Tom Stellard <tstellar@...hat.com> wrote:
>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

Ok, yet another problem (this time with paravirtualization, no surprise there.). Good, let's find the actual problems and fix them where they should be fixed.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.