linux-kernel - Re: [RFC] security: replace indirect calls with static 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: <20200824150504.GA575149@google.com>
Date:   Mon, 24 Aug 2020 17:05:04 +0200
From:   Brendan Jackman <jackmanb@...gle.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Kees Cook <keescook@...omium.org>,
        Brendan Jackman <jackmanb@...omium.org>,
        linux-kernel@...r.kernel.org, bpf@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        James Morris <jmorris@...ei.org>, pjt@...gle.com,
        Jann Horn <jannh@...gle.com>, rafael.j.wysocki@...el.com,
        thgarnie@...omium.org, KP Singh <kpsingh@...gle.com>,
        paul.renauld.epfl@...il.com
Subject: Re: [RFC] security: replace indirect calls with static calls

On Mon, Aug 24, 2020 at 04:33:44PM +0200, Peter Zijlstra wrote:
> On Mon, Aug 24, 2020 at 04:09:09PM +0200, Brendan Jackman wrote:
> 
> > > > Why this trick with a switch statement? The table of static call is defined
> > > > at compile time. The number of hook callbacks that will be defined is
> > > > unknown at that time, and the table cannot be resized at runtime.  Static
> > > > calls do not define a conditional execution for a non-void function, so the
> > > > executed slots must be non-empty.  With this use of the table and the
> > > > switch, it is possible to jump directly to the first used slot and execute
> > > > all of the slots after. This essentially makes the entry point of the table
> > > > dynamic. Instead, it would also be possible to start from 0 and break after
> > > > the final populated slot, but that would require an additional conditional
> > > > after each slot.
> > >
> > > Instead of just "NOP", having the static branches perform a jump would
> > > solve this pretty cleanly, yes? Something like:
> > >
> > >         ret = DEFAULT_RET;
> > >
> > >         ret = A(args); <--- direct call, no retpoline
> > >         if ret != 0:
> > >                 goto out;
> > >
> > >         ret = B(args); <--- direct call, no retpoline
> > >         if ret != 0:
> > >                 goto out;
> > >
> > >         goto out;
> > >         if ret != 0:
> > >                 goto out;
> > >
> > > out:
> > >         return ret;
> > 
> > Hmm yeah that's a cool idea. This would either need to be implemented
> > with custom code-modification logic for the LSM hooks, or we'd need to
> > think of a way to express it in a sensible addition to the static_call
> > API. I do wonder if the latter could take the form of a generic system
> > for arrays of static calls.
> 
> So you basically want something like:
> 
> 	if (A[0] && (ret = static_call(A[0])(...)))
> 		return ret;
> 
> 	if (A[1] && (ret = static_call(A[1])(...)))
> 		return ret;
> 
> 	....
> 
> 	return ret;
> 
> Right? The problem with static_call_cond() is that we don't know what to
> do with the return value when !func, which is why it's limited to void
> return type.
> 
> You can however construct something like the above with a combination of
> static_branch() and static_call() though. It'll not be pretty, but it
> ought to work:
> 
> 	if (static_branch_likely(A[0].key)) {
> 		ret = static_call(A[0].call)(...);
> 		if (ret)
> 			return ret;
> 	}
> 
> 	...
> 
> 	return ret;
> 
Right. That's actually exactly what Paul's first implementation
looked like for call_int_hook. But we thought the switch thing was
easier to understand.

> 
> > It would also need to handle the fact that IIUC at the moment the last
> > static_call may be a tail call, so we'd be patching an existing jump
> > into a jump to a different target, I don't know if we can do that
> > atomically.
> 
> Of course we can, the static_call() series supports tail-calls just
> fine. In fact, patching jumps is far easier, it was patching call that
> was the real problem because it mucks about with the stack.
> 
OK great. I had a vague apprehension that we could only patch to or from
a NOP.