The patch "bpf: compat64: add handler and convert bpf_attr in" handled the case where a compat64 version of union bpf_attr is passed into the kernel via the bpf syscall. It resolved the differences in size and member offsets by copying the compat version into a native version of the struct to be used in the kernel.
When writing back out to the original __user *bpf_attr union, we must reverse the conversion to correctly write to the appropriate offsets.
The "technically correct" approach would probably be to replace all functions that have a union bpf_attr __user* parameter with void __user* (at least where we pass uattr.user in dispatch_bpf() and propagate this down). This forces us to check the type and cast before we use uattr, since the type is technically unknown. This avoids any potential mis-use by not checking for compat64 before trying to access a compat_bpf_attr ptr with native union offsets. The downside of this approach is we would end up touching many function signatures as uattr propagates down the call stack (currently 73).
The minimal diff approach used here is to handle only specifically where we write directly to uattr with macros to select the correct struct type (compat or native). This greatly reduces the diff size at the cost of having compat_bpf_attr unions passed around as union bpf_attr and needing to remember to cast back where required.
The instances where we write out to uattr were found by grepping 'copy_to_user(&uattr', 'put_user(.*uattr' and 'copy_to_bpfptr' so the list may or may not be complete. Luckily this doesn't appear to happen that often and usage seems to be consistent.
We can also replace all instances of copy_to_user() here with put_user() which somewhat simplifies things. Similarly since we're only copying individual members with copy_to_bpfptr_offset(), bpfptr_put_uattr() usage is simplified by not having to specify the size.
Note: conversion out is only required where we're writing out to the original userspace union bpf_attr i.e. uattr. There are many instances where we receive a __user ptr from userspace via bpf_attr, e.g. a ptr to some userspace buffer. This pointer is copied into a native struct (it may be converted from a compat layout), and we can then *write directly to pointer target* without requiring any additional conversion out. copy_to_user() or put_user() calls where the target is a __user ptr (not a ptr to the __user union bpf_attr) therefore do not require any additional handling, since the offsets and struct layouts are irrelevant.
Signed-off-by: Zachary Leaf zachary.leaf@arm.com --- drivers/media/rc/bpf-lirc.c | 4 ++-- include/linux/bpf.h | 13 +++++++++++++ include/linux/bpfptr.h | 8 ++++++++ kernel/bpf/btf.c | 5 ++--- kernel/bpf/cgroup.c | 4 ++-- kernel/bpf/hashtab.c | 4 ++-- kernel/bpf/net_namespace.c | 4 ++-- kernel/bpf/syscall.c | 22 +++++++++++----------- kernel/bpf/verifier.c | 17 +++++------------ net/bpf/bpf_dummy_struct_ops.c | 2 +- net/bpf/test_run.c | 15 +++++++-------- net/core/sock_map.c | 4 ++-- 12 files changed, 57 insertions(+), 45 deletions(-)
diff --git a/drivers/media/rc/bpf-lirc.c b/drivers/media/rc/bpf-lirc.c index fe17c7f98e81..7f713422049d 100644 --- a/drivers/media/rc/bpf-lirc.c +++ b/drivers/media/rc/bpf-lirc.c @@ -319,12 +319,12 @@ int lirc_prog_query(const union bpf_attr *attr, union bpf_attr __user *uattr) progs = lirc_rcu_dereference(rcdev->raw->progs); cnt = progs ? bpf_prog_array_length(progs) : 0;
- if (copy_to_user(&uattr->query.prog_cnt, &cnt, sizeof(cnt))) { + if (bpf_put_uattr(cnt, uattr, query.prog_cnt)) { ret = -EFAULT; goto unlock; }
- if (copy_to_user(&uattr->query.attach_flags, &flags, sizeof(flags))) { + if (bpf_put_uattr(flags, uattr, query.attach_flags)) { ret = -EFAULT; goto unlock; } diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 6c8594a0f883..e056737bc823 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -59,6 +59,19 @@ extern struct kobject *btf_kobj; extern struct bpf_mem_alloc bpf_global_ma; extern bool bpf_global_ma_set;
+#define __bpf_put_uattr(x, uattr, to_field) \ + (put_user(x, &((uattr)->to_field))) + +#define bpf_put_uattr(x, uattr, to_field) \ + (in_compat_syscall() ? \ + __bpf_put_uattr(x, (union compat_bpf_attr __user *)uattr, to_field) : \ + __bpf_put_uattr(x, (union bpf_attr __user *)uattr, to_field)) + +#define bpf_field_exists(uattr_size, field) \ + (in_compat_syscall() ? \ + (uattr_size >= offsetofend(union compat_bpf_attr, field)) : \ + (uattr_size >= offsetofend(union bpf_attr, field))) + typedef u64 (*bpf_callback_t)(u64, u64, u64, u64, u64); typedef int (*bpf_iter_init_seq_priv_t)(void *private_data, struct bpf_iter_aux_info *aux); diff --git a/include/linux/bpfptr.h b/include/linux/bpfptr.h index 79b2f78eec1a..7fdf9692d76e 100644 --- a/include/linux/bpfptr.h +++ b/include/linux/bpfptr.h @@ -8,6 +8,14 @@
typedef sockptr_t bpfptr_t;
+#define __bpfptr_put_uattr(type, x, uattr, to_field) \ + (copy_to_bpfptr_offset(uattr, offsetof(type, to_field), &x, sizeof(x))) + +#define bpfptr_put_uattr(x, uattr, to_field) \ + (in_compat_syscall() ? \ + __bpfptr_put_uattr(union compat_bpf_attr, x, uattr, to_field) : \ + __bpfptr_put_uattr(union bpf_attr, x, uattr, to_field)) + static inline bool bpfptr_is_kernel(bpfptr_t bpfptr) { return bpfptr.is_kernel; diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 72b32b7cd9cd..061a8356abf5 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -5454,9 +5454,8 @@ static int finalize_log(struct bpf_verifier_log *log, bpfptr_t uattr, u32 uattr_
err = bpf_vlog_finalize(log, &log_true_size);
- if (uattr_size >= offsetofend(union bpf_attr, btf_log_true_size) && - copy_to_bpfptr_offset(uattr, offsetof(union bpf_attr, btf_log_true_size), - &log_true_size, sizeof(log_true_size))) + if (bpf_field_exists(uattr_size, btf_log_true_size) && + bpfptr_put_uattr(log_true_size, uattr, btf_log_true_size)) err = -EFAULT;
return err; diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c index 517b6a5928cc..44bbd2309ce5 100644 --- a/kernel/bpf/cgroup.c +++ b/kernel/bpf/cgroup.c @@ -1061,9 +1061,9 @@ static int __cgroup_bpf_query(struct cgroup *cgrp, const union bpf_attr *attr,
/* always output uattr->query.attach_flags as 0 during effective query */ flags = effective_query ? 0 : flags; - if (copy_to_user(&uattr->query.attach_flags, &flags, sizeof(flags))) + if (bpf_put_uattr(flags, uattr, query.attach_flags)) return -EFAULT; - if (copy_to_user(&uattr->query.prog_cnt, &total_cnt, sizeof(total_cnt))) + if (bpf_put_uattr(total_cnt, uattr, query.prog_cnt)) return -EFAULT; if (attr->query.prog_cnt == 0 || !prog_ids || !total_cnt) /* return early if user requested only program count + flags */ diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c index 9901efee4339..522760031688 100644 --- a/kernel/bpf/hashtab.c +++ b/kernel/bpf/hashtab.c @@ -1693,7 +1693,7 @@ __htab_map_lookup_and_delete_batch(struct bpf_map *map, if (!max_count) return 0;
- if (put_user(0, &uattr->batch.count)) + if (bpf_put_uattr(0, uattr, batch.count)) return -EFAULT;
batch = 0; @@ -1875,7 +1875,7 @@ __htab_map_lookup_and_delete_batch(struct bpf_map *map, /* copy # of entries and next batch */ ubatch = u64_to_user_ptr(attr->batch.out_batch); if (copy_to_user(ubatch, &batch, sizeof(batch)) || - put_user(total, &uattr->batch.count)) + bpf_put_uattr(total, uattr, batch.count)) ret = -EFAULT;
out: diff --git a/kernel/bpf/net_namespace.c b/kernel/bpf/net_namespace.c index 868cc2c43899..466c7b1acb61 100644 --- a/kernel/bpf/net_namespace.c +++ b/kernel/bpf/net_namespace.c @@ -257,9 +257,9 @@ static int __netns_bpf_prog_query(const union bpf_attr *attr, if (run_array) prog_cnt = bpf_prog_array_length(run_array);
- if (copy_to_user(&uattr->query.attach_flags, &flags, sizeof(flags))) + if (bpf_put_uattr(flags, uattr, query.attach_flags)) return -EFAULT; - if (copy_to_user(&uattr->query.prog_cnt, &prog_cnt, sizeof(prog_cnt))) + if (bpf_put_uattr(prog_cnt, uattr, query.prog_cnt)) return -EFAULT; if (!attr->query.prog_cnt || !prog_ids || !prog_cnt) return 0; diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 60d1c52eda46..970d9934faa1 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -1649,7 +1649,7 @@ int generic_map_delete_batch(struct bpf_map *map, break; cond_resched(); } - if (copy_to_user(&uattr->batch.count, &cp, sizeof(cp))) + if (bpf_put_uattr(cp, uattr, batch.count)) err = -EFAULT;
kvfree(key); @@ -1707,7 +1707,7 @@ int generic_map_update_batch(struct bpf_map *map, struct file *map_file, cond_resched(); }
- if (copy_to_user(&uattr->batch.count, &cp, sizeof(cp))) + if (bpf_put_uattr(cp, uattr, batch.count)) err = -EFAULT;
kvfree(value); @@ -1742,7 +1742,7 @@ int generic_map_lookup_batch(struct bpf_map *map, if (!max_count) return 0;
- if (put_user(0, &uattr->batch.count)) + if (bpf_put_uattr(0, uattr, batch.count)) return -EFAULT;
buf_prevkey = kvmalloc(map->key_size, GFP_USER | __GFP_NOWARN); @@ -1807,8 +1807,8 @@ int generic_map_lookup_batch(struct bpf_map *map, if (err == -EFAULT) goto free_buf;
- if ((copy_to_user(&uattr->batch.count, &cp, sizeof(cp)) || - (cp && copy_to_user(uobatch, prev_key, map->key_size)))) + if (bpf_put_uattr(cp, uattr, batch.count) || + (cp && copy_to_user(uobatch, prev_key, map->key_size))) err = -EFAULT;
free_buf: @@ -3716,7 +3716,7 @@ static int bpf_obj_get_next_id(const union bpf_attr *attr, spin_unlock_bh(lock);
if (!err) - err = put_user(next_id, &uattr->next_id); + err = bpf_put_uattr(next_id, uattr, next_id);
return err; } @@ -4408,7 +4408,7 @@ static int bpf_task_fd_query_copy(const union bpf_attr *attr, u32 len = buf ? strlen(buf) : 0, input_len; int err = 0;
- if (put_user(len, &uattr->task_fd_query.buf_len)) + if (bpf_put_uattr(len, uattr, task_fd_query.buf_len)) return -EFAULT; input_len = attr->task_fd_query.buf_len; if (input_len && ubuf) { @@ -4436,10 +4436,10 @@ static int bpf_task_fd_query_copy(const union bpf_attr *attr, } }
- if (put_user(prog_id, &uattr->task_fd_query.prog_id) || - put_user(fd_type, &uattr->task_fd_query.fd_type) || - put_user(probe_offset, &uattr->task_fd_query.probe_offset) || - put_user(probe_addr, &uattr->task_fd_query.probe_addr)) + if (bpf_put_uattr(prog_id, uattr, task_fd_query.prog_id) || + bpf_put_uattr(fd_type, uattr, task_fd_query.fd_type) || + bpf_put_uattr(probe_offset, uattr, task_fd_query.probe_offset) || + bpf_put_uattr(probe_addr, uattr, task_fd_query.probe_addr)) return -EFAULT;
return err; diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index cf5f230360f5..fdec45bbbc7e 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -14253,9 +14253,7 @@ static int check_btf_func(struct bpf_verifier_env *env, /* set the size kernel expects so loader can zero * out the rest of the record. */ - if (copy_to_bpfptr_offset(uattr, - offsetof(union bpf_attr, func_info_rec_size), - &min_size, sizeof(min_size))) + if (bpfptr_put_uattr(min_size, uattr, func_info_rec_size)) ret = -EFAULT; } goto err_free; @@ -14387,9 +14385,7 @@ static int check_btf_line(struct bpf_verifier_env *env, if (err) { if (err == -E2BIG) { verbose(env, "nonzero tailing record in line_info"); - if (copy_to_bpfptr_offset(uattr, - offsetof(union bpf_attr, line_info_rec_size), - &expected_size, sizeof(expected_size))) + if (bpfptr_put_uattr(expected_size, uattr, line_info_rec_size)) err = -EFAULT; } goto err_free; @@ -14510,9 +14506,7 @@ static int check_core_relo(struct bpf_verifier_env *env, if (err) { if (err == -E2BIG) { verbose(env, "nonzero tailing record in core_relo"); - if (copy_to_bpfptr_offset(uattr, - offsetof(union bpf_attr, core_relo_rec_size), - &expected_size, sizeof(expected_size))) + if (bpfptr_put_uattr(expected_size, uattr, core_relo_rec_size)) err = -EFAULT; } break; @@ -18957,9 +18951,8 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3 if (err) ret = err;
- if (uattr_size >= offsetofend(union bpf_attr, log_true_size) && - copy_to_bpfptr_offset(uattr, offsetof(union bpf_attr, log_true_size), - &log_true_size, sizeof(log_true_size))) { + if (bpf_field_exists(uattr_size, log_true_size) && + bpfptr_put_uattr(log_true_size, uattr, log_true_size)) { ret = -EFAULT; goto err_release_maps; } diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c index 5918d1b32e19..c7bd67d75dac 100644 --- a/net/bpf/bpf_dummy_struct_ops.c +++ b/net/bpf/bpf_dummy_struct_ops.c @@ -130,7 +130,7 @@ int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr, err = dummy_ops_copy_args(args); if (err) goto out; - if (put_user(prog_ret, &uattr->test.retval)) + if (bpf_put_uattr(prog_ret, uattr, test.retval)) err = -EFAULT; out: kfree(args); diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c index e79e3a415ca9..8ffff57e13a0 100644 --- a/net/bpf/test_run.c +++ b/net/bpf/test_run.c @@ -485,11 +485,11 @@ static int bpf_test_finish(const union bpf_attr *kattr, } }
- if (copy_to_user(&uattr->test.data_size_out, &size, sizeof(size))) + if (bpf_put_uattr(size, uattr, test.data_size_out)) goto out; - if (copy_to_user(&uattr->test.retval, &retval, sizeof(retval))) + if (bpf_put_uattr(retval, uattr, test.retval)) goto out; - if (copy_to_user(&uattr->test.duration, &duration, sizeof(duration))) + if (bpf_put_uattr(duration, uattr, test.duration)) goto out; if (err != -ENOSPC) err = 0; @@ -871,7 +871,7 @@ int bpf_prog_test_run_tracing(struct bpf_prog *prog, }
retval = ((u32)side_effect << 16) | ret; - if (copy_to_user(&uattr->test.retval, &retval, sizeof(retval))) + if (bpf_put_uattr(retval, uattr, test.retval)) goto out;
err = 0; @@ -946,8 +946,7 @@ int bpf_prog_test_run_raw_tp(struct bpf_prog *prog, } put_cpu();
- if (!err && - copy_to_user(&uattr->test.retval, &info.retval, sizeof(u32))) + if (!err && bpf_put_uattr(info.retval, uattr, test.retval)) err = -EFAULT;
kfree(info.ctx); @@ -1003,7 +1002,7 @@ static int bpf_ctx_finish(const union bpf_attr *kattr,
if (copy_to_user(data_out, data, copy_size)) goto out; - if (copy_to_user(&uattr->test.ctx_size_out, &size, sizeof(size))) + if (bpf_put_uattr(size, uattr, test.ctx_size_out)) goto out; if (err != -ENOSPC) err = 0; @@ -1681,7 +1680,7 @@ int bpf_prog_test_run_syscall(struct bpf_prog *prog, retval = bpf_prog_run_pin_on_cpu(prog, ctx); rcu_read_unlock_trace();
- if (copy_to_user(&uattr->test.retval, &retval, sizeof(u32))) { + if (bpf_put_uattr(retval, uattr, test.retval)) { err = -EFAULT; goto out; } diff --git a/net/core/sock_map.c b/net/core/sock_map.c index 00afb66cd095..782e9f7799ae 100644 --- a/net/core/sock_map.c +++ b/net/core/sock_map.c @@ -1547,9 +1547,9 @@ int sock_map_bpf_prog_query(const union bpf_attr *attr, end: rcu_read_unlock();
- if (copy_to_user(&uattr->query.attach_flags, &flags, sizeof(flags)) || + if (bpf_put_uattr(flags, uattr, query.attach_flags) || (id != 0 && copy_to_user(prog_ids, &id, sizeof(u32))) || - copy_to_user(&uattr->query.prog_cnt, &prog_cnt, sizeof(prog_cnt))) + bpf_put_uattr(prog_cnt, uattr, query.prog_cnt)) ret = -EFAULT;
fdput(f);