linux-kernel - Re: [PATCH] kprobe: sync issue's on ftraced-kprobe.

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]

Date:   Wed, 4 May 2022 12:09:03 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Levi Yun <ppbuk5246@...il.com>
Cc:     tglx@...utronix.de, mingo@...hat.com, bp@...en8.de, hpa@...or.com,
        naveen.n.rao@...ux.ibm.com, davem@...emloft.net,
        rostedt@...dmis.org, yun.wang@...ux.alibaba.com, x86@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kprobe: sync issue's on ftraced-kprobe.

On Mon,  2 May 2022 13:51:02 +0900
Levi Yun <ppbuk5246@...il.com> wrote:

> In kprobe_ftrace_handler, it accesses get kporbe without kprobe_mutex
> held.
> 
> This makes some of synchronizing issue when we use kprobe API in
> kernel-module.

NAK this, because get_kprobes() doesn't require the kprobe_mutex in
the preempt-disabled context. Please read the comment of get_kprobe().

/*
 * This routine is called either:
 *      - under the 'kprobe_mutex' - during kprobe_[un]register().
 *                              OR
 *      - with preemption disabled - from architecture specific code.
 */
struct kprobe *get_kprobe(void *addr)

Moreover, we can not use mutex inside kprobe handler because it runs
in the interrupt context.

Thank you,

> 
> Below is what i experienced:
> 
> CPU 0									CPU 1
> <...>									<In module code>
> kprobe_ftrace_handler
>     get_kprobe
>         __this_cpu_write
> 									unregister_kprobe
> 									unload_module
> 						<			kprobe memory gone>
> 	p->pre_handler <access invalid memory>
> 	page_fault
> 		kprobe_fault_handler
> 			(In here, kprobe memory gone,
> 			 double page fault is happening inifinie).
> 
> Signed-off-by: Levi Yun <ppbuk5246@...il.com>
> ---
>  arch/x86/kernel/kprobes/ftrace.c | 3 +++
>  include/linux/kprobes.h          | 2 ++
>  kernel/kprobes.c                 | 2 +-
>  3 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c
> index dd2ec14adb77..76147ff6ed88 100644
> --- a/arch/x86/kernel/kprobes/ftrace.c
> +++ b/arch/x86/kernel/kprobes/ftrace.c
> @@ -25,6 +25,7 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
>  	if (bit < 0)
>  		return;
>  
> +	mutex_lock(&kprobe_mutex);
>  	p = get_kprobe((kprobe_opcode_t *)ip);
>  	if (unlikely(!p) || kprobe_disabled(p))
>  		goto out;
> @@ -57,7 +58,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
>  		 */
>  		__this_cpu_write(current_kprobe, NULL);
>  	}
> +
>  out:
> +	mutex_unlock(&kprobe_mutex);
>  	ftrace_test_recursion_unlock(bit);
>  }
>  NOKPROBE_SYMBOL(kprobe_ftrace_handler);
> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> index 157168769fc2..4a18147ff6d6 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -191,6 +191,8 @@ struct kprobe_blacklist_entry {
>  DECLARE_PER_CPU(struct kprobe *, current_kprobe);
>  DECLARE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
>  
> +extern struct mutex kprobe_mutex;
> +
>  extern void kprobe_busy_begin(void);
>  extern void kprobe_busy_end(void);
>  
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index dd58c0be9ce2..b65f055b6fa2 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -64,7 +64,7 @@ static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
>  static bool kprobes_all_disarmed;
>  
>  /* This protects 'kprobe_table' and 'optimizing_list' */
> -static DEFINE_MUTEX(kprobe_mutex);
> +DEFINE_MUTEX(kprobe_mutex);
>  static DEFINE_PER_CPU(struct kprobe *, kprobe_instance);
>  
>  kprobe_opcode_t * __weak kprobe_lookup_name(const char *name,
> -- 
> 2.35.1
> 


-- 
Masami Hiramatsu <mhiramat@...nel.org>