Re: [RFC PATCH v0 10/19] x86/kprobes: Convert to insn_decode()

From: Masami Hiramatsu
Date: Wed Nov 25 2020 - 12:19:39 EST


On Tue, 24 Nov 2020 11:19:43 +0100
Borislav Petkov <bp@xxxxxxxxx> wrote:

> From: Borislav Petkov <bp@xxxxxxx>
>
> Simplify code, no functional changes.

You've made a functional change. Improve decoding error check :)
Anyway, this looks good to me.

Acked-by: Masami Hiramatsu <mhiramat@xxxxxxxxxx>

Thank you!

>
> Signed-off-by: Borislav Petkov <bp@xxxxxxx>
> ---
> arch/x86/kernel/kprobes/core.c | 17 +++++++++++------
> arch/x86/kernel/kprobes/opt.c | 9 +++++++--
> 2 files changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index 547c7abb39f5..43d4a3056d21 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -285,6 +285,8 @@ static int can_probe(unsigned long paddr)
> /* Decode instructions */
> addr = paddr - offset;
> while (addr < paddr) {
> + int ret;
> +
> /*
> * Check if the instruction has been modified by another
> * kprobe, in which case we replace the breakpoint by the
> @@ -296,8 +298,10 @@ static int can_probe(unsigned long paddr)
> __addr = recover_probed_instruction(buf, addr);
> if (!__addr)
> return 0;
> - kernel_insn_init(&insn, (void *)__addr, MAX_INSN_SIZE);
> - insn_get_length(&insn);
> +
> + ret = insn_decode(&insn, (void *)__addr, MAX_INSN_SIZE, INSN_MODE_KERN);
> + if (ret < 0)
> + return 0;
>
> /*
> * Another debugging subsystem might insert this breakpoint.
> @@ -340,8 +344,8 @@ static int is_IF_modifier(kprobe_opcode_t *insn)
> int __copy_instruction(u8 *dest, u8 *src, u8 *real, struct insn *insn)
> {
> kprobe_opcode_t buf[MAX_INSN_SIZE];
> - unsigned long recovered_insn =
> - recover_probed_instruction(buf, (unsigned long)src);
> + unsigned long recovered_insn = recover_probed_instruction(buf, (unsigned long)src);
> + int ret;
>
> if (!recovered_insn || !insn)
> return 0;
> @@ -351,8 +355,9 @@ int __copy_instruction(u8 *dest, u8 *src, u8 *real, struct insn *insn)
> MAX_INSN_SIZE))
> return 0;
>
> - kernel_insn_init(insn, dest, MAX_INSN_SIZE);
> - insn_get_length(insn);
> + ret = insn_decode(insn, dest, MAX_INSN_SIZE, INSN_MODE_KERN);
> + if (ret < 0)
> + return 0;
>
> /* We can not probe force emulate prefixed instruction */
> if (insn_has_emulate_prefix(insn))
> diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
> index 041f0b50bc27..4d67571249e1 100644
> --- a/arch/x86/kernel/kprobes/opt.c
> +++ b/arch/x86/kernel/kprobes/opt.c
> @@ -299,6 +299,8 @@ static int can_optimize(unsigned long paddr)
> addr = paddr - offset;
> while (addr < paddr - offset + size) { /* Decode until function end */
> unsigned long recovered_insn;
> + int ret;
> +
> if (search_exception_tables(addr))
> /*
> * Since some fixup code will jumps into this function,
> @@ -308,8 +310,11 @@ static int can_optimize(unsigned long paddr)
> recovered_insn = recover_probed_instruction(buf, addr);
> if (!recovered_insn)
> return 0;
> - kernel_insn_init(&insn, (void *)recovered_insn, MAX_INSN_SIZE);
> - insn_get_length(&insn);
> +
> + ret = insn_decode(&insn, (void *)recovered_insn, MAX_INSN_SIZE, INSN_MODE_KERN);
> + if (ret < 0)
> + return 0;
> +
> /* Another subsystem puts a breakpoint */
> if (insn.opcode.bytes[0] == INT3_INSN_OPCODE)
> return 0;
> --
> 2.21.0
>


--
Masami Hiramatsu <mhiramat@xxxxxxxxxx>