On Thu, May 2, 2024 at 12:30 AM Stephen Brennan <stephen.s.brennan@xxxxxxxxxx> wrote: > > If an error happens in ftrace, ftrace_kill() will prevent disarming > kprobes. Eventually, the ftrace_ops associated with the kprobes will be > freed, yet the kprobes will still be active, and when triggered, they > will use the freed memory, likely resulting in a page fault and panic. > > This behavior can be reproduced quite easily, by creating a kprobe and > then triggering a ftrace_kill(). For simplicity, we can simulate an > ftrace error with a kernel module like [1]: > > [1]: https://github.com/brenns10/kernel_stuff/tree/master/ftrace_killer > > sudo perf probe --add commit_creds > sudo perf trace -e probe:commit_creds > # In another terminal > make > sudo insmod ftrace_killer.ko # calls ftrace_kill(), simulating bug > # Back to perf terminal > # ctrl-c > sudo perf probe --del commit_creds > > After a short period, a page fault and panic would occur as the kprobe > continues to execute and uses the freed ftrace_ops. While ftrace_kill() > is supposed to be used only in extreme circumstances, it is invoked in > FTRACE_WARN_ON() and so there are many places where an unexpected bug > could be triggered, yet the system may continue operating, possibly > without the administrator noticing. If ftrace_kill() does not panic the > system, then we should do everything we can to continue operating, > rather than leave a ticking time bomb. > > Signed-off-by: Stephen Brennan <stephen.s.brennan@xxxxxxxxxx> > --- > Changes in v3: > Don't expose ftrace_is_dead(). Create a "kprobe_ftrace_disabled" > variable and check it directly in the kprobe handlers. > Link to v1/v2 discussion: > https://lore.kernel.org/all/20240426225834.993353-1-stephen.s.brennan@xxxxxxxxxx/ > > arch/csky/kernel/probes/ftrace.c | 3 +++ > arch/loongarch/kernel/ftrace_dyn.c | 3 +++ > arch/parisc/kernel/ftrace.c | 3 +++ > arch/powerpc/kernel/kprobes-ftrace.c | 3 +++ > arch/riscv/kernel/probes/ftrace.c | 3 +++ > arch/s390/kernel/ftrace.c | 3 +++ > arch/x86/kernel/kprobes/ftrace.c | 3 +++ > include/linux/kprobes.h | 7 +++++++ > kernel/kprobes.c | 6 ++++++ > kernel/trace/ftrace.c | 1 + > 10 files changed, 35 insertions(+) > > diff --git a/arch/csky/kernel/probes/ftrace.c b/arch/csky/kernel/probes/ftrace.c > index 834cffcfbce3..7ba4b98076de 100644 > --- a/arch/csky/kernel/probes/ftrace.c > +++ b/arch/csky/kernel/probes/ftrace.c > @@ -12,6 +12,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip, > struct kprobe_ctlblk *kcb; > struct pt_regs *regs; > > + if (unlikely(kprobe_ftrace_disabled)) > + return; > + For csky part. Acked-by: Guo Ren <guoren@xxxxxxxxxx> > bit = ftrace_test_recursion_trylock(ip, parent_ip); > if (bit < 0) > return; > diff --git a/arch/loongarch/kernel/ftrace_dyn.c b/arch/loongarch/kernel/ftrace_dyn.c > index 73858c9029cc..bff058317062 100644 > --- a/arch/loongarch/kernel/ftrace_dyn.c > +++ b/arch/loongarch/kernel/ftrace_dyn.c > @@ -287,6 +287,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip, > struct kprobe *p; > struct kprobe_ctlblk *kcb; > > + if (unlikely(kprobe_ftrace_disabled)) > + return; > + > bit = ftrace_test_recursion_trylock(ip, parent_ip); > if (bit < 0) > return; > diff --git a/arch/parisc/kernel/ftrace.c b/arch/parisc/kernel/ftrace.c > index 621a4b386ae4..c91f9c2e61ed 100644 > --- a/arch/parisc/kernel/ftrace.c > +++ b/arch/parisc/kernel/ftrace.c > @@ -206,6 +206,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip, > struct kprobe *p; > int bit; > > + if (unlikely(kprobe_ftrace_disabled)) > + return; > + > bit = ftrace_test_recursion_trylock(ip, parent_ip); > if (bit < 0) > return; > diff --git a/arch/powerpc/kernel/kprobes-ftrace.c b/arch/powerpc/kernel/kprobes-ftrace.c > index 072ebe7f290b..f8208c027148 100644 > --- a/arch/powerpc/kernel/kprobes-ftrace.c > +++ b/arch/powerpc/kernel/kprobes-ftrace.c > @@ -21,6 +21,9 @@ void kprobe_ftrace_handler(unsigned long nip, unsigned long parent_nip, > struct pt_regs *regs; > int bit; > > + if (unlikely(kprobe_ftrace_disabled)) > + return; > + > bit = ftrace_test_recursion_trylock(nip, parent_nip); > if (bit < 0) > return; > diff --git a/arch/riscv/kernel/probes/ftrace.c b/arch/riscv/kernel/probes/ftrace.c > index 7142ec42e889..a69dfa610aa8 100644 > --- a/arch/riscv/kernel/probes/ftrace.c > +++ b/arch/riscv/kernel/probes/ftrace.c > @@ -11,6 +11,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip, > struct kprobe_ctlblk *kcb; > int bit; > > + if (unlikely(kprobe_ftrace_disabled)) > + return; > + > bit = ftrace_test_recursion_trylock(ip, parent_ip); > if (bit < 0) > return; > diff --git a/arch/s390/kernel/ftrace.c b/arch/s390/kernel/ftrace.c > index c46381ea04ec..7f6f8c438c26 100644 > --- a/arch/s390/kernel/ftrace.c > +++ b/arch/s390/kernel/ftrace.c > @@ -296,6 +296,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip, > struct kprobe *p; > int bit; > > + if (unlikely(kprobe_ftrace_disabled)) > + return; > + > bit = ftrace_test_recursion_trylock(ip, parent_ip); > if (bit < 0) > return; > diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c > index dd2ec14adb77..15af7e98e161 100644 > --- a/arch/x86/kernel/kprobes/ftrace.c > +++ b/arch/x86/kernel/kprobes/ftrace.c > @@ -21,6 +21,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip, > struct kprobe_ctlblk *kcb; > int bit; > > + if (unlikely(kprobe_ftrace_disabled)) > + return; > + > bit = ftrace_test_recursion_trylock(ip, parent_ip); > if (bit < 0) > return; > diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h > index 0ff44d6633e3..5fcbc254d186 100644 > --- a/include/linux/kprobes.h > +++ b/include/linux/kprobes.h > @@ -378,11 +378,15 @@ static inline void wait_for_kprobe_optimizer(void) { } > extern void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip, > struct ftrace_ops *ops, struct ftrace_regs *fregs); > extern int arch_prepare_kprobe_ftrace(struct kprobe *p); > +/* Set when ftrace has been killed: kprobes on ftrace must be disabled for safety */ > +extern bool kprobe_ftrace_disabled __read_mostly; > +extern void kprobe_ftrace_kill(void); > #else > static inline int arch_prepare_kprobe_ftrace(struct kprobe *p) > { > return -EINVAL; > } > +static inline void kprobe_ftrace_kill(void) {} > #endif /* CONFIG_KPROBES_ON_FTRACE */ > > /* Get the kprobe at this addr (if any) - called with preemption disabled */ > @@ -495,6 +499,9 @@ static inline void kprobe_flush_task(struct task_struct *tk) > static inline void kprobe_free_init_mem(void) > { > } > +static inline void kprobe_ftrace_kill(void) > +{ > +} > static inline int disable_kprobe(struct kprobe *kp) > { > return -EOPNOTSUPP; > diff --git a/kernel/kprobes.c b/kernel/kprobes.c > index 65adc815fc6e..166ebf81dc45 100644 > --- a/kernel/kprobes.c > +++ b/kernel/kprobes.c > @@ -1068,6 +1068,7 @@ static struct ftrace_ops kprobe_ipmodify_ops __read_mostly = { > > static int kprobe_ipmodify_enabled; > static int kprobe_ftrace_enabled; > +bool kprobe_ftrace_disabled; > > static int __arm_kprobe_ftrace(struct kprobe *p, struct ftrace_ops *ops, > int *cnt) > @@ -1136,6 +1137,11 @@ static int disarm_kprobe_ftrace(struct kprobe *p) > ipmodify ? &kprobe_ipmodify_ops : &kprobe_ftrace_ops, > ipmodify ? &kprobe_ipmodify_enabled : &kprobe_ftrace_enabled); > } > + > +void kprobe_ftrace_kill() > +{ > + kprobe_ftrace_disabled = true; > +} > #else /* !CONFIG_KPROBES_ON_FTRACE */ > static inline int arm_kprobe_ftrace(struct kprobe *p) > { > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index da1710499698..96db99c347b3 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -7895,6 +7895,7 @@ void ftrace_kill(void) > ftrace_disabled = 1; > ftrace_enabled = 0; > ftrace_trace_function = ftrace_stub; > + kprobe_ftrace_kill(); > } > > /** > -- > 2.39.3 > -- Best Regards Guo Ren
