linux-kernel - Re: [PATCH 3/7] objtool: Add support for intra-function calls

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: <20200403144423.lqcvdhi54cl3btvj@treble>
Date:   Fri, 3 Apr 2020 09:44:23 -0500
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Julien Thierry <jthierry@...hat.com>
Cc:     Alexandre Chartre <alexandre.chartre@...cle.com>, x86@...nel.org,
        linux-kernel@...r.kernel.org, peterz@...radead.org,
        tglx@...utronix.de
Subject: Re: [PATCH 3/7] objtool: Add support for intra-function calls

On Fri, Apr 03, 2020 at 09:01:38AM +0100, Julien Thierry wrote:
> 
> 
> On 4/2/20 4:49 PM, Josh Poimboeuf wrote:
> > On Thu, Apr 02, 2020 at 01:53:49PM +0100, Julien Thierry wrote:
> > > Hi Alexandre,
> > > 
> > > I ran into the limitation of intra-function call for the arm64 support but
> > > didn't take the time to make a clean patch to support them properly.
> > 
> > Can you give an example of where arm64 uses intra-function calls?  It
> > sounds sketchy to me :-)  Is it really needed/useful?
> > 
> 
> So the most notable/necessary one(s) is the one in tramp_ventry [1]. This
> macro is used as the begining of exception handlers for exceptions coming
> from userland. It was added as part of the mitigations of spectre (v1???).
> 
> To give some context, x30 is the register that "ret" instruction will use as
> return address, "bl" is the equivalent of x86 "call" and sets x30 before
> jumping to the target address. (However, it doesn't have a special semantic
> for exception returns)
> 
> Note: I believe the comment about the return "stack" is about processor
> internal state (speculative thingies and all) rather than the actual stack,
> since the stack is untouched by that code. But I don't know the actual
> details.

Ok.  So another Spectre special case.

> There are also some in arch/arm64/crypto/crct10dif-ce-core.o , which is
> probably full of fast, smart and optimized code I don't understand :) . So I
> wouldn't feel confident commenting on whether those intra-function calls are
> needed or not.

Glancing at that code, there's a macro which has bl to
.L__pmull_p8_core, which, because it has a local label prefix, doesn't
have an ELF symbol associated with it.  I bet changing that branch to
"bl __pmull_p8_core" (and removing the unnecessary .L__pmull_p8_core
label) would fix the warnings.

So IIUC, this is actually a case where the warning found a cleanup,
albeit a trivial one.

> Last I found is in qcom_link_stack_sanitization() [2], but that's just a
> workaround for a very specific hardware. In my local tree I just put the
> function as STACK_FRAME_NON_STANDARD. But the code just saves the return
> address, has 16 call instructions that just call the instruction after them,
> restores the return address and lets the C-function return normally (and it
> somehow fixes something for that hardware).

Yeah, like Peter said this sounds like x86 RSB stuffing.  More Spectre
nastiness.

So it sounds like the only valid use case for intra-function calls is
Spectre BS...

So, at the risk of possibly contradicting the past version of myself, my
current feeling is that we should annotate such cases, and then warn on
non-annotated cases like the crypto example above.  There's nothing
normal about these Spectre mitigations, so it's ok to add annotations to
such craziness.

-- 
Josh