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 <[email protected]> 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/[email protected]?part=2
