Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] openrisc: instruction simulator corrupts sr via gpr[0] aliasing - [High] openrisc: silent text patching failure on RO memory - [Low] build: undefined Kprobe constants and handlers in traps.c --
commit 55276d42aa86ee1b0ca046c1e956df26c99385de Author: Sahil Siddiq <[email protected]> openrisc: Add utilities and clean up simulation of instructions Introduce new instruction-related utilities and macros for OpenRISC. This is in preparation for patches that add tracing support such as KProbes and Kretprobes. Link: https://raw.githubusercontent.com/openrisc/doc/master/openrisc-arch-1.4-rev0.pdf > diff --git a/arch/openrisc/kernel/insn.c b/arch/openrisc/kernel/insn.c > new file mode 100644 > index 0000000000000..285658512b433 > --- /dev/null > +++ b/arch/openrisc/kernel/insn.c [ ... ] > +void simulate_pc(struct pt_regs *regs, unsigned long pc_insn) > +{ > + int page_offset; > + unsigned int rd, op; > + > + page_offset = sign_extend32( > + (pc_insn & OPENRISC_ADRP_IMM_MASK) << OPENRISC_ADRP_IMM_SHIFT, > 31); > + rd = (pc_insn & OPENRISC_ADRP_REG_MASK) >> OPENRISC_ADRP_REG_SHIFT; > + op = get_6_bit_opcode(pc_insn); > + > + switch (op) { > + case OPENRISC_OPCODE_ADRP: > + regs->gpr[rd] = page_offset + (regs->pc & > OPENRISC_ADRP_OFFSET_MASK); [Severity: High] Does this code corrupt the Status Register (sr) if rd is 0? In OpenRISC's pt_regs, gpr[0] aliases to the Status Register (sr). If a probe is placed on an instruction like l.adrp r0, ..., writing to regs->gpr[rd] without checking if rd == 0 could inadvertently overwrite the kernel's sr. > + regs->pc += OPENRISC_INSN_SIZE; > + return; > + default: > + break; > + } > +} > + > +void simulate_branch(struct pt_regs *regs, unsigned long jmp_insn) > +{ > + int displacement; > + unsigned int rb, op, link_offset, temp; > + > + displacement = sign_extend32( > + ((jmp_insn) & OPENRISC_JUMP_IMM_MASK) << > OPENRISC_JUMP_IMM_SHIFT, 27); > + rb = (jmp_insn & OPENRISC_JUMP_REG_MASK) >> OPENRISC_JUMP_REG_SHIFT; > + op = get_6_bit_opcode(jmp_insn); > + link_offset = OPENRISC_INSN_SIZE; [ ... ] > + switch (op) { > + case OPENRISC_OPCODE_J: /* l.j */ > + regs->pc += displacement; > + return; [ ... ] > + case OPENRISC_OPCODE_JR: /* l.jr */ > + regs->pc = regs->gpr[rb]; [Severity: High] Similarly, does this code read the Status Register instead of 0 if rb is 0? Since gpr[0] aliases to sr in pt_regs, an instruction like l.jr r0 would end up reading the sr rather than 0 in simulate_branch(). > + return; > + case OPENRISC_OPCODE_JALR: /* l.jalr */ > + temp = regs->pc + link_offset; > + regs->pc = regs->gpr[rb]; > + regs->gpr[9] = temp; > + return; > + default: > + break; > + } > +} > diff --git a/arch/openrisc/kernel/setup.c b/arch/openrisc/kernel/setup.c > index 000a9cc10e6f6..0f69c36472d55 100644 > --- a/arch/openrisc/kernel/setup.c > +++ b/arch/openrisc/kernel/setup.c > @@ -255,6 +255,10 @@ void __init setup_arch(char **cmdline_p) > /* paging_init() sets up the MMU and marks all pages as reserved */ > paging_init(); > > + /* determine if the processor supports delay slots */ > + if (mfspr(SPR_CPUCFGR) & SPR_CPUCFGR_ND) > + static_branch_disable(&delay_slot); > + [Severity: High] Does this static branch update silently fail to patch read-only memory? Since this is called immediately after paging_init() marks the kernel text as read-only, and interrupts are disabled, jump label text patching will fall back to copy_to_kernel_nofault(). This might silently fail to write to the read-only memory, leaving the delay_slot key incorrectly enabled on CPUs without delay slots and leading to corrupted return addresses. > *cmdline_p = boot_command_line; > > printk(KERN_INFO "OpenRISC Linux -- http://openrisc.io\n";); > diff --git a/arch/openrisc/kernel/traps.c b/arch/openrisc/kernel/traps.c > index c195be9cc9fcc..c9ab630ec8950 100644 > --- a/arch/openrisc/kernel/traps.c > +++ b/arch/openrisc/kernel/traps.c [ ... ] > @@ -215,6 +216,32 @@ asmlinkage void do_trap(struct pt_regs *regs, unsigned > long address) > if (user_mode(regs)) { > force_sig_fault(SIGTRAP, TRAP_BRKPT, (void __user *)regs->pc); > } else { > + unsigned long trap, bcode; > + > + if (get_kernel_nofault(trap, (unsigned long *)regs->pc) < 0) > + return; > + > + /* > + * Trap instruction was probably removed and no further > processing > + * is required. > + */ > + if ((trap & OPENRISC_MASK_TRAP) != OPENRISC_INSN_TRAP) > + return; > + > + bcode = (trap & OPENRISC_MASK_BCODE); > + switch (bcode) { > + case BRK_KPROBE_BP: > + if (kprobe_breakpoint_handler(regs)) [Severity: Low] Will this unconditionally break the build during a git bisect? BRK_KPROBE_BP and kprobe_breakpoint_handler() do not appear to be defined until a follow-up commit ("openrisc: Add KProbes"). Introducing their usage here in do_trap() will result in undefined constants and undeclared functions when compiling this commit. > + return; > + break; > + case BRK_KPROBE_SSTEPBP: > + if (kprobe_singlestep_handler(regs)) > + return; > + break; > + default: > + break; > + } > + > pr_emerg("KERNEL: Illegal trap exception 0x%.8lx\n", regs->pc); > die("Die:", regs, SIGILL); > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
