Re: [PATCH v2 1/4] openrisc: Add utilities and clean up simulation of instructions

[sashiko-bot@kernel.org]

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 <sahilcdq0@gmail.com>

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/20260930203129.51048-1-sahilcdq0@gmail.com?part=1