Re: [PATCH v2 1/4] openrisc: Add utilities and clean up simulation of instructions
flat view
From: sashiko-bot@kernel.org
Date: 2026-09-30 20:46:31
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 [off-list ref] 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
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+ 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;
[ ... ]
quoted hunk ↗ jump to hunk
+ 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().
quoted hunk ↗ jump to hunk
+ return; + case OPENRISC_OPCODE_JALR: /* l.jalr */ + temp = regs->pc + link_offset; + regs->pc = regs->gpr[rb]; + regs->gpr[9] = temp; + return; + default: + break; + } +}
quoted hunk ↗ jump to hunk
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");
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
+ 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/20260930203129.51048-1-sahilcdq0@gmail.com?part=1