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

[Masami Hiramatsu <mhiramat@kernel.org>]

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

> From: Borislav Petkov <bp@suse.de>
> 
> 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@kernel.org>

Thank you!

> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  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@kernel.org>