Re: [PATCH v3 3/4] openrisc: Add unit tests for KProbes on branch instructions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] openrisc: kprobes test probes l.addi instead of forward l.j in test_kprobes_j()
- [Medium] openrisc: kprobes test leaves r13 stale, misplaces label, and lacks fallthrough returns
--

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

openrisc: Add unit tests for KProbes on branch instructions

This commit adds KUnit tests for OpenRISC KProbes to verify instruction
simulation and delay slot handling for branch, jump, and page-relative
address calculation instructions.

> diff --git a/arch/openrisc/kernel/tests/test_kprobes_asm.S b/arch/openrisc/kernel/tests/test_kprobes_asm.S
> new file mode 100644
> index 0000000000000..bc87a5fb00d52
> --- /dev/null
> +++ b/arch/openrisc/kernel/tests/test_kprobes_asm.S
> @@ -0,0 +1,157 @@

[ ... ]

> +SYM_FUNC_START(test_kprobes_j)
> +test_kprobes_j_addr1:
> +	l.addi	r11,r0,0
> +	l.j	2f
> +	l.nop

[Severity: Medium]
Should test_kprobes_j_addr1 be placed after l.addi r11,r0,0 and immediately
before l.j 2f?

When test_kprobe_openrisc() in arch/openrisc/kernel/tests/test_kprobes.c
iterates over test_kprobes_addresses[] and calls register_kprobe(),
test_kprobes_j_addr1 currently points to l.addi r11,r0,0 instead of the
forward jump instruction l.j 2f. As a result, the kprobe is installed on
l.addi rather than testing branch simulation on the forward l.j instruction.

> +1:	l.ori	r13,r13,KPROBE_TEST_MAGIC_LOWER
> +	l.addi	r11,r13,0
> +	l.jr	r9
> +	l.nop
> +2:	l.movhi	r13,KPROBE_TEST_MAGIC_UPPER
> +test_kprobes_j_addr2:
> +	l.j	1b
> +	l.nop
> +SYM_FUNC_END(test_kprobes_j)
> +
> +SYM_FUNC_START(test_kprobes_jal)
> +	l.addi	r11,r0,0
> +	l.addi	r15,r9,0
> +test_kprobes_jal_addr1:
> +	l.jal	2f
> +	l.nop
> +1:	l.movhi	r13,KPROBE_TEST_MAGIC_UPPER
> +	l.jr	r9
> +	l.nop
> +test_kprobes_jal_addr2:
> +2:	l.jal	1b
> +	l.nop
> +	l.ori	r13,r13,KPROBE_TEST_MAGIC_LOWER
> +	l.addi	r11,r13,0
> +	l.addi	r9,r15,0
> +	l.jr	r9
> +	l.nop
> +SYM_FUNC_END(test_kprobes_jal)
> +
> +SYM_FUNC_START(test_kprobes_jr)
> +	l.addi	r11,r0,0
> +	l.movhi	r15,hi(2f)
> +	l.ori	r15,r15,lo(2f)
> +test_kprobes_jr_addr1:
> +	l.jr	r15
> +	l.nop
> +1:	l.ori	r13,r13,KPROBE_TEST_MAGIC_LOWER
> +	l.addi	r11,r13,0
> +	l.jr	r9
> +	l.nop
> +2:	l.movhi	r13,KPROBE_TEST_MAGIC_UPPER
> +	l.movhi	r15,hi(1b)
> +	l.ori	r15,r15,lo(1b)
> +test_kprobes_jr_addr2:
> +	l.jr	r15
> +	l.nop
> +SYM_FUNC_END(test_kprobes_jr)
> +
> +SYM_FUNC_START(test_kprobes_jalr)
> +	l.addi	r11,r0,0
> +	l.addi	r15,r9,0
> +	l.movhi	r17,hi(2f)
> +	l.ori	r17,r17,lo(2f)
> +test_kprobes_jalr_addr1:
> +	l.jalr	r17
> +	l.nop
> +1:	l.movhi	r13,KPROBE_TEST_MAGIC_UPPER
> +	l.jr	r9
> +	l.nop
> +2:	l.movhi	r17,hi(1b)
> +	l.ori	r17,r17,lo(1b)
> +test_kprobes_jalr_addr2:
> +	l.jalr	r17
> +	l.nop
> +	l.ori	r13,r13,KPROBE_TEST_MAGIC_LOWER
> +	l.addi	r11,r13,0
> +	l.addi	r9,r15,0
> +	l.jr	r9
> +	l.nop
> +SYM_FUNC_END(test_kprobes_jalr)
> +
> +SYM_FUNC_START(test_kprobes_branch)
> +	l.addi	r11,r0,0
> +	l.addi	r15,r0,1
> +	l.addi	r17,r0,0
> +	l.sfgts	r15,r17
> +test_kprobes_branch_addr1:
> +	l.bf	2f
> +	l.nop
> +1:	l.ori	r13,r13,KPROBE_TEST_MAGIC_LOWER
> +	l.addi	r11,r13,0
> +	l.jr	r9
> +	l.nop
> +2:	l.movhi	r13,KPROBE_TEST_MAGIC_UPPER
> +	l.sfeq	r15,r17
> +test_kprobes_branch_addr2:
> +	l.bnf	1b
> +	l.nop
> +SYM_FUNC_END(test_kprobes_branch)

[Severity: Medium]
Can stale values in r13 and missing fallthrough returns in test_kprobes_j(),
test_kprobes_jal(), test_kprobes_jr(), test_kprobes_jalr(), and
test_kprobes_branch() cause false passes or execution past function
boundaries if branch simulation fails?

First, each of these functions initializes r11 to 0 at entry:

    l.addi  r11,r0,0

However, they construct KPROBE_TEST_MAGIC in r13 using l.movhi and l.ori, and
then copy r13 into r11 before returning:

    l.ori   r13,r13,KPROBE_TEST_MAGIC_LOWER
    l.addi  r11,r13,0

Unlike test_kprobes_adrp(), these five functions do not clear r13 on entry.
Because the preceding test function in test_kprobe_openrisc() leaves
0xDEADC0DE in r13, if a simulated branch (such as l.jal 1b at
test_kprobes_jal_addr2, l.jr r15 at test_kprobes_jr_addr1, l.jalr r17 at
test_kprobes_jalr_addr2, or l.bf 2f at test_kprobes_branch_addr1) fails to
branch and falls through to label 1 (or the instructions following the call),
l.ori r13,r13,KPROBE_TEST_MAGIC_LOWER preserves the upper bits of r13 from
the previous test, overwrites r11 with 0xDEADC0DE, and returns a false
success.

Second, in test_kprobes_j(), test_kprobes_jr(), and test_kprobes_branch(),
there is no return sequence (l.jr r9; l.nop) after the backward branch at
test_kprobes_j_addr2, test_kprobes_jr_addr2, and test_kprobes_branch_addr2.
If one of those backward branches is not taken, execution will fall off the
end of the function into the next function in .text (or, after
test_kprobes_branch(), into the .align PAGE_SIZE zero-padding where
0x00000000 encodes l.j 0, an infinite self-jump) instead of returning to
test_kprobe_openrisc() with a failure value in r11.

Would it make sense to clear r13 on entry in all test functions and add a
fallthrough return after the backward branches?

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