Re: [PATCH v2 2/4] openrisc: Add KProbes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] kprobes: unsafe direct memory access in arch_prepare_kprobe()
- [High] kprobes: unsafe direct memory access in kprobe_breakpoint_handler()
- [High] kprobes: SSOL executes with interrupts enabled due to missing regs->sr update
- [High] kprobes: preemption count and lockdep state leak on KPROBE_REENTER
- [Medium] kprobes: missing NOKPROBE_SYMBOL() for simulate_branch() and simulate_pc()
- [Medium] kprobes: l.jalr state corruption on delay slot page fault
--

commit a255007d852431d07365105e6070af0de294b166
Author: Sahil Siddiq <sahilcdq0@gmail.com>

openrisc: Add KProbes
    
Add KProbes support for OpenRISC, based on similar work done for LoongArch,
MIPS and RISC-V. KProbes make it possible to trap at almost any address in the
kernel to collect performance/debugging info.

> diff --git a/arch/openrisc/kernel/kprobes.c b/arch/openrisc/kernel/kprobes.c
> new file mode 100644
> index 0000000000000..3268ffb0d7077
> --- /dev/null
> +++ b/arch/openrisc/kernel/kprobes.c

[ ... ]

> +int arch_prepare_kprobe(struct kprobe *p)
> +{

[ ... ]

> +	if (insns_need_simulation(insn, &exec_delay_slot) && !exec_delay_slot) {
> +		p->ainsn.insn = NULL;
> +	} else {
> +		/*
> +		 * Single step probed instruction or, in case of branch instructions, single
> +		 * step instruction in delay slot.
> +		 */
> +		p->ainsn.insn = get_insn_slot();
> +		if (!p->ainsn.insn)
> +			return -ENOMEM;
> +
> +		if (exec_delay_slot)
> +			patch_insn_write(p->ainsn.insn, *(p->addr + 1));

[Severity: High]
Does this directly dereference p->addr + 1, bypassing the safety provided
by copy_from_kernel_nofault() used earlier in the function? If the delay slot
instruction falls into an unmapped page, couldn't this cause a kernel panic
during probe registration?

> +		else
> +			patch_insn_write(p->ainsn.insn, p->opcode);
> +		patch_insn_write(&p->ainsn.insn[1], KPROBE_SSTEPBP_INSN);
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static void post_kprobe_handler(struct kprobe *cur, struct kprobe_ctlblk *kcb,
> +				struct pt_regs *regs)
> +{
> +	instruction_pointer_set(regs, kcb->restore_pc);
> +
> +	if (kcb->kprobe_status == KPROBE_REENTER) {
> +		restore_previous_kprobe(kcb);
> +		return;

[Severity: High]
Does this early return skip calling preempt_enable_no_resched()? If a reentrant
kprobe finishes single-stepping, this seems to permanently leak the preemption
count disabled in kprobe_breakpoint_handler(), which could lead to a system
lockup.

> +	}
> +
> +	kcb->kprobe_status = KPROBE_HIT_SSDONE;
> +
> +	if (cur->post_handler)
> +		cur->post_handler(cur, regs, 0);
> +
> +	reset_current_kprobe();
> +	preempt_enable_no_resched();
> +}

[ ... ]

> +static void setup_singlestep(struct kprobe *p, struct pt_regs *regs,
> +			     struct kprobe_ctlblk *kcb, int reenter)
> +{
> +	kprobe_opcode_t insn;
> +
> +	if (reenter) {
> +		save_previous_kprobe(kcb);
> +		set_current_kprobe(p);
> +		kcb->kprobe_status = KPROBE_REENTER;
> +	} else {
> +		kcb->kprobe_status = KPROBE_HIT_SS;
> +	}
> +
> +	/* Emulate instruction if required. */
> +	insn = p->opcode;
> +	if (is_branch_insn(insn)) {
> +		simulate_branch(regs, insn);

[Severity: Medium]
Since simulate_branch() and simulate_pc() are executed during Kprobe handling,
should they be marked with NOKPROBE_SYMBOL() in arch/openrisc/kernel/insn.c?
Without it, placing a kprobe on these functions and hitting another branch-related
kprobe could cause a reentrant probe loop and trigger the "Unrecoverable KProbe
detected" panic.

> +		/* Save target addr before updating PC to SSOL slot */
> +		kcb->restore_pc = regs->pc;
> +	} else if (is_pc_insn(insn)) {
> +		simulate_pc(regs, insn);
> +		kcb->restore_pc = regs->pc;
> +		/* No SSOL is required here, so call post-process immediately. */
> +		post_kprobe_handler(p, kcb, regs);
> +		return;
> +	} else
> +		kcb->restore_pc = regs->pc + OPENRISC_INSN_SIZE;
> +
> +	if (p->ainsn.insn) {
> +		/* Disable IRQs before single stepping */
> +		local_irq_save(kcb->irq_flags);

[Severity: High]
Does this only disable interrupts for the exception handler itself, without
modifying regs->sr? Since OpenRISC's exception handler returns using regs->sr,
it appears the single-step out-of-line (SSOL) instruction would execute with
interrupts enabled, violating atomicity and allowing reentrant interrupts.

> +		instruction_pointer_set(regs, (unsigned long)p->ainsn.insn);
> +	} else {
> +		/*
> +		 * The instruction is a branch but delay slots are disabled.
> +		 * Simply call the post-handler.
> +		 */
> +		post_kprobe_handler(p, kcb, regs);
> +	}
> +}

[ ... ]

> +bool kprobe_breakpoint_handler(struct pt_regs *regs)
> +{

[ ... ]

> +	/*
> +	 * The breakpoint instruction was removed right after we hit it
> +	 * possibly by another cpu. If the original instruction was also
> +	 * a trap instruction then return to the trap handler for further
> +	 * processing, else no further processing is required.
> +	 */
> +	if (get_kernel_nofault(insn, addr) < 0
> +		|| (*addr & OPENRISC_MASK_TRAP) != OPENRISC_INSN_TRAP) {

[Severity: High]
Is this directly dereferencing *addr instead of using the safely read 'insn'
variable? If the probe is concurrently unregistered and the memory is unmapped,
dereferencing *addr directly could cause a kernel panic, defeating the safety of
get_kernel_nofault().

> +		preempt_enable_no_resched();
> +		return true;
> +	}
> +
> +	preempt_enable_no_resched();
> +	return false;
> +}

[ ... ]

> +int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
> +{
> +	struct kprobe *cur = kprobe_running();
> +	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
> +
> +	switch (kcb->kprobe_status) {
> +	case KPROBE_HIT_SS:
> +	case KPROBE_REENTER:
> +		/*
> +		 * The instruction being single-stepped caused a page fault.
> +		 * Reset the current KProbe and set PC to the probe's address.
> +		 * Then allow the page fault handler to continue as usual.
> +		 */
> +		instruction_pointer_set(regs, (unsigned long)cur->addr);

[Severity: Medium]
If a page fault occurs during the delay slot of an "l.jalr r9" instruction,
doesn't rewinding the PC to the branch instruction cause state corruption?
simulate_branch() permanently modifies regs->gpr[9], so re-executing the
branch after the fault handler returns would jump to the return address instead
of the original target.

> +
> +		if (kcb->kprobe_status == KPROBE_REENTER) {
> +			restore_previous_kprobe(kcb);

[Severity: High]
Does this KPROBE_REENTER path skip local_irq_restore() and
preempt_enable_no_resched()? This appears to leak the preemption count and
corrupt the lockdep state when a page fault occurs during a reentrant kprobe.

> +		} else {
> +			regs->sr |= kcb->irq_flags;
> +			local_irq_restore(kcb->irq_flags);
> +			reset_current_kprobe();
> +			preempt_enable_no_resched();
> +		}
> +		break;
> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930203129.51048-1-sahilcdq0@gmail.com?part=2