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:   Thu, 17 Nov 2022 22:09:15 +0900
From:   Masami Hiramatsu (Google) <mhiramat@...nel.org>
To:     Tiezhu Yang <yangtiezhu@...ngson.cn>
Cc:     "Naveen N. Rao" <naveen.n.rao@...ux.ibm.com>,
        Anil S Keshavamurthy <anil.s.keshavamurthy@...el.com>,
        "David S. Miller" <davem@...emloft.net>,
        Huacai Chen <chenhuacai@...ngson.cn>,
        Jinyang He <hejinyang@...ngson.cn>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: Questions about kprobe handler

On Thu, 17 Nov 2022 09:07:37 +0800
Tiezhu Yang <yangtiezhu@...ngson.cn> wrote:

> Hi KPROBES maintainers,
> 
> There are some differences of kprobe handler implementations on various
> archs, the implementations are almost same on arm64, riscv, csky, the
> code logic is clear and understandable. But on mips and loongarch (not
> upstreamed yet), if get_kprobe() returns NULL, what is the purpose of
> the check "if (addr->word != breakpoint_insn.word)", is it necessary?
> Can we just return directly? Please take a look, thank you.

Good question!

This means that when the software breakpoint was hit on that CPU, but
before calling kprobe handler function, the other CPU can remove that
kprobe from hash table, becahse the hash table is not locked.
In that case, the get_kprobe(addr) will return NULL, and the software
breakpoint instruction is already removed (replaced with the original
instruction). Thus it is safe to go back. But this is originally
implemented for x86, which doesn't need stop_machine() to modify the
code. On the other hand, if an architecture which needs stop_machine()
to modify code, the above scenario never happen. In that case, you
don't need this "if" case.

Thank you,

> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/kernel/kprobes.c#n323
> 		p = get_kprobe(addr);
> 		if (p) {
> 			...
> 		} else if (addr->word != breakpoint_insn.word) {
> 			/*
> 			 * The breakpoint instruction was removed by
> 			 * another cpu right after we hit, no further
> 			 * handling of this interrupt is appropriate
> 			 */
> 			ret = 1;
> 		}
> https://github.com/loongson/linux/blob/loongarch-next-generic-stub/arch/loongarch/kernel/kprobes.c#L262
> 	p = get_kprobe(addr);
> 	if (p) {
> 		...
> 	} else {
> 		if (addr->word != breakpoint_insn.word) {
> 			/*
> 			 * The breakpoint instruction was removed right
> 			 * after we hit it.  Another cpu has removed
> 			 * either a probepoint or a debugger breakpoint
> 			 * at this address.  In either case, no further
> 			 * handling of this interrupt is appropriate.
> 			 */
> 			preempt_enable_no_resched();
> 			return 1;
> 		}
> 	}
> 
> (1) arm64
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/probes/kprobes.c#n309
> static void __kprobes kprobe_handler(struct pt_regs *regs)
> {
> 	struct kprobe *p, *cur_kprobe;
> 	struct kprobe_ctlblk *kcb;
> 	unsigned long addr = instruction_pointer(regs);
> 
> 	kcb = get_kprobe_ctlblk();
> 	cur_kprobe = kprobe_running();
> 
> 	p = get_kprobe((kprobe_opcode_t *) addr);
> 
> 	if (p) {
> 		if (cur_kprobe) {
> 			if (reenter_kprobe(p, regs, kcb))
> 				return;
> 		} else {
> 			/* Probe hit */
> 			set_current_kprobe(p);
> 			kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> 
> 			/*
> 			 * If we have no pre-handler or it returned 0, we
> 			 * continue with normal processing.  If we have a
> 			 * pre-handler and it returned non-zero, it will
> 			 * modify the execution path and no need to single
> 			 * stepping. Let's just reset current kprobe and exit.
> 			 */
> 			if (!p->pre_handler || !p->pre_handler(p, regs)) {
> 				setup_singlestep(p, regs, kcb, 0);
> 			} else
> 				reset_current_kprobe();
> 		}
> 	}
> 	/*
> 	 * The breakpoint instruction was removed right
> 	 * after we hit it.  Another cpu has removed
> 	 * either a probepoint or a debugger breakpoint
> 	 * at this address.  In either case, no further
> 	 * handling of this interrupt is appropriate.
> 	 * Return back to original instruction, and continue.
> 	 */
> }
> 
> (2) riscv
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/riscv/kernel/probes/kprobes.c#n269
> bool __kprobes
> kprobe_breakpoint_handler(struct pt_regs *regs)
> {
> 	struct kprobe *p, *cur_kprobe;
> 	struct kprobe_ctlblk *kcb;
> 	unsigned long addr = instruction_pointer(regs);
> 
> 	kcb = get_kprobe_ctlblk();
> 	cur_kprobe = kprobe_running();
> 
> 	p = get_kprobe((kprobe_opcode_t *) addr);
> 
> 	if (p) {
> 		if (cur_kprobe) {
> 			if (reenter_kprobe(p, regs, kcb))
> 				return true;
> 		} else {
> 			/* Probe hit */
> 			set_current_kprobe(p);
> 			kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> 
> 			/*
> 			 * If we have no pre-handler or it returned 0, we
> 			 * continue with normal processing.  If we have a
> 			 * pre-handler and it returned non-zero, it will
> 			 * modify the execution path and no need to single
> 			 * stepping. Let's just reset current kprobe and exit.
> 			 *
> 			 * pre_handler can hit a breakpoint and can step thru
> 			 * before return.
> 			 */
> 			if (!p->pre_handler || !p->pre_handler(p, regs))
> 				setup_singlestep(p, regs, kcb, 0);
> 			else
> 				reset_current_kprobe();
> 		}
> 		return true;
> 	}
> 
> 	/*
> 	 * The breakpoint instruction was removed right
> 	 * after we hit it.  Another cpu has removed
> 	 * either a probepoint or a debugger breakpoint
> 	 * at this address.  In either case, no further
> 	 * handling of this interrupt is appropriate.
> 	 * Return back to original instruction, and continue.
> 	 */
> 	return false;
> }
> 
> (3) csky
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/csky/kernel/probes/kprobes.c#n311
> int __kprobes
> kprobe_breakpoint_handler(struct pt_regs *regs)
> {
> 	struct kprobe *p, *cur_kprobe;
> 	struct kprobe_ctlblk *kcb;
> 	unsigned long addr = instruction_pointer(regs);
> 
> 	kcb = get_kprobe_ctlblk();
> 	cur_kprobe = kprobe_running();
> 
> 	p = get_kprobe((kprobe_opcode_t *) addr);
> 
> 	if (p) {
> 		if (cur_kprobe) {
> 			if (reenter_kprobe(p, regs, kcb))
> 				return 1;
> 		} else {
> 			/* Probe hit */
> 			set_current_kprobe(p);
> 			kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> 
> 			/*
> 			 * If we have no pre-handler or it returned 0, we
> 			 * continue with normal processing.  If we have a
> 			 * pre-handler and it returned non-zero, it will
> 			 * modify the execution path and no need to single
> 			 * stepping. Let's just reset current kprobe and exit.
> 			 *
> 			 * pre_handler can hit a breakpoint and can step thru
> 			 * before return.
> 			 */
> 			if (!p->pre_handler || !p->pre_handler(p, regs))
> 				setup_singlestep(p, regs, kcb, 0);
> 			else
> 				reset_current_kprobe();
> 		}
> 		return 1;
> 	}
> 
> 	/*
> 	 * The breakpoint instruction was removed right
> 	 * after we hit it.  Another cpu has removed
> 	 * either a probepoint or a debugger breakpoint
> 	 * at this address.  In either case, no further
> 	 * handling of this interrupt is appropriate.
> 	 * Return back to original instruction, and continue.
> 	 */
> 	return 0;
> }
> 
> (4) mips
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/kernel/kprobes.c#n279
> static int kprobe_handler(struct pt_regs *regs)
> {
> 	struct kprobe *p;
> 	int ret = 0;
> 	kprobe_opcode_t *addr;
> 	struct kprobe_ctlblk *kcb;
> 
> 	addr = (kprobe_opcode_t *) regs->cp0_epc;
> 
> 	/*
> 	 * We don't want to be preempted for the entire
> 	 * duration of kprobe processing
> 	 */
> 	preempt_disable();
> 	kcb = get_kprobe_ctlblk();
> 
> 	/* Check we're not actually recursing */
> 	if (kprobe_running()) {
> 		p = get_kprobe(addr);
> 		if (p) {
> 			if (kcb->kprobe_status == KPROBE_HIT_SS &&
> 			    p->ainsn.insn->word == breakpoint_insn.word) {
> 				regs->cp0_status &= ~ST0_IE;
> 				regs->cp0_status |= kcb->kprobe_saved_SR;
> 				goto no_kprobe;
> 			}
> 			/*
> 			 * We have reentered the kprobe_handler(), since
> 			 * another probe was hit while within the handler.
> 			 * We here save the original kprobes variables and
> 			 * just single step on the instruction of the new probe
> 			 * without calling any user handlers.
> 			 */
> 			save_previous_kprobe(kcb);
> 			set_current_kprobe(p, regs, kcb);
> 			kprobes_inc_nmissed_count(p);
> 			prepare_singlestep(p, regs, kcb);
> 			kcb->kprobe_status = KPROBE_REENTER;
> 			if (kcb->flags & SKIP_DELAYSLOT) {
> 				resume_execution(p, regs, kcb);
> 				restore_previous_kprobe(kcb);
> 				preempt_enable_no_resched();
> 			}
> 			return 1;
> 		} else if (addr->word != breakpoint_insn.word) {
> 			/*
> 			 * The breakpoint instruction was removed by
> 			 * another cpu right after we hit, no further
> 			 * handling of this interrupt is appropriate
> 			 */
> 			ret = 1;
> 		}
> 		goto no_kprobe;
> 	}
> ...
> }
> 
> (5) loongarch
> https://github.com/loongson/linux/blob/loongarch-next-generic-stub/arch/loongarch/kernel/kprobes.c#L228
> static int __kprobes kprobe_handler(struct pt_regs *regs)
> {
> 	struct kprobe *p;
> 	kprobe_opcode_t *addr;
> 	struct kprobe_ctlblk *kcb;
> 
> 	addr = (kprobe_opcode_t *) regs->csr_era;
> 
> 	/*
> 	 * We don't want to be preempted for the entire
> 	 * duration of kprobe processing
> 	 */
> 	preempt_disable();
> 	kcb = get_kprobe_ctlblk();
> 
> 	p = get_kprobe(addr);
> 	if (p) {
> 		if (kprobe_running()) {
> 			if (reenter_kprobe(p, regs, kcb))
> 				return 1;
> 		} else {
> 			set_current_kprobe(p, regs, kcb);
> 			kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> 			if (p->pre_handler && p->pre_handler(p, regs)) {
> 			/* handler has already set things up, so skip ss setup */
> 				reset_current_kprobe();
> 				preempt_enable_no_resched();
> 				return 1;
> 			} else {
> 				setup_singlestep(p, regs, kcb, 0);
> 				return 1;
> 			}
> 		}
> 	} else {
> 		if (addr->word != breakpoint_insn.word) {
> 			/*
> 			 * The breakpoint instruction was removed right
> 			 * after we hit it.  Another cpu has removed
> 			 * either a probepoint or a debugger breakpoint
> 			 * at this address.  In either case, no further
> 			 * handling of this interrupt is appropriate.
> 			 */
> 			preempt_enable_no_resched();
> 			return 1;
> 		}
> 	}
> 
> 	preempt_enable_no_resched();
> 
> 	return 0;
> }
> 
> Thanks,
> Tiezhu
> 


-- 
Masami Hiramatsu (Google) <mhiramat@...nel.org>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ