| Message ID | 20230827114003.224958-1-bjorn@kernel.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2] riscv: Only consider swbp/ss handlers for correct privileged mode | expand |
On Sun, Aug 27, 2023 at 01:40:03PM +0200, Björn Töpel wrote: > From: Björn Töpel <bjorn@rivosinc.com> > > RISC-V software breakpoint trap handlers are used for {k,u}probes. > > When trapping from kernelmode, only the kernelmode handlers should be > considered. Vice versa, only usermode handlers for usermode > traps. This is not the case on RISC-V, which can trigger a bug if a > userspace process uses uprobes, and a WARN() is triggered from > kernelmode (which is implemented via {c.,}ebreak). > > The kernel will trap on the kernelmode {c.,}ebreak, look for uprobes > handlers, realize incorrectly that uprobes need to be handled, and > exit the trap handler early. The trap returns to re-executing the > {c.,}ebreak, and enter an infinite trap-loop. > > The issue was found running the BPF selftest [1]. > > Fix this issue by only considering the swbp/ss handlers for > kernel/usermode respectively. Also, move CONFIG ifdeffery from traps.c > to the asm/{k,u}probes.h headers. > > Note that linux/uprobes.h only include asm/uprobes.h if CONFIG_UPROBES > is defined, which is why asm/uprobes.h needs to be unconditionally > included in traps.c > > Link: https://lore.kernel.org/linux-riscv/87v8d19aun.fsf@all.your.base.are.belong.to.us/ # [1] > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202308271841.HlnnHFL7-lkp@intel.com/ Delete these, LKP did not report the probes issue. The LKP bot says: > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags
Conor Dooley <conor@kernel.org> writes: >> Reported-by: kernel test robot <lkp@intel.com> >> Closes: https://lore.kernel.org/oe-kbuild-all/202308271841.HlnnHFL7-lkp@intel.com/ > > Delete these, LKP did not report the probes issue. The LKP bot says: >> If you fix the issue in a separate patch/commit (i.e. not just a new version of >> the same patch/commit), kindly add following tags Ugh, sloppy. Thanks for clearing that up. I'll wait and see if there's more comments. If not, Palmer, can you remove these two tags? Björn
On Sun, Aug 27, 2023 at 01:40:03PM +0200, Björn Töpel wrote: > From: Björn Töpel <bjorn@rivosinc.com> > > RISC-V software breakpoint trap handlers are used for {k,u}probes. > > When trapping from kernelmode, only the kernelmode handlers should be > considered. Vice versa, only usermode handlers for usermode > traps. This is not the case on RISC-V, which can trigger a bug if a > userspace process uses uprobes, and a WARN() is triggered from > kernelmode (which is implemented via {c.,}ebreak). > > The kernel will trap on the kernelmode {c.,}ebreak, look for uprobes > handlers, realize incorrectly that uprobes need to be handled, and > exit the trap handler early. The trap returns to re-executing the > {c.,}ebreak, and enter an infinite trap-loop. > > The issue was found running the BPF selftest [1]. > > Fix this issue by only considering the swbp/ss handlers for > kernel/usermode respectively. Also, move CONFIG ifdeffery from traps.c > to the asm/{k,u}probes.h headers. > > Note that linux/uprobes.h only include asm/uprobes.h if CONFIG_UPROBES > is defined, which is why asm/uprobes.h needs to be unconditionally > included in traps.c > > Link: https://lore.kernel.org/linux-riscv/87v8d19aun.fsf@all.your.base.are.belong.to.us/ # [1] > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202308271841.HlnnHFL7-lkp@intel.com/ > Reviewed-by: Guo Ren <guoren@kernel.org> > Fixes: 74784081aac8 ("riscv: Add uprobes supported") > Signed-off-by: Björn Töpel <bjorn@rivosinc.com> Reviewed-by: Nam Cao <namcaov@gmail.com> Best regards, Nam
Tested-by: Puranjay Mohan <puranjay12@gmail.com>
diff --git a/arch/riscv/include/asm/kprobes.h b/arch/riscv/include/asm/kprobes.h index e7882ccb0fd4..78ea44f76718 100644 --- a/arch/riscv/include/asm/kprobes.h +++ b/arch/riscv/include/asm/kprobes.h @@ -40,6 +40,15 @@ void arch_remove_kprobe(struct kprobe *p); int kprobe_fault_handler(struct pt_regs *regs, unsigned int trapnr); bool kprobe_breakpoint_handler(struct pt_regs *regs); bool kprobe_single_step_handler(struct pt_regs *regs); - +#else +static inline bool kprobe_breakpoint_handler(struct pt_regs *regs) +{ + return false; +} + +static inline bool kprobe_single_step_handler(struct pt_regs *regs) +{ + return false; +} #endif /* CONFIG_KPROBES */ #endif /* _ASM_RISCV_KPROBES_H */ diff --git a/arch/riscv/include/asm/uprobes.h b/arch/riscv/include/asm/uprobes.h index f2183e00fdd2..3fc7deda9190 100644 --- a/arch/riscv/include/asm/uprobes.h +++ b/arch/riscv/include/asm/uprobes.h @@ -34,7 +34,18 @@ struct arch_uprobe { bool simulate; }; +#ifdef CONFIG_UPROBES bool uprobe_breakpoint_handler(struct pt_regs *regs); bool uprobe_single_step_handler(struct pt_regs *regs); - +#else +static inline bool uprobe_breakpoint_handler(struct pt_regs *regs) +{ + return false; +} + +static inline bool uprobe_single_step_handler(struct pt_regs *regs) +{ + return false; +} +#endif /* CONFIG_UPROBES */ #endif /* _ASM_RISCV_UPROBES_H */ diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c index f798c853bede..cd6f10c73a16 100644 --- a/arch/riscv/kernel/traps.c +++ b/arch/riscv/kernel/traps.c @@ -13,6 +13,8 @@ #include <linux/kdebug.h> #include <linux/uaccess.h> #include <linux/kprobes.h> +#include <linux/uprobes.h> +#include <asm/uprobes.h> #include <linux/mm.h> #include <linux/module.h> #include <linux/irq.h> @@ -246,22 +248,28 @@ static inline unsigned long get_break_insn_length(unsigned long pc) return GET_INSN_LENGTH(insn); } +static bool probe_single_step_handler(struct pt_regs *regs) +{ + bool user = user_mode(regs); + + return user ? uprobe_single_step_handler(regs) : kprobe_single_step_handler(regs); +} + +static bool probe_breakpoint_handler(struct pt_regs *regs) +{ + bool user = user_mode(regs); + + return user ? uprobe_breakpoint_handler(regs) : kprobe_breakpoint_handler(regs); +} + void handle_break(struct pt_regs *regs) { -#ifdef CONFIG_KPROBES - if (kprobe_single_step_handler(regs)) + if (probe_single_step_handler(regs)) return; - if (kprobe_breakpoint_handler(regs)) - return; -#endif -#ifdef CONFIG_UPROBES - if (uprobe_single_step_handler(regs)) + if (probe_breakpoint_handler(regs)) return; - if (uprobe_breakpoint_handler(regs)) - return; -#endif current->thread.bad_cause = regs->cause; if (user_mode(regs))