linux-kernel - Re: [BUGFIX PATCH 3/3] kprobes/arm: Fix the return address of multiple kretprobes

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: <1487068371.2978.6.camel@linaro.org>
Date:   Tue, 14 Feb 2017 10:32:51 +0000
From:   "Jon Medhurst (Tixy)" <tixy@...aro.org>
To:     Masami Hiramatsu <mhiramat@...nel.org>,
        Russell King <linux@...linux.org.uk>
Cc:     linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Ananth N Mavinakayanahalli <ananth@...ux.vnet.ibm.com>,
        Ingo Molnar <mingo@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        "H . Peter Anvin" <hpa@...or.com>, Wang Nan <wangnan0@...wei.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        "David A . Long" <dave.long@...aro.org>,
        Sandeepa Prabhu <sandeepa.s.prabhu@...il.com>
Subject: Re: [BUGFIX PATCH 3/3] kprobes/arm: Fix the return address of
 multiple kretprobes

On Tue, 2017-02-14 at 00:05 +0900, Masami Hiramatsu wrote:
> This is arm port of commit 737480a0d525 ("kprobes/x86:
> Fix the return address of multiple kretprobes").
> 
> Fix the return address of subsequent kretprobes when multiple
> kretprobes are set on the same function.
> 
> For example:
> 
>   # cd /sys/kernel/debug/tracing
>   # echo "r:event1 sys_symlink" > kprobe_events
>   # echo "r:event2 sys_symlink" >> kprobe_events
>   # echo 1 > events/kprobes/enable
>   # ln -s /tmp/foo /tmp/bar
> 
>  (without this patch)
> 
>   # cat trace | grep -v ^#
>               ln-82    [000] dn.2    68.446525: event1: (kretprobe_trampoline+0x0/0x18 <- SyS_symlink)
>               ln-82    [000] dn.2    68.447831: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> 
>  (with this patch)
> 
>   # cat trace | grep -v ^#
>               ln-81    [000] dn.1    39.463469: event1: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
>               ln-81    [000] dn.1    39.464701: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@...nel.org>
> Cc: KUMANO Syuhei <kumano.prog@...il.com>
> ---

I don't fully understand this function, but I've checked that the ARM
version now matches the x86 version (apart from the x86 specific
register fixup and some comments). So, FWIW

Acked-by: Jon Medhurst <tixy@...aro.org>

I ran the before and after test case in the commit log on ARM and
verified the result is correct. I also tried running the ARM kprobe
tests with these 3 fixes but the tests fail. However, they also fail
without any of these changes, so I'll investigate that further...
  
>  arch/arm/probes/kprobes/core.c |   24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
> index 84989ae..023800a 100644
> --- a/arch/arm/probes/kprobes/core.c
> +++ b/arch/arm/probes/kprobes/core.c
> @@ -440,6 +440,7 @@ static __used __kprobes void *trampoline_handler(struct pt_regs *regs)
>  	struct hlist_node *tmp;
>  	unsigned long flags, orig_ret_address = 0;
>  	unsigned long trampoline_address = (unsigned long)&kretprobe_trampoline;
> +	kprobe_opcode_t *correct_ret_addr = NULL;
>  
>  	INIT_HLIST_HEAD(&empty_rp);
>  	kretprobe_hash_lock(current, &head, &flags);
> @@ -462,14 +463,34 @@ static __used __kprobes void *trampoline_handler(struct pt_regs *regs)
>  			/* another task is sharing our hash bucket */
>  			continue;
>  
> +		orig_ret_address = (unsigned long)ri->ret_addr;
> +
> +		if (orig_ret_address != trampoline_address)
> +			/*
> +			 * This is the real return address. Any other
> +			 * instances associated with this task are for
> +			 * other calls deeper on the call stack
> +			 */
> +			break;
> +	}
> +
> +	kretprobe_assert(ri, orig_ret_address, trampoline_address);
> +
> +	correct_ret_addr = ri->ret_addr;
> +	hlist_for_each_entry_safe(ri, tmp, head, hlist) {
> +		if (ri->task != current)
> +			/* another task is sharing our hash bucket */
> +			continue;
> +
> +		orig_ret_address = (unsigned long)ri->ret_addr;
>  		if (ri->rp && ri->rp->handler) {
>  			__this_cpu_write(current_kprobe, &ri->rp->kp);
>  			get_kprobe_ctlblk()->kprobe_status = KPROBE_HIT_ACTIVE;
> +			ri->ret_addr = correct_ret_addr;
>  			ri->rp->handler(ri, regs);
>  			__this_cpu_write(current_kprobe, NULL);
>  		}
>  
> -		orig_ret_address = (unsigned long)ri->ret_addr;
>  		recycle_rp_inst(ri, &empty_rp);
>  
>  		if (orig_ret_address != trampoline_address)
> @@ -481,7 +502,6 @@ static __used __kprobes void *trampoline_handler(struct pt_regs *regs)
>  			break;
>  	}
>  
> -	kretprobe_assert(ri, orig_ret_address, trampoline_address);
>  	kretprobe_hash_unlock(current, &flags);
>  
>  	hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) {
>