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 | 5 +++-- include/linux/bpf.h | 13 +++++++++++++ include/linux/bpfptr.h | 8 ++++++++ kernel/bpf/btf.c | 6 +++--- kernel/bpf/cgroup.c | 5 +++-- kernel/bpf/hashtab.c | 5 +++-- kernel/bpf/net_namespace.c | 5 +++-- kernel/bpf/syscall.c | 22 +++++++++++----------- kernel/bpf/verifier.c | 18 ++++++------------ net/bpf/bpf_dummy_struct_ops.c | 3 ++- net/bpf/test_run.c | 16 ++++++++-------- net/core/sock_map.c | 5 +++-- 12 files changed, 66 insertions(+), 45 deletions(-)
diff --git a/drivers/media/rc/bpf-lirc.c b/drivers/media/rc/bpf-lirc.c index fe17c7f98e81..f419d7452295 100644 --- a/drivers/media/rc/bpf-lirc.c +++ b/drivers/media/rc/bpf-lirc.c @@ -4,6 +4,7 @@ // Copyright (C) 2018 Sean Young sean@mess.org
#include <linux/bpf.h> +#include <linux/bpf_compat.h> #include <linux/filter.h> #include <linux/bpf_lirc.h> #include "rc-core-priv.h" @@ -319,12 +320,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(uattr, cnt, query.prog_cnt)) { ret = -EFAULT; goto unlock; }
- if (copy_to_user(&uattr->query.attach_flags, &flags, sizeof(flags))) { + if (bpf_put_uattr(uattr, flags, query.attach_flags)) { ret = -EFAULT; goto unlock; } diff --git a/include/linux/bpf.h b/include/linux/bpf.h index e53ceee1df37..d73442571290 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -58,6 +58,19 @@ extern struct kobject *btf_kobj; extern struct bpf_mem_alloc bpf_global_ma; extern bool bpf_global_ma_set;
+#define __bpf_put_uattr(uattr, x, to_field) \ + (put_user(x, &((uattr)->to_field))) + +#define bpf_put_uattr(uattr, x, to_field) \ + (in_compat_syscall() ? \ + __bpf_put_uattr((union compat_bpf_attr __user *)uattr, x, to_field) : \ + __bpf_put_uattr((union bpf_attr __user *)uattr, x, 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..8c4a90172eb9 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -16,6 +16,7 @@ #include <linux/kernel.h> #include <linux/idr.h> #include <linux/sort.h> +#include <linux/bpf_compat.h> #include <linux/bpf_verifier.h> #include <linux/btf.h> #include <linux/btf_ids.h> @@ -5454,9 +5455,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..fa5e67fbbbca 100644 --- a/kernel/bpf/cgroup.c +++ b/kernel/bpf/cgroup.c @@ -14,6 +14,7 @@ #include <linux/string.h> #include <linux/bpf.h> #include <linux/bpf-cgroup.h> +#include <linux/bpf_compat.h> #include <linux/bpf_lsm.h> #include <linux/bpf_verifier.h> #include <net/sock.h> @@ -1061,9 +1062,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(uattr, flags, query.attach_flags)) return -EFAULT; - if (copy_to_user(&uattr->query.prog_cnt, &total_cnt, sizeof(total_cnt))) + if (bpf_put_uattr(uattr, total_cnt, 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..acd3561a1254 100644 --- a/kernel/bpf/hashtab.c +++ b/kernel/bpf/hashtab.c @@ -3,6 +3,7 @@ * Copyright (c) 2016 Facebook */ #include <linux/bpf.h> +#include <linux/bpf_compat.h> #include <linux/btf.h> #include <linux/jhash.h> #include <linux/filter.h> @@ -1693,7 +1694,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(uattr, 0, batch.count)) return -EFAULT;
batch = 0; @@ -1875,7 +1876,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(uattr, total, batch.count)) ret = -EFAULT;
out: diff --git a/kernel/bpf/net_namespace.c b/kernel/bpf/net_namespace.c index 868cc2c43899..29ae0e3fe5bd 100644 --- a/kernel/bpf/net_namespace.c +++ b/kernel/bpf/net_namespace.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-2.0
#include <linux/bpf.h> +#include <linux/bpf_compat.h> #include <linux/bpf-netns.h> #include <linux/filter.h> #include <net/net_namespace.h> @@ -257,9 +258,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(uattr, flags, query.attach_flags)) return -EFAULT; - if (copy_to_user(&uattr->query.prog_cnt, &prog_cnt, sizeof(prog_cnt))) + if (bpf_put_uattr(uattr, prog_cnt, 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 867c51d45a83..e3ac922bd62e 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -1651,7 +1651,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(uattr, cp, batch.count)) err = -EFAULT;
kvfree(key); @@ -1709,7 +1709,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(uattr, cp, batch.count)) err = -EFAULT;
kvfree(value); @@ -1744,7 +1744,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(uattr, 0, batch.count)) return -EFAULT;
buf_prevkey = kvmalloc(map->key_size, GFP_USER | __GFP_NOWARN); @@ -1809,8 +1809,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(uattr, cp, batch.count) || + (cp && copy_to_user(uobatch, prev_key, map->key_size))) err = -EFAULT;
free_buf: @@ -3718,7 +3718,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(uattr, next_id, next_id);
return err; } @@ -4410,7 +4410,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(uattr, len, task_fd_query.buf_len)) return -EFAULT; input_len = attr->task_fd_query.buf_len; if (input_len && ubuf) { @@ -4438,10 +4438,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(uattr, prog_id, task_fd_query.prog_id) || + bpf_put_uattr(uattr, fd_type, task_fd_query.fd_type) || + bpf_put_uattr(uattr, probe_offset, task_fd_query.probe_offset) || + bpf_put_uattr(uattr, probe_addr, task_fd_query.probe_addr)) return -EFAULT;
return err; diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index cf5f230360f5..e524f2ef0e73 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -9,6 +9,7 @@ #include <linux/types.h> #include <linux/slab.h> #include <linux/bpf.h> +#include <linux/bpf_compat.h> #include <linux/btf.h> #include <linux/bpf_verifier.h> #include <linux/filter.h> @@ -14253,9 +14254,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 +14386,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 +14507,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 +18952,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..50848fbeb26c 100644 --- a/net/bpf/bpf_dummy_struct_ops.c +++ b/net/bpf/bpf_dummy_struct_ops.c @@ -5,6 +5,7 @@ #include <linux/kernel.h> #include <linux/bpf_verifier.h> #include <linux/bpf.h> +#include <linux/bpf_compat.h> #include <linux/btf.h>
extern struct bpf_struct_ops bpf_bpf_dummy_ops; @@ -130,7 +131,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(uattr, prog_ret, test.retval)) err = -EFAULT; out: kfree(args); diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c index e79e3a415ca9..899f6d2d3ff4 100644 --- a/net/bpf/test_run.c +++ b/net/bpf/test_run.c @@ -2,6 +2,7 @@ /* Copyright (c) 2017 Facebook */ #include <linux/bpf.h> +#include <linux/bpf_compat.h> #include <linux/btf.h> #include <linux/btf_ids.h> #include <linux/slab.h> @@ -485,11 +486,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(uattr, size, test.data_size_out)) goto out; - if (copy_to_user(&uattr->test.retval, &retval, sizeof(retval))) + if (bpf_put_uattr(uattr, retval, test.retval)) goto out; - if (copy_to_user(&uattr->test.duration, &duration, sizeof(duration))) + if (bpf_put_uattr(uattr, duration, test.duration)) goto out; if (err != -ENOSPC) err = 0; @@ -871,7 +872,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(uattr, retval, test.retval)) goto out;
err = 0; @@ -946,8 +947,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(uattr, info.retval, test.retval)) err = -EFAULT;
kfree(info.ctx); @@ -1003,7 +1003,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(uattr, size, test.ctx_size_out)) goto out; if (err != -ENOSPC) err = 0; @@ -1681,7 +1681,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(uattr, retval, test.retval)) { err = -EFAULT; goto out; } diff --git a/net/core/sock_map.c b/net/core/sock_map.c index 00afb66cd095..aa7522a61e30 100644 --- a/net/core/sock_map.c +++ b/net/core/sock_map.c @@ -2,6 +2,7 @@ /* Copyright (c) 2017 - 2018 Covalent IO, Inc. http://covalent.io */
#include <linux/bpf.h> +#include <linux/bpf_compat.h> #include <linux/btf_ids.h> #include <linux/filter.h> #include <linux/errno.h> @@ -1547,9 +1548,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(uattr, flags, 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(uattr, prog_cnt, query.prog_cnt)) ret = -EFAULT;
fdput(f);