On Mon, Nov 26, 2018 at 03:26:28PM -0600, Josh Poimboeuf wrote:
> Yeah, that's probably better. I assume you also mean that we would have
> all text_poke_bp() users create a handler callback? That way the
> interface is clear and consistent for everybody. Like:
Can do, it does indeed make the interface less like a hack. It is not
like there are too many users.
> diff --git a/arch/x86/kernel/jump_label.c b/arch/x86/kernel/jump_label.c
> index aac0c1f7e354..d4b0abe4912d 100644
> --- a/arch/x86/kernel/jump_label.c
> +++ b/arch/x86/kernel/jump_label.c
> @@ -37,6 +37,11 @@ static void bug_at(unsigned char *ip, int line)
> BUG();
> }
>
> +static inline void jump_label_bp_handler(struct pt_regs *regs, void *data)
> +{
> + regs->ip += JUMP_LABEL_NOP_SIZE - 1;
> +}
> +
> static void __ref __jump_label_transform(struct jump_entry *entry,
> enum jump_label_type type,
> void *(*poker)(void *, const void *,
> size_t),
> @@ -91,7 +96,7 @@ static void __ref __jump_label_transform(struct jump_entry
> *entry,
> }
>
> text_poke_bp((void *)jump_entry_code(entry), code, JUMP_LABEL_NOP_SIZE,
> - (void *)jump_entry_code(entry) + JUMP_LABEL_NOP_SIZE);
> + jump_label_bp_handler, NULL);
> }
>
> void arch_jump_label_transform(struct jump_entry *entry,
Per that example..
> diff --git a/arch/x86/kernel/static_call.c b/arch/x86/kernel/static_call.c
> index d3869295b88d..e05ebc6d4db5 100644
> --- a/arch/x86/kernel/static_call.c
> +++ b/arch/x86/kernel/static_call.c
> @@ -7,24 +7,30 @@
>
> #define CALL_INSN_SIZE 5
>
> +struct static_call_bp_data {
> + unsigned long func, ret;
> +};
> +
> +static void static_call_bp_handler(struct pt_regs *regs, void *_data)
> +{
> + struct static_call_bp_data *data = _data;
> +
> + /*
> + * For inline static calls, push the return address on the stack so the
> + * "called" function will return to the location immediately after the
> + * call site.
> + *
> + * NOTE: This code will need to be revisited when kernel CET gets
> + * implemented.
> + */
> + if (data->ret) {
> + regs->sp -= sizeof(long);
> + *(unsigned long *)regs->sp = data->ret;
> + }
> +
> + /* The exception handler will 'return' to the destination function. */
> + regs->ip = data->func;
> +}
Now; if I'm not mistaken, the below @site is in fact @regs->ip - 1, no?
We already patched site with INT3, which is what we just trapped on. So
we could in fact write something like:
static void static_call_bp_handler(struct pt_regs *regs, void *data)
{
struct static_call_bp_data *scd = data;
switch (data->type) {
case CALL_INSN: /* emulate CALL instruction */
regs->sp -= sizeof(unsigned long);
*(unsigned long *)regs->sp = regs->ip + CALL_INSN_SIZE - 1;
regs->ip = data->func;
break;
case JMP_INSN: /* emulate JMP instruction */
regs->ip = data->func;
break;
}
}
> void arch_static_call_transform(void *site, void *tramp, void *func)
> {
> @@ -32,11 +38,17 @@ void arch_static_call_transform(void *site, void *tramp,
> void *func)
> unsigned long insn;
> unsigned char insn_opcode;
> unsigned char opcodes[CALL_INSN_SIZE];
> + struct static_call_bp_data handler_data;
> +
> + handler_data.func = (unsigned long)func;
>
> - if (IS_ENABLED(CONFIG_HAVE_STATIC_CALL_INLINE))
> + if (IS_ENABLED(CONFIG_HAVE_STATIC_CALL_INLINE)) {
> insn = (unsigned long)site;
> + handler_data.ret = insn + CALL_INSN_SIZE;
> + } else {
> insn = (unsigned long)tramp;
> + handler_data.ret = 0;
> + }
handler_data = (struct static_call_bp_data){
.type = IS_ENABLED(CONFIG_HAVE_STATIC_CALL_INLINE) ? CALL_INSN
: JMP_INSN,
.func = func,
};
> mutex_lock(&text_mutex);
>
> @@ -52,14 +64,9 @@ void arch_static_call_transform(void *site, void *tramp,
> void *func)
> opcodes[0] = insn_opcode;
> memcpy(&opcodes[1], &dest_relative, CALL_INSN_SIZE - 1);
>
> /* Patch the call site: */
> text_poke_bp((void *)insn, opcodes, CALL_INSN_SIZE,
> + static_call_bp_handler, &handler_data);
>
> done:
> mutex_unlock(&text_mutex);
