Re: [PATCH v3 4/4] openrisc: Add Kretprobes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] openrisc: missing NOKPROBE annotation on arch_rethook_trampoline()
--

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

openrisc: Add Kretprobes

This commit adds Kretprobes support for OpenRISC on top of the generic
rethook framework, providing arch_rethook_prepare(),
arch_rethook_trampoline_callback(), and the assembly return trampoline
arch_rethook_trampoline.

> diff --git a/arch/openrisc/kernel/rethook_trampoline.S b/arch/openrisc/kernel/rethook_trampoline.S
> new file mode 100644
> index 0000000000000..5e8fd5a652222
> --- /dev/null
> +++ b/arch/openrisc/kernel/rethook_trampoline.S
> @@ -0,0 +1,111 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Author: Sahil Siddiq <sahilcdq0@gmail.com>
> + */
> +
> +#include <linux/linkage.h>
> +
> +#include <asm/asm.h>

[ ... ]

> +SYM_CODE_START(arch_rethook_trampoline)
> +	/* Allocate enough space on stack for pt_regs */
> +	l.addi	r1,r1,-PT_SIZE
> +	save_all_base_regs
> +
> +	l.addi	r3,r1,0	/* pt_regs */
> +	l.jal	arch_rethook_trampoline_callback
> +	l.nop
> +
> +	/* use the result as the return-address */
> +	l.addi	r9,r11,0
> +
> +	restore_all_base_regs
> +	l.addi	r1,r1,PT_SIZE
> +
> +	l.jr	r9
> +	l.nop
> +SYM_CODE_END(arch_rethook_trampoline)

[Severity: High]
Should arch_rethook_trampoline be marked with _ASM_NOKPROBE() here (or
NOKPROBE_SYMBOL() in arch/openrisc/kernel/rethook.c)?

Although arch/openrisc/kernel/rethook_trampoline.S includes <asm/asm.h> and
arch/openrisc/kernel/rethook.c marks arch_rethook_trampoline_callback() and
arch_rethook_prepare() with NOKPROBE_SYMBOL(), arch_rethook_trampoline is
placed in .text without a kprobe blacklist annotation.

Because arch_rethook_trampoline is not blacklisted, registering a kretprobe
or kprobe on arch_rethook_trampoline is permitted. When a kretprobed function
returns to arch_rethook_trampoline with r9 holding the address of
arch_rethook_trampoline, a kretprobe on arch_rethook_trampoline triggers:

pre_handler_kretprobe()
  rethook_hook()
    arch_rethook_prepare()
      rhn->ret_addr = regs->gpr9;

which pushes a rethook_node with ret_addr set to arch_rethook_trampoline onto
current->rethooks. When arch_rethook_trampoline_callback() then calls
rethook_trampoline_handler(), __rethook_find_ret_addr() skips nodes whose
ret_addr equals arch_rethook_trampoline:

kernel/trace/rethook.c:__rethook_find_ret_addr() {
    ...
	while (node) {
		rh = container_of(node, struct rethook_node, llist);
		if (rh->ret_addr != (unsigned long)arch_rethook_trampoline) {
			*cur = node;
			return rh->ret_addr;
		}
		node = node->next;
	}
	return 0;
}

Can this corrupt the current->rethooks shadow stack or trigger the BUG_ON(1)
in rethook_trampoline_handler() when a probe is attached to
arch_rethook_trampoline?

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