diff mbox series

[bpf,1/2] bpf: Forbid bpf_ktime_get_coarse_ns and bpf_timer_* in tracing progs

Message ID 20211108164620.407305-2-me@ubique.spb.ru (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series Forbid bpf_ktime_get_coarse_ns and bpf_timer_* in tracing progs | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for bpf
netdev/fixes_present fail Series targets non-next tree, but doesn't contain any Fixes tags
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 9 this patch: 9
netdev/cc_maintainers success CCed 10 of 10 maintainers
netdev/build_clang success Errors and warnings before: 22 this patch: 22
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 13 this patch: 13
netdev/checkpatch warning WARNING: line length of 91 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf success VM_Test
bpf/vmtest-bpf-PR success PR summary

Commit Message

Dmitrii Banshchikov Nov. 8, 2021, 4:46 p.m. UTC
bpf_ktime_get_coarse_ns() helper uses ktime_get_coarse_ns() time
accessor that isn't safe for any context.
This results in locking issues:

======================================================
WARNING: possible circular locking dependency detected
5.15.0-syzkaller #0 Not tainted
------------------------------------------------------
syz-executor.4/14877 is trying to acquire lock:
ffffffff8cb30008 (tk_core.seq.seqcount){----}-{0:0}, at: ktime_get_coarse_ts64+0x25/0x110 kernel/time/timekeeping.c:2255

but task is already holding lock:
ffffffff90dbf200 (&obj_hash[i].lock){-.-.}-{2:2}, at: debug_object_deactivate+0x61/0x400 lib/debugobjects.c:735

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #1 (&obj_hash[i].lock){-.-.}-{2:2}:
       lock_acquire+0x19f/0x4d0 kernel/locking/lockdep.c:5625
       __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
       _raw_spin_lock_irqsave+0xd1/0x120 kernel/locking/spinlock.c:162
       __debug_object_init+0xd9/0x1860 lib/debugobjects.c:569
       debug_hrtimer_init kernel/time/hrtimer.c:414 [inline]
       debug_init kernel/time/hrtimer.c:468 [inline]
       hrtimer_init+0x20/0x40 kernel/time/hrtimer.c:1592
       ntp_init_cmos_sync kernel/time/ntp.c:676 [inline]
       ntp_init+0xa1/0xad kernel/time/ntp.c:1095
       timekeeping_init+0x512/0x6bf kernel/time/timekeeping.c:1639
       start_kernel+0x267/0x56e init/main.c:1030
       secondary_startup_64_no_verify+0xb1/0xbb

-> #0 (tk_core.seq.seqcount){----}-{0:0}:
       check_prev_add kernel/locking/lockdep.c:3051 [inline]
       check_prevs_add kernel/locking/lockdep.c:3174 [inline]
       validate_chain+0x1dfb/0x8240 kernel/locking/lockdep.c:3789
       __lock_acquire+0x1382/0x2b00 kernel/locking/lockdep.c:5015
       lock_acquire+0x19f/0x4d0 kernel/locking/lockdep.c:5625
       seqcount_lockdep_reader_access+0xfe/0x230 include/linux/seqlock.h:103
       ktime_get_coarse_ts64+0x25/0x110 kernel/time/timekeeping.c:2255
       ktime_get_coarse include/linux/timekeeping.h:120 [inline]
       ktime_get_coarse_ns include/linux/timekeeping.h:126 [inline]
       ____bpf_ktime_get_coarse_ns kernel/bpf/helpers.c:173 [inline]
       bpf_ktime_get_coarse_ns+0x7e/0x130 kernel/bpf/helpers.c:171
       bpf_prog_a99735ebafdda2f1+0x10/0xb50
       bpf_dispatcher_nop_func include/linux/bpf.h:721 [inline]
       __bpf_prog_run include/linux/filter.h:626 [inline]
       bpf_prog_run include/linux/filter.h:633 [inline]
       BPF_PROG_RUN_ARRAY include/linux/bpf.h:1294 [inline]
       trace_call_bpf+0x2cf/0x5d0 kernel/trace/bpf_trace.c:127
       perf_trace_run_bpf_submit+0x7b/0x1d0 kernel/events/core.c:9708
       perf_trace_lock+0x37c/0x440 include/trace/events/lock.h:39
       trace_lock_release+0x128/0x150 include/trace/events/lock.h:58
       lock_release+0x82/0x810 kernel/locking/lockdep.c:5636
       __raw_spin_unlock_irqrestore include/linux/spinlock_api_smp.h:149 [inline]
       _raw_spin_unlock_irqrestore+0x75/0x130 kernel/locking/spinlock.c:194
       debug_hrtimer_deactivate kernel/time/hrtimer.c:425 [inline]
       debug_deactivate kernel/time/hrtimer.c:481 [inline]
       __run_hrtimer kernel/time/hrtimer.c:1653 [inline]
       __hrtimer_run_queues+0x2f9/0xa60 kernel/time/hrtimer.c:1749
       hrtimer_interrupt+0x3b3/0x1040 kernel/time/hrtimer.c:1811
       local_apic_timer_interrupt arch/x86/kernel/apic/apic.c:1086 [inline]
       __sysvec_apic_timer_interrupt+0xf9/0x270 arch/x86/kernel/apic/apic.c:1103
       sysvec_apic_timer_interrupt+0x8c/0xb0 arch/x86/kernel/apic/apic.c:1097
       asm_sysvec_apic_timer_interrupt+0x12/0x20
       __raw_spin_unlock_irqrestore include/linux/spinlock_api_smp.h:152 [inline]
       _raw_spin_unlock_irqrestore+0xd4/0x130 kernel/locking/spinlock.c:194
       try_to_wake_up+0x702/0xd20 kernel/sched/core.c:4118
       wake_up_process kernel/sched/core.c:4200 [inline]
       wake_up_q+0x9a/0xf0 kernel/sched/core.c:953
       futex_wake+0x50f/0x5b0 kernel/futex/waitwake.c:184
       do_futex+0x367/0x560 kernel/futex/syscalls.c:127
       __do_sys_futex kernel/futex/syscalls.c:199 [inline]
       __se_sys_futex+0x401/0x4b0 kernel/futex/syscalls.c:180
       do_syscall_x64 arch/x86/entry/common.c:50 [inline]
       do_syscall_64+0x44/0xd0 arch/x86/entry/common.c:80
       entry_SYSCALL_64_after_hwframe+0x44/0xae

Locking issues are also possible with bpf_timer_* set of helpers:

hrtimer_start()
  lock_base();
  trace_hrtimer...()
    perf_event()
      bpf_run()
        bpf_timer_start()
          hrtimer_start()
            lock_base()         <- DEADLOCK

Forbid use of bpf_ktime_get_coarse_ns() and bpf_timer_* helpers in
BPF_PROG_TYPE_KPROBE, BPF_PROG_TYPE_TRACEPOINT, BPF_PROG_TYPE_PERF_EVENT
and BPF_PROG_TYPE_RAW_TRACEPOINT prog types as it may result locking
issues.

Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
Reported-by: syzbot+43fd005b5a1b4d10781e@syzkaller.appspotmail.com
---
 kernel/bpf/helpers.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

Comments

Denis Kirjanov Nov. 8, 2021, 4:55 p.m. UTC | #1
11/8/21 7:46 PM, Dmitrii Banshchikov пишет:
> bpf_ktime_get_coarse_ns() helper uses ktime_get_coarse_ns() time
> accessor that isn't safe for any context.
> This results in locking issues:
> 
> ======================================================
> WARNING: possible circular locking dependency detected
> 5.15.0-syzkaller #0 Not tainted
> ------------------------------------------------------
> syz-executor.4/14877 is trying to acquire lock:
> ffffffff8cb30008 (tk_core.seq.seqcount){----}-{0:0}, at: ktime_get_coarse_ts64+0x25/0x110 kernel/time/timekeeping.c:2255
> 
> but task is already holding lock:
> ffffffff90dbf200 (&obj_hash[i].lock){-.-.}-{2:2}, at: debug_object_deactivate+0x61/0x400 lib/debugobjects.c:735
> 
> which lock already depends on the new lock.
> 
> the existing dependency chain (in reverse order) is:
> 
> -> #1 (&obj_hash[i].lock){-.-.}-{2:2}:
>         lock_acquire+0x19f/0x4d0 kernel/locking/lockdep.c:5625
>         __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
>         _raw_spin_lock_irqsave+0xd1/0x120 kernel/locking/spinlock.c:162
>         __debug_object_init+0xd9/0x1860 lib/debugobjects.c:569
>         debug_hrtimer_init kernel/time/hrtimer.c:414 [inline]
>         debug_init kernel/time/hrtimer.c:468 [inline]
>         hrtimer_init+0x20/0x40 kernel/time/hrtimer.c:1592
>         ntp_init_cmos_sync kernel/time/ntp.c:676 [inline]
>         ntp_init+0xa1/0xad kernel/time/ntp.c:1095
>         timekeeping_init+0x512/0x6bf kernel/time/timekeeping.c:1639
>         start_kernel+0x267/0x56e init/main.c:1030
>         secondary_startup_64_no_verify+0xb1/0xbb
> 
> -> #0 (tk_core.seq.seqcount){----}-{0:0}:
>         check_prev_add kernel/locking/lockdep.c:3051 [inline]
>         check_prevs_add kernel/locking/lockdep.c:3174 [inline]
>         validate_chain+0x1dfb/0x8240 kernel/locking/lockdep.c:3789
>         __lock_acquire+0x1382/0x2b00 kernel/locking/lockdep.c:5015
>         lock_acquire+0x19f/0x4d0 kernel/locking/lockdep.c:5625
>         seqcount_lockdep_reader_access+0xfe/0x230 include/linux/seqlock.h:103
>         ktime_get_coarse_ts64+0x25/0x110 kernel/time/timekeeping.c:2255
>         ktime_get_coarse include/linux/timekeeping.h:120 [inline]
>         ktime_get_coarse_ns include/linux/timekeeping.h:126 [inline]
>         ____bpf_ktime_get_coarse_ns kernel/bpf/helpers.c:173 [inline]
>         bpf_ktime_get_coarse_ns+0x7e/0x130 kernel/bpf/helpers.c:171
>         bpf_prog_a99735ebafdda2f1+0x10/0xb50
>         bpf_dispatcher_nop_func include/linux/bpf.h:721 [inline]
>         __bpf_prog_run include/linux/filter.h:626 [inline]
>         bpf_prog_run include/linux/filter.h:633 [inline]
>         BPF_PROG_RUN_ARRAY include/linux/bpf.h:1294 [inline]
>         trace_call_bpf+0x2cf/0x5d0 kernel/trace/bpf_trace.c:127
>         perf_trace_run_bpf_submit+0x7b/0x1d0 kernel/events/core.c:9708
>         perf_trace_lock+0x37c/0x440 include/trace/events/lock.h:39
>         trace_lock_release+0x128/0x150 include/trace/events/lock.h:58
>         lock_release+0x82/0x810 kernel/locking/lockdep.c:5636
>         __raw_spin_unlock_irqrestore include/linux/spinlock_api_smp.h:149 [inline]
>         _raw_spin_unlock_irqrestore+0x75/0x130 kernel/locking/spinlock.c:194
>         debug_hrtimer_deactivate kernel/time/hrtimer.c:425 [inline]
>         debug_deactivate kernel/time/hrtimer.c:481 [inline]
>         __run_hrtimer kernel/time/hrtimer.c:1653 [inline]
>         __hrtimer_run_queues+0x2f9/0xa60 kernel/time/hrtimer.c:1749
>         hrtimer_interrupt+0x3b3/0x1040 kernel/time/hrtimer.c:1811
>         local_apic_timer_interrupt arch/x86/kernel/apic/apic.c:1086 [inline]
>         __sysvec_apic_timer_interrupt+0xf9/0x270 arch/x86/kernel/apic/apic.c:1103
>         sysvec_apic_timer_interrupt+0x8c/0xb0 arch/x86/kernel/apic/apic.c:1097
>         asm_sysvec_apic_timer_interrupt+0x12/0x20
>         __raw_spin_unlock_irqrestore include/linux/spinlock_api_smp.h:152 [inline]
>         _raw_spin_unlock_irqrestore+0xd4/0x130 kernel/locking/spinlock.c:194
>         try_to_wake_up+0x702/0xd20 kernel/sched/core.c:4118
>         wake_up_process kernel/sched/core.c:4200 [inline]
>         wake_up_q+0x9a/0xf0 kernel/sched/core.c:953
>         futex_wake+0x50f/0x5b0 kernel/futex/waitwake.c:184
>         do_futex+0x367/0x560 kernel/futex/syscalls.c:127
>         __do_sys_futex kernel/futex/syscalls.c:199 [inline]
>         __se_sys_futex+0x401/0x4b0 kernel/futex/syscalls.c:180
>         do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>         do_syscall_64+0x44/0xd0 arch/x86/entry/common.c:80
>         entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> Locking issues are also possible with bpf_timer_* set of helpers:
> 
> hrtimer_start()
>    lock_base();
>    trace_hrtimer...()
>      perf_event()
>        bpf_run()
>          bpf_timer_start()
>            hrtimer_start()
>              lock_base()         <- DEADLOCK
> 
> Forbid use of bpf_ktime_get_coarse_ns() and bpf_timer_* helpers in
> BPF_PROG_TYPE_KPROBE, BPF_PROG_TYPE_TRACEPOINT, BPF_PROG_TYPE_PERF_EVENT
> and BPF_PROG_TYPE_RAW_TRACEPOINT prog types as it may result locking
> issues.
> 
> Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
> Reported-by: syzbot+43fd005b5a1b4d10781e@syzkaller.appspotmail.com
Add Fixes tag

> ---
>   kernel/bpf/helpers.c | 30 ++++++++++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
> 
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 1ffd469c217f..3de24928166e 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -18,6 +18,19 @@
>   
>   #include "../../lib/kstrtox.h"
>   
> +static bool is_tracing_prog_type(enum bpf_prog_type type)
> +{
> +	switch (type) {
> +	case BPF_PROG_TYPE_KPROBE:
> +	case BPF_PROG_TYPE_TRACEPOINT:
> +	case BPF_PROG_TYPE_PERF_EVENT:
> +	case BPF_PROG_TYPE_RAW_TRACEPOINT:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
>   /* If kernel subsystem is allowing eBPF programs to call this function,
>    * inside its own verifier_ops->get_func_proto() callback it should return
>    * bpf_map_lookup_elem_proto, so that verifier can properly check the arguments
> @@ -173,10 +186,18 @@ BPF_CALL_0(bpf_ktime_get_coarse_ns)
>   	return ktime_get_coarse_ns();
>   }
>   
> +static bool bpf_ktime_get_coarse_ns_allowed(const struct bpf_prog *prog)
> +{
> +	// Forbid prog types that might be non-safe for non-fast variants of time accessors
The comments embaced with /* */
> +
> +	return !is_tracing_prog_type(prog->type);
> +}
> +
>   const struct bpf_func_proto bpf_ktime_get_coarse_ns_proto = {
>   	.func		= bpf_ktime_get_coarse_ns,
>   	.gpl_only	= false,
>   	.ret_type	= RET_INTEGER,
> +	.allowed	= bpf_ktime_get_coarse_ns_allowed,
>   };
>   
>   BPF_CALL_0(bpf_get_current_pid_tgid)
> @@ -1140,6 +1161,11 @@ BPF_CALL_3(bpf_timer_init, struct bpf_timer_kern *, timer, struct bpf_map *, map
>   	return ret;
>   }
>   
> +static bool bpf_timer_allowed(const struct bpf_prog *prog)
> +{
> +	return !is_tracing_prog_type(prog->type);
> +}
> +
>   static const struct bpf_func_proto bpf_timer_init_proto = {
>   	.func		= bpf_timer_init,
>   	.gpl_only	= true,
> @@ -1147,6 +1173,7 @@ static const struct bpf_func_proto bpf_timer_init_proto = {
>   	.arg1_type	= ARG_PTR_TO_TIMER,
>   	.arg2_type	= ARG_CONST_MAP_PTR,
>   	.arg3_type	= ARG_ANYTHING,
> +	.allowed	= bpf_timer_allowed,
>   };
>   
>   BPF_CALL_3(bpf_timer_set_callback, struct bpf_timer_kern *, timer, void *, callback_fn,
> @@ -1200,6 +1227,7 @@ static const struct bpf_func_proto bpf_timer_set_callback_proto = {
>   	.ret_type	= RET_INTEGER,
>   	.arg1_type	= ARG_PTR_TO_TIMER,
>   	.arg2_type	= ARG_PTR_TO_FUNC,
> +	.allowed	= bpf_timer_allowed,
>   };
>   
>   BPF_CALL_3(bpf_timer_start, struct bpf_timer_kern *, timer, u64, nsecs, u64, flags)
> @@ -1230,6 +1258,7 @@ static const struct bpf_func_proto bpf_timer_start_proto = {
>   	.arg1_type	= ARG_PTR_TO_TIMER,
>   	.arg2_type	= ARG_ANYTHING,
>   	.arg3_type	= ARG_ANYTHING,
> +	.allowed	= bpf_timer_allowed,
>   };
>   
>   static void drop_prog_refcnt(struct bpf_hrtimer *t)
> @@ -1279,6 +1308,7 @@ static const struct bpf_func_proto bpf_timer_cancel_proto = {
>   	.gpl_only	= true,
>   	.ret_type	= RET_INTEGER,
>   	.arg1_type	= ARG_PTR_TO_TIMER,
> +	.allowed	= bpf_timer_allowed,
>   };
>   
>   /* This function is called by map_delete/update_elem for individual element and
>
Alexei Starovoitov Nov. 9, 2021, 9:52 p.m. UTC | #2
On Mon, Nov 8, 2021 at 8:46 AM Dmitrii Banshchikov <me@ubique.spb.ru> wrote:
>
> bpf_ktime_get_coarse_ns() helper uses ktime_get_coarse_ns() time
> accessor that isn't safe for any context.
> This results in locking issues:

Please trim the cc. vger thinks it's a spam.
The patch didn't reach the mailing list or patchwork.

>  const struct bpf_func_proto bpf_ktime_get_coarse_ns_proto = {
>         .func           = bpf_ktime_get_coarse_ns,
>         .gpl_only       = false,
>         .ret_type       = RET_INTEGER,
> +       .allowed        = bpf_ktime_get_coarse_ns_allowed,

I think it's easier to move to networking prog types
instead of going through a callback.

>  static const struct bpf_func_proto bpf_timer_init_proto = {
>         .func           = bpf_timer_init,
>         .gpl_only       = true,
> @@ -1147,6 +1173,7 @@ static const struct bpf_func_proto bpf_timer_init_proto = {
>         .arg1_type      = ARG_PTR_TO_TIMER,
>         .arg2_type      = ARG_CONST_MAP_PTR,
>         .arg3_type      = ARG_ANYTHING,
> +       .allowed        = bpf_timer_allowed,

I think disabling timers in check_map_prog_compatibility
(similar to how bpf_spin_lock is restricted) would be cleaner.
diff mbox series

Patch

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 1ffd469c217f..3de24928166e 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -18,6 +18,19 @@ 
 
 #include "../../lib/kstrtox.h"
 
+static bool is_tracing_prog_type(enum bpf_prog_type type)
+{
+	switch (type) {
+	case BPF_PROG_TYPE_KPROBE:
+	case BPF_PROG_TYPE_TRACEPOINT:
+	case BPF_PROG_TYPE_PERF_EVENT:
+	case BPF_PROG_TYPE_RAW_TRACEPOINT:
+		return true;
+	default:
+		return false;
+	}
+}
+
 /* If kernel subsystem is allowing eBPF programs to call this function,
  * inside its own verifier_ops->get_func_proto() callback it should return
  * bpf_map_lookup_elem_proto, so that verifier can properly check the arguments
@@ -173,10 +186,18 @@  BPF_CALL_0(bpf_ktime_get_coarse_ns)
 	return ktime_get_coarse_ns();
 }
 
+static bool bpf_ktime_get_coarse_ns_allowed(const struct bpf_prog *prog)
+{
+	// Forbid prog types that might be non-safe for non-fast variants of time accessors
+
+	return !is_tracing_prog_type(prog->type);
+}
+
 const struct bpf_func_proto bpf_ktime_get_coarse_ns_proto = {
 	.func		= bpf_ktime_get_coarse_ns,
 	.gpl_only	= false,
 	.ret_type	= RET_INTEGER,
+	.allowed	= bpf_ktime_get_coarse_ns_allowed,
 };
 
 BPF_CALL_0(bpf_get_current_pid_tgid)
@@ -1140,6 +1161,11 @@  BPF_CALL_3(bpf_timer_init, struct bpf_timer_kern *, timer, struct bpf_map *, map
 	return ret;
 }
 
+static bool bpf_timer_allowed(const struct bpf_prog *prog)
+{
+	return !is_tracing_prog_type(prog->type);
+}
+
 static const struct bpf_func_proto bpf_timer_init_proto = {
 	.func		= bpf_timer_init,
 	.gpl_only	= true,
@@ -1147,6 +1173,7 @@  static const struct bpf_func_proto bpf_timer_init_proto = {
 	.arg1_type	= ARG_PTR_TO_TIMER,
 	.arg2_type	= ARG_CONST_MAP_PTR,
 	.arg3_type	= ARG_ANYTHING,
+	.allowed	= bpf_timer_allowed,
 };
 
 BPF_CALL_3(bpf_timer_set_callback, struct bpf_timer_kern *, timer, void *, callback_fn,
@@ -1200,6 +1227,7 @@  static const struct bpf_func_proto bpf_timer_set_callback_proto = {
 	.ret_type	= RET_INTEGER,
 	.arg1_type	= ARG_PTR_TO_TIMER,
 	.arg2_type	= ARG_PTR_TO_FUNC,
+	.allowed	= bpf_timer_allowed,
 };
 
 BPF_CALL_3(bpf_timer_start, struct bpf_timer_kern *, timer, u64, nsecs, u64, flags)
@@ -1230,6 +1258,7 @@  static const struct bpf_func_proto bpf_timer_start_proto = {
 	.arg1_type	= ARG_PTR_TO_TIMER,
 	.arg2_type	= ARG_ANYTHING,
 	.arg3_type	= ARG_ANYTHING,
+	.allowed	= bpf_timer_allowed,
 };
 
 static void drop_prog_refcnt(struct bpf_hrtimer *t)
@@ -1279,6 +1308,7 @@  static const struct bpf_func_proto bpf_timer_cancel_proto = {
 	.gpl_only	= true,
 	.ret_type	= RET_INTEGER,
 	.arg1_type	= ARG_PTR_TO_TIMER,
+	.allowed	= bpf_timer_allowed,
 };
 
 /* This function is called by map_delete/update_elem for individual element and