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:

quoted

quoted

quoted

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.

quoted

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.