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