Commit 0911287c authored by Alexei Starovoitov's avatar Alexei Starovoitov Committed by Daniel Borkmann

bpf: fix bpf_prog_array_copy_to_user() issues

1. move copy_to_user out of rcu section to fix the following issue:

./include/linux/rcupdate.h:302 Illegal context switch in RCU read-side critical section!
stack backtrace:
 __dump_stack lib/dump_stack.c:17 [inline]
 dump_stack+0x194/0x257 lib/dump_stack.c:53
 lockdep_rcu_suspicious+0x123/0x170 kernel/locking/lockdep.c:4592
 rcu_preempt_sleep_check include/linux/rcupdate.h:301 [inline]
 ___might_sleep+0x385/0x470 kernel/sched/core.c:6079
 __might_sleep+0x95/0x190 kernel/sched/core.c:6067
 __might_fault+0xab/0x1d0 mm/memory.c:4532
 _copy_to_user+0x2c/0xc0 lib/usercopy.c:25
 copy_to_user include/linux/uaccess.h:155 [inline]
 bpf_prog_array_copy_to_user+0x217/0x4d0 kernel/bpf/core.c:1587
 bpf_prog_array_copy_info+0x17b/0x1c0 kernel/bpf/core.c:1685
 perf_event_query_prog_array+0x196/0x280 kernel/trace/bpf_trace.c:877
 _perf_ioctl kernel/events/core.c:4737 [inline]
 perf_ioctl+0x3e1/0x1480 kernel/events/core.c:4757

2. move *prog under rcu, since it's not ok to dereference it afterwards

3. in a rare case of prog array being swapped between bpf_prog_array_length()
   and bpf_prog_array_copy_to_user() calls make sure to copy zeros to user space,
   so the user doesn't walk over uninited prog_ids while kernel reported
   uattr->query.prog_cnt > 0

Reported-by: syzbot+7dbcd2d3b85f9b608b23@syzkaller.appspotmail.com
Fixes: 468e2f64 ("bpf: introduce BPF_PROG_QUERY command")
Signed-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
Signed-off-by: default avatarDaniel Borkmann <daniel@iogearbox.net>
parent 6215ea6b
...@@ -1576,25 +1576,41 @@ int bpf_prog_array_copy_to_user(struct bpf_prog_array __rcu *progs, ...@@ -1576,25 +1576,41 @@ int bpf_prog_array_copy_to_user(struct bpf_prog_array __rcu *progs,
__u32 __user *prog_ids, u32 cnt) __u32 __user *prog_ids, u32 cnt)
{ {
struct bpf_prog **prog; struct bpf_prog **prog;
u32 i = 0, id; unsigned long err = 0;
u32 i = 0, *ids;
bool nospc;
/* users of this function are doing:
* cnt = bpf_prog_array_length();
* if (cnt > 0)
* bpf_prog_array_copy_to_user(..., cnt);
* so below kcalloc doesn't need extra cnt > 0 check, but
* bpf_prog_array_length() releases rcu lock and
* prog array could have been swapped with empty or larger array,
* so always copy 'cnt' prog_ids to the user.
* In a rare race the user will see zero prog_ids
*/
ids = kcalloc(cnt, sizeof(u32), GFP_USER);
if (!ids)
return -ENOMEM;
rcu_read_lock(); rcu_read_lock();
prog = rcu_dereference(progs)->progs; prog = rcu_dereference(progs)->progs;
for (; *prog; prog++) { for (; *prog; prog++) {
if (*prog == &dummy_bpf_prog.prog) if (*prog == &dummy_bpf_prog.prog)
continue; continue;
id = (*prog)->aux->id; ids[i] = (*prog)->aux->id;
if (copy_to_user(prog_ids + i, &id, sizeof(id))) {
rcu_read_unlock();
return -EFAULT;
}
if (++i == cnt) { if (++i == cnt) {
prog++; prog++;
break; break;
} }
} }
nospc = !!(*prog);
rcu_read_unlock(); rcu_read_unlock();
if (*prog) err = copy_to_user(prog_ids, ids, cnt * sizeof(u32));
kfree(ids);
if (err)
return -EFAULT;
if (nospc)
return -ENOSPC; return -ENOSPC;
return 0; return 0;
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment