| 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
| ||
|
Message-Id: <20210525230640.1acf44107ee9679b0a7a04ea@kernel.org>
Date: Tue, 25 May 2021 23:06:40 +0900
From: Masami Hiramatsu <mhiramat@...nel.org>
To: Peter Zijlstra <peterz@...radead.org>
Cc: linux-kernel@...r.kernel.org, mingo@...nel.org,
rostedt@...dmis.org, naveen.n.rao@...ux.vnet.ibm.com,
ananth@...ux.ibm.com, x86@...nel.org,
Christoph Hellwig <hch@....de>
Subject: Re: [PATCH 1/2] kprobes: Remove kprobe::fault_handler
On Tue, 25 May 2021 09:25:19 +0200
Peter Zijlstra <peterz@...radead.org> wrote:
> The reason for kprobe::fault_handler(), as given by their comment:
>
> * We come here because instructions in the pre/post
> * handler caused the page_fault, this could happen
> * if handler tries to access user space by
> * copy_from_user(), get_user() etc. Let the
> * user-specified handler try to fix it first.
>
> Is just plain bad. Those other handlers are ran from non-preemptible
> context and had better use _nofault() functions. Also, there is no
> upstream usage of this.
OK, this looks good to me.
Acked-by: Masami Hiramatsu <mhiramat@...nel.org>
Thanks,
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> Reviewed-by: Christoph Hellwig <hch@....de>
> ---
> Documentation/trace/kprobes.rst | 24 +++++-------------------
> arch/arc/kernel/kprobes.c | 10 ----------
> arch/arm/probes/kprobes/core.c | 9 ---------
> arch/arm64/kernel/probes/kprobes.c | 10 ----------
> arch/csky/kernel/probes/kprobes.c | 10 ----------
> arch/ia64/kernel/kprobes.c | 9 ---------
> arch/mips/kernel/kprobes.c | 3 ---
> arch/powerpc/kernel/kprobes.c | 10 ----------
> arch/riscv/kernel/probes/kprobes.c | 10 ----------
> arch/s390/kernel/kprobes.c | 10 ----------
> arch/sh/kernel/kprobes.c | 10 ----------
> arch/sparc/kernel/kprobes.c | 10 ----------
> arch/x86/kernel/kprobes/core.c | 10 ----------
> include/linux/kprobes.h | 8 --------
> kernel/kprobes.c | 19 -------------------
> samples/kprobes/kprobe_example.c | 15 ---------------
> 16 files changed, 5 insertions(+), 172 deletions(-)
>
> --- a/Documentation/trace/kprobes.rst
> +++ b/Documentation/trace/kprobes.rst
> @@ -362,14 +362,11 @@ register_kprobe
> #include <linux/kprobes.h>
> int register_kprobe(struct kprobe *kp);
>
> -Sets a breakpoint at the address kp->addr. When the breakpoint is
> -hit, Kprobes calls kp->pre_handler. After the probed instruction
> -is single-stepped, Kprobe calls kp->post_handler. If a fault
> -occurs during execution of kp->pre_handler or kp->post_handler,
> -or during single-stepping of the probed instruction, Kprobes calls
> -kp->fault_handler. Any or all handlers can be NULL. If kp->flags
> -is set KPROBE_FLAG_DISABLED, that kp will be registered but disabled,
> -so, its handlers aren't hit until calling enable_kprobe(kp).
> +Sets a breakpoint at the address kp->addr. When the breakpoint is hit, Kprobes
> +calls kp->pre_handler. After the probed instruction is single-stepped, Kprobe
> +calls kp->post_handler. Any or all handlers can be NULL. If kp->flags is set
> +KPROBE_FLAG_DISABLED, that kp will be registered but disabled, so, its handlers
> +aren't hit until calling enable_kprobe(kp).
>
> .. note::
>
> @@ -415,17 +412,6 @@ the breakpoint was hit. Return 0 here u
> p and regs are as described for the pre_handler. flags always seems
> to be zero.
>
> -User's fault-handler (kp->fault_handler)::
> -
> - #include <linux/kprobes.h>
> - #include <linux/ptrace.h>
> - int fault_handler(struct kprobe *p, struct pt_regs *regs, int trapnr);
> -
> -p and regs are as described for the pre_handler. trapnr is the
> -architecture-specific trap number associated with the fault (e.g.,
> -on i386, 13 for a general protection fault or 14 for a page fault).
> -Returns 1 if it successfully handled the exception.
> -
> register_kretprobe
> ------------------
>
> --- a/arch/arc/kernel/kprobes.c
> +++ b/arch/arc/kernel/kprobes.c
> @@ -324,16 +324,6 @@ int __kprobes kprobe_fault_handler(struc
> kprobes_inc_nmissed_count(cur);
>
> /*
> - * We come here because instructions in the pre/post
> - * handler caused the page_fault, this could happen
> - * if handler tries to access user space by
> - * copy_from_user(), get_user() etc. Let the
> - * user-specified handler try to fix it first.
> - */
> - if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr))
> - return 1;
> -
> - /*
> * In case the user-specified fault handler returned zero,
> * try to fix up.
> */
> --- a/arch/arm/probes/kprobes/core.c
> +++ b/arch/arm/probes/kprobes/core.c
> @@ -358,15 +358,6 @@ int __kprobes kprobe_fault_handler(struc
> */
> kprobes_inc_nmissed_count(cur);
>
> - /*
> - * We come here because instructions in the pre/post
> - * handler caused the page_fault, this could happen
> - * if handler tries to access user space by
> - * copy_from_user(), get_user() etc. Let the
> - * user-specified handler try to fix it.
> - */
> - if (cur->fault_handler && cur->fault_handler(cur, regs, fsr))
> - return 1;
> break;
>
> default:
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -283,16 +283,6 @@ int __kprobes kprobe_fault_handler(struc
> kprobes_inc_nmissed_count(cur);
>
> /*
> - * We come here because instructions in the pre/post
> - * handler caused the page_fault, this could happen
> - * if handler tries to access user space by
> - * copy_from_user(), get_user() etc. Let the
> - * user-specified handler try to fix it first.
> - */
> - if (cur->fault_handler && cur->fault_handler(cur, regs, fsr))
> - return 1;
> -
> - /*
> * In case the user-specified fault handler returned
> * zero, try to fix up.
> */
> --- a/arch/csky/kernel/probes/kprobes.c
> +++ b/arch/csky/kernel/probes/kprobes.c
> @@ -302,16 +302,6 @@ int __kprobes kprobe_fault_handler(struc
> kprobes_inc_nmissed_count(cur);
>
> /*
> - * We come here because instructions in the pre/post
> - * handler caused the page_fault, this could happen
> - * if handler tries to access user space by
> - * copy_from_user(), get_user() etc. Let the
> - * user-specified handler try to fix it first.
> - */
> - if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr))
> - return 1;
> -
> - /*
> * In case the user-specified fault handler returned
> * zero, try to fix up.
> */
> --- a/arch/ia64/kernel/kprobes.c
> +++ b/arch/ia64/kernel/kprobes.c
> @@ -851,15 +851,6 @@ int __kprobes kprobe_fault_handler(struc
> kprobes_inc_nmissed_count(cur);
>
> /*
> - * We come here because instructions in the pre/post
> - * handler caused the page_fault, this could happen
> - * if handler tries to access user space by
> - * copy_from_user(), get_user() etc. Let the
> - * user-specified handler try to fix it first.
> - */
> - if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr))
> - return 1;
> - /*
> * In case the user-specified fault handler returned
> * zero, try to fix up.
> */
> --- a/arch/mips/kernel/kprobes.c
> +++ b/arch/mips/kernel/kprobes.c
> @@ -403,9 +403,6 @@ int kprobe_fault_handler(struct pt_regs
> struct kprobe *cur = kprobe_running();
> struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
>
> - if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr))
> - return 1;
> -
> if (kcb->kprobe_status & KPROBE_HIT_SS) {
> resume_execution(cur, regs, kcb);
> regs->cp0_status |= kcb->kprobe_old_SR;
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -509,16 +509,6 @@ int kprobe_fault_handler(struct pt_regs
> kprobes_inc_nmissed_count(cur);
>
> /*
> - * We come here because instructions in the pre/post
> - * handler caused the page_fault, this could happen
> - * if handler tries to access user space by
> - * copy_from_user(), get_user() etc. Let the
> - * user-specified handler try to fix it first.
> - */
> - if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr))
> - return 1;
> -
> - /*
> * In case the user-specified fault handler returned
> * zero, try to fix up.
> */
> --- a/arch/riscv/kernel/probes/kprobes.c
> +++ b/arch/riscv/kernel/probes/kprobes.c
> @@ -275,16 +275,6 @@ int __kprobes kprobe_fault_handler(struc
> kprobes_inc_nmissed_count(cur);
>
> /*
> - * We come here because instructions in the pre/post
> - * handler caused the page_fault, this could happen
> - * if handler tries to access user space by
> - * copy_from_user(), get_user() etc. Let the
> - * user-specified handler try to fix it first.
> - */
> - if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr))
> - return 1;
> -
> - /*
> * In case the user-specified fault handler returned
> * zero, try to fix up.
> */
> --- a/arch/s390/kernel/kprobes.c
> +++ b/arch/s390/kernel/kprobes.c
> @@ -453,16 +453,6 @@ static int kprobe_trap_handler(struct pt
> kprobes_inc_nmissed_count(p);
>
> /*
> - * We come here because instructions in the pre/post
> - * handler caused the page_fault, this could happen
> - * if handler tries to access user space by
> - * copy_from_user(), get_user() etc. Let the
> - * user-specified handler try to fix it first.
> - */
> - if (p->fault_handler && p->fault_handler(p, regs, trapnr))
> - return 1;
> -
> - /*
> * In case the user-specified fault handler returned
> * zero, try to fix up.
> */
> --- a/arch/sh/kernel/kprobes.c
> +++ b/arch/sh/kernel/kprobes.c
> @@ -390,16 +390,6 @@ int __kprobes kprobe_fault_handler(struc
> kprobes_inc_nmissed_count(cur);
>
> /*
> - * We come here because instructions in the pre/post
> - * handler caused the page_fault, this could happen
> - * if handler tries to access user space by
> - * copy_from_user(), get_user() etc. Let the
> - * user-specified handler try to fix it first.
> - */
> - if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr))
> - return 1;
> -
> - /*
> * In case the user-specified fault handler returned
> * zero, try to fix up.
> */
> --- a/arch/sparc/kernel/kprobes.c
> +++ b/arch/sparc/kernel/kprobes.c
> @@ -353,16 +353,6 @@ int __kprobes kprobe_fault_handler(struc
> kprobes_inc_nmissed_count(cur);
>
> /*
> - * We come here because instructions in the pre/post
> - * handler caused the page_fault, this could happen
> - * if handler tries to access user space by
> - * copy_from_user(), get_user() etc. Let the
> - * user-specified handler try to fix it first.
> - */
> - if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr))
> - return 1;
> -
> - /*
> * In case the user-specified fault handler returned
> * zero, try to fix up.
> */
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -947,16 +947,6 @@ int kprobe_fault_handler(struct pt_regs
> * these specific fault cases.
> */
> kprobes_inc_nmissed_count(cur);
> -
> - /*
> - * We come here because instructions in the pre/post
> - * handler caused the page_fault, this could happen
> - * if handler tries to access user space by
> - * copy_from_user(), get_user() etc. Let the
> - * user-specified handler try to fix it first.
> - */
> - if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr))
> - return 1;
> }
>
> return 0;
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -54,8 +54,6 @@ struct kretprobe_instance;
> typedef int (*kprobe_pre_handler_t) (struct kprobe *, struct pt_regs *);
> typedef void (*kprobe_post_handler_t) (struct kprobe *, struct pt_regs *,
> unsigned long flags);
> -typedef int (*kprobe_fault_handler_t) (struct kprobe *, struct pt_regs *,
> - int trapnr);
> typedef int (*kretprobe_handler_t) (struct kretprobe_instance *,
> struct pt_regs *);
>
> @@ -83,12 +81,6 @@ struct kprobe {
> /* Called after addr is executed, unless... */
> kprobe_post_handler_t post_handler;
>
> - /*
> - * ... called if executing addr causes a fault (eg. page fault).
> - * Return 1 if it handled fault, otherwise kernel will see it.
> - */
> - kprobe_fault_handler_t fault_handler;
> -
> /* Saved opcode (which has been replaced with breakpoint) */
> kprobe_opcode_t opcode;
>
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1183,23 +1183,6 @@ static void aggr_post_handler(struct kpr
> }
> NOKPROBE_SYMBOL(aggr_post_handler);
>
> -static int aggr_fault_handler(struct kprobe *p, struct pt_regs *regs,
> - int trapnr)
> -{
> - struct kprobe *cur = __this_cpu_read(kprobe_instance);
> -
> - /*
> - * if we faulted "during" the execution of a user specified
> - * probe handler, invoke just that probe's fault handler
> - */
> - if (cur && cur->fault_handler) {
> - if (cur->fault_handler(cur, regs, trapnr))
> - return 1;
> - }
> - return 0;
> -}
> -NOKPROBE_SYMBOL(aggr_fault_handler);
> -
> /* Walks the list and increments nmissed count for multiprobe case */
> void kprobes_inc_nmissed_count(struct kprobe *p)
> {
> @@ -1330,7 +1313,6 @@ static void init_aggr_kprobe(struct kpro
> ap->addr = p->addr;
> ap->flags = p->flags & ~KPROBE_FLAG_OPTIMIZED;
> ap->pre_handler = aggr_pre_handler;
> - ap->fault_handler = aggr_fault_handler;
> /* We don't care the kprobe which has gone. */
> if (p->post_handler && !kprobe_gone(p))
> ap->post_handler = aggr_post_handler;
> @@ -2014,7 +1996,6 @@ int register_kretprobe(struct kretprobe
>
> rp->kp.pre_handler = pre_handler_kretprobe;
> rp->kp.post_handler = NULL;
> - rp->kp.fault_handler = NULL;
>
> /* Pre-allocate memory for max kretprobe instances */
> if (rp->maxactive <= 0) {
> --- a/samples/kprobes/kprobe_example.c
> +++ b/samples/kprobes/kprobe_example.c
> @@ -86,26 +86,11 @@ static void __kprobes handler_post(struc
> #endif
> }
>
> -/*
> - * fault_handler: this is called if an exception is generated for any
> - * instruction within the pre- or post-handler, or when Kprobes
> - * single-steps the probed instruction.
> - */
> -static int handler_fault(struct kprobe *p, struct pt_regs *regs, int trapnr)
> -{
> - pr_info("fault_handler: p->addr = 0x%p, trap #%dn", p->addr, trapnr);
> - /* Return 0 because we don't handle the fault. */
> - return 0;
> -}
> -/* NOKPROBE_SYMBOL() is also available */
> -NOKPROBE_SYMBOL(handler_fault);
> -
> static int __init kprobe_init(void)
> {
> int ret;
> kp.pre_handler = handler_pre;
> kp.post_handler = handler_post;
> - kp.fault_handler = handler_fault;
>
> ret = register_kprobe(&kp);
> if (ret < 0) {
>
>
--
Masami Hiramatsu <mhiramat@...nel.org>
Powered by blists - more mailing lists