[linux-morello] [PATCH v5 09/13] bpf: compat64: support CHECK_ATTR macro

The CHECK_ATTR macro needs adapting to support union compat_bpf_attr as well as union bpf_attr.

The union bpf_attr passed in from userspace contains many structs of various sizes for each of the bpf syscall's multiplexed options. Since most of the structs do not take up the full memory footprint of the union, we should not have any data past the last struct member for any given option.

The CHECK_ATTR macro checks for non-null data in the gap between the end of the last member and the end of the union. Any non-zero data signals a malformed request from userspace so should be rejected.

Adapt the macro to be able to handle the compat64 offsets based on in_compat64_syscall().

Since compat64 conversion involves copying out individual fields to a new native sized union, we need to check for invalid data before the conversion occurs i.e. in copy_bpf_attr_from_user.

This presents a problem since the existing macro relies on the string XYZ_LAST_FIELD called inside each sub-command handler. However at the __sys_bpf + copy_bpf_attr_from_user stage we have only int cmd to distinguish subcommands as per enum bpf_cmd, and: 1. there is no way to go cleanly from int to enum string/name 2. enum bpf_cmd does not cleanly match 1:1 in all cases for XYZ_LAST_FIELD

We can either: a) rework/refactor XYZ_LAST_FIELD system to encode by int cmd b) add a check_attr function with case statement to call the macro with the correct XYZ_LAST_FIELD argument

The latter is preferred here for being the slightly easier and cleaner approach. Rather than refactor the entire system, abstract the details away in a check_attr() function called from copy_bpf_attr_from_user().

Signed-off-by: Zachary Leaf zachary.leaf@arm.com --- kernel/bpf/syscall.c | 193 +++++++++++++++++++++++-------------------- 1 file changed, 104 insertions(+), 89 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index bad24f30e53f..22ba6cc7b0a1 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -973,12 +973,17 @@ int bpf_get_file_flag(int flags) }

/* helper macro to check that unused fields 'union bpf_attr' are zero */ +#define __CHECK_ATTR(CMD, TYPE) \ + (memchr_inv((void *) &(((TYPE *)vattr)->CMD##_LAST_FIELD) + \ + sizeof(((TYPE *)vattr)->CMD##_LAST_FIELD), 0, \ + sizeof(*(TYPE *)vattr) - \ + offsetof(TYPE, CMD##_LAST_FIELD) - \ + sizeof(((TYPE *)vattr)->CMD##_LAST_FIELD)) != NULL) + #define CHECK_ATTR(CMD) \ - memchr_inv((void *) &attr->CMD##_LAST_FIELD + \ - sizeof(attr->CMD##_LAST_FIELD), 0, \ - sizeof(*attr) - \ - offsetof(union bpf_attr, CMD##_LAST_FIELD) - \ - sizeof(attr->CMD##_LAST_FIELD)) != NULL + (in_compat64_syscall() ? \ + __CHECK_ATTR(CMD, union compat_bpf_attr) : \ + __CHECK_ATTR(CMD, union bpf_attr))

/* dst and src must have at least "size" number of bytes. * Return strlen on success and < 0 on error. @@ -1132,10 +1137,6 @@ static int map_create(union bpf_attr *attr) int f_flags; int err;

- err = CHECK_ATTR(BPF_MAP_CREATE); - if (err) - return -EINVAL; - if (attr->btf_vmlinux_value_type_id) { if (attr->map_type != BPF_MAP_TYPE_STRUCT_OPS || attr->btf_key_type_id || attr->btf_value_type_id) @@ -1365,9 +1366,6 @@ static int map_lookup_elem(union bpf_attr *attr) struct fd f; int err;

- if (CHECK_ATTR(BPF_MAP_LOOKUP_ELEM)) - return -EINVAL; - if (attr->flags & ~BPF_F_LOCK) return -EINVAL;

@@ -1440,9 +1438,6 @@ static int map_update_elem(union bpf_attr *attr, bpfptr_t uattr) struct fd f; int err;

- if (CHECK_ATTR(BPF_MAP_UPDATE_ELEM)) - return -EINVAL; - f = fdget(ufd); map = __bpf_map_get(f); if (IS_ERR(map)) @@ -1494,9 +1489,6 @@ static int map_delete_elem(union bpf_attr *attr, bpfptr_t uattr) void *key; int err;

- if (CHECK_ATTR(BPF_MAP_DELETE_ELEM)) - return -EINVAL; - f = fdget(ufd); map = __bpf_map_get(f); if (IS_ERR(map)) @@ -1550,9 +1542,6 @@ static int map_get_next_key(union bpf_attr *attr) struct fd f; int err;

- if (CHECK_ATTR(BPF_MAP_GET_NEXT_KEY)) - return -EINVAL; - f = fdget(ufd); map = __bpf_map_get(f); if (IS_ERR(map)) @@ -1830,9 +1819,6 @@ static int map_lookup_and_delete_elem(union bpf_attr *attr) struct fd f; int err;

- if (CHECK_ATTR(BPF_MAP_LOOKUP_AND_DELETE_ELEM)) - return -EINVAL; - if (attr->flags & ~BPF_F_LOCK) return -EINVAL;

@@ -1918,9 +1904,6 @@ static int map_freeze(const union bpf_attr *attr) struct bpf_map *map; struct fd f;

- if (CHECK_ATTR(BPF_MAP_FREEZE)) - return -EINVAL; - f = fdget(ufd); map = __bpf_map_get(f); if (IS_ERR(map)) @@ -2508,9 +2491,6 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size) char license[128]; bool is_gpl;

- if (CHECK_ATTR(BPF_PROG_LOAD)) - return -EINVAL; - if (attr->prog_flags & ~(BPF_F_STRICT_ALIGNMENT | BPF_F_ANY_ALIGNMENT | BPF_F_TEST_STATE_FREQ | @@ -2705,7 +2685,7 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)

static int bpf_obj_pin(const union bpf_attr *attr) { - if (CHECK_ATTR(BPF_OBJ) || attr->file_flags != 0) + if (attr->file_flags != 0) return -EINVAL;

return bpf_obj_pin_user(attr->bpf_fd, u64_to_user_ptr(attr->pathname)); @@ -2713,8 +2693,7 @@ static int bpf_obj_pin(const union bpf_attr *attr)

static int bpf_obj_get(const union bpf_attr *attr) { - if (CHECK_ATTR(BPF_OBJ) || attr->bpf_fd != 0 || - attr->file_flags & ~BPF_OBJ_FLAG_MASK) + if (attr->bpf_fd != 0 || attr->file_flags & ~BPF_OBJ_FLAG_MASK) return -EINVAL;

return bpf_obj_get_user(u64_to_user_ptr(attr->pathname), @@ -3409,9 +3388,6 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr) struct bpf_prog *prog; int fd;

- if (CHECK_ATTR(BPF_RAW_TRACEPOINT_OPEN)) - return -EINVAL; - prog = bpf_prog_get(attr->raw_tracepoint.prog_fd); if (IS_ERR(prog)) return PTR_ERR(prog); @@ -3524,9 +3500,6 @@ static int bpf_prog_attach(const union bpf_attr *attr) struct bpf_prog *prog; int ret;

- if (CHECK_ATTR(BPF_PROG_ATTACH)) - return -EINVAL; - if (attr->attach_flags & ~BPF_F_ATTACH_MASK) return -EINVAL;

@@ -3583,9 +3556,6 @@ static int bpf_prog_detach(const union bpf_attr *attr) { enum bpf_prog_type ptype;

- if (CHECK_ATTR(BPF_PROG_DETACH)) - return -EINVAL; - ptype = attach_type_to_prog_type(attr->attach_type);

switch (ptype) { @@ -3617,8 +3587,6 @@ static int bpf_prog_query(const union bpf_attr *attr, { if (!capable(CAP_NET_ADMIN)) return -EPERM; - if (CHECK_ATTR(BPF_PROG_QUERY)) - return -EINVAL; if (attr->query.query_flags & ~BPF_F_QUERY_EFFECTIVE) return -EINVAL;

@@ -3671,9 +3639,6 @@ static int bpf_prog_test_run(const union bpf_attr *attr, struct bpf_prog *prog; int ret = -ENOTSUPP;

- if (CHECK_ATTR(BPF_PROG_TEST_RUN)) - return -EINVAL; - if ((attr->test.ctx_size_in && !attr->test.ctx_in) || (!attr->test.ctx_size_in && attr->test.ctx_in)) return -EINVAL; @@ -3703,7 +3668,7 @@ static int bpf_obj_get_next_id(const union bpf_attr *attr, u32 next_id = attr->start_id; int err = 0;

- if (CHECK_ATTR(BPF_OBJ_GET_NEXT_ID) || next_id >= INT_MAX) + if (next_id >= INT_MAX) return -EINVAL;

if (!capable(CAP_SYS_ADMIN)) @@ -3784,9 +3749,6 @@ static int bpf_prog_get_fd_by_id(const union bpf_attr *attr) u32 id = attr->prog_id; int fd;

- if (CHECK_ATTR(BPF_PROG_GET_FD_BY_ID)) - return -EINVAL; - if (!capable(CAP_SYS_ADMIN)) return -EPERM;

@@ -3810,8 +3772,7 @@ static int bpf_map_get_fd_by_id(const union bpf_attr *attr) int f_flags; int fd;

- if (CHECK_ATTR(BPF_MAP_GET_FD_BY_ID) || - attr->open_flags & ~BPF_OBJ_FLAG_MASK) + if (attr->open_flags & ~BPF_OBJ_FLAG_MASK) return -EINVAL;

if (!capable(CAP_SYS_ADMIN)) @@ -4601,9 +4562,6 @@ static int bpf_obj_get_info_by_fd(const union bpf_attr *attr, struct fd f; int err;

- if (CHECK_ATTR(BPF_OBJ_GET_INFO_BY_FD)) - return -EINVAL; - f = fdget(ufd); if (!f.file) return -EBADFD; @@ -4630,9 +4588,6 @@ static int bpf_obj_get_info_by_fd(const union bpf_attr *attr,

static int bpf_btf_load(const union bpf_attr *attr, bpfptr_t uattr, __u32 uattr_size) { - if (CHECK_ATTR(BPF_BTF_LOAD)) - return -EINVAL; - if (!bpf_capable()) return -EPERM;

@@ -4643,9 +4598,6 @@ static int bpf_btf_load(const union bpf_attr *attr, bpfptr_t uattr, __u32 uattr_

static int bpf_btf_get_fd_by_id(const union bpf_attr *attr) { - if (CHECK_ATTR(BPF_BTF_GET_FD_BY_ID)) - return -EINVAL; - if (!capable(CAP_SYS_ADMIN)) return -EPERM;

@@ -4711,9 +4663,6 @@ static int bpf_task_fd_query(const union bpf_attr *attr, struct file *file; int err;

- if (CHECK_ATTR(BPF_TASK_FD_QUERY)) - return -EINVAL; - if (!capable(CAP_SYS_ADMIN)) return -EPERM;

@@ -4795,9 +4744,6 @@ static int bpf_map_do_batch(const union bpf_attr *attr, int err, ufd; struct fd f;

- if (CHECK_ATTR(BPF_MAP_BATCH)) - return -EINVAL; - ufd = attr->batch.map_fd; f = fdget(ufd); map = __bpf_map_get(f); @@ -4836,9 +4782,6 @@ static int link_create(union bpf_attr *attr, bpfptr_t uattr) struct bpf_prog *prog; int ret;

- if (CHECK_ATTR(BPF_LINK_CREATE)) - return -EINVAL; - if (attr->link_create.attach_type == BPF_STRUCT_OPS) return bpf_struct_ops_link_create(attr);

@@ -4987,9 +4930,6 @@ static int link_update(union bpf_attr *attr) u32 flags; int ret;

- if (CHECK_ATTR(BPF_LINK_UPDATE)) - return -EINVAL; - flags = attr->link_update.flags; if (flags & ~BPF_F_REPLACE) return -EINVAL; @@ -5043,9 +4983,6 @@ static int link_detach(union bpf_attr *attr) struct bpf_link *link; int ret;

- if (CHECK_ATTR(BPF_LINK_DETACH)) - return -EINVAL; - link = bpf_link_get_from_fd(attr->link_detach.link_fd); if (IS_ERR(link)) return PTR_ERR(link); @@ -5113,9 +5050,6 @@ static int bpf_link_get_fd_by_id(const union bpf_attr *attr) u32 id = attr->link_id; int fd;

- if (CHECK_ATTR(BPF_LINK_GET_FD_BY_ID)) - return -EINVAL; - if (!capable(CAP_SYS_ADMIN)) return -EPERM;

@@ -5169,9 +5103,6 @@ static int bpf_enable_runtime_stats(void) static int bpf_enable_stats(union bpf_attr *attr) {

- if (CHECK_ATTR(BPF_ENABLE_STATS)) - return -EINVAL; - if (!capable(CAP_SYS_ADMIN)) return -EPERM;

@@ -5191,9 +5122,6 @@ static int bpf_iter_create(union bpf_attr *attr) struct bpf_link *link; int err;

- if (CHECK_ATTR(BPF_ITER_CREATE)) - return -EINVAL; - if (attr->iter_create.flags) return -EINVAL;

@@ -5216,9 +5144,6 @@ static int bpf_prog_bind_map(union bpf_attr *attr) struct bpf_map **used_maps_old, **used_maps_new; int i, ret = 0;

- if (CHECK_ATTR(BPF_PROG_BIND_MAP)) - return -EINVAL; - if (attr->prog_bind_map.flags) return -EINVAL;

@@ -5269,6 +5194,90 @@ static int bpf_prog_bind_map(union bpf_attr *attr) return ret; }

+static int check_attr(enum bpf_cmd cmd, void *vattr) +{ + switch (cmd) { + case BPF_MAP_CREATE: + return CHECK_ATTR(BPF_MAP_CREATE); + case BPF_MAP_LOOKUP_ELEM: + return CHECK_ATTR(BPF_MAP_LOOKUP_ELEM); + case BPF_MAP_UPDATE_ELEM: + return CHECK_ATTR(BPF_MAP_UPDATE_ELEM); + case BPF_MAP_DELETE_ELEM: + return CHECK_ATTR(BPF_MAP_DELETE_ELEM); + case BPF_MAP_GET_NEXT_KEY: + return CHECK_ATTR(BPF_MAP_GET_NEXT_KEY); + case BPF_MAP_FREEZE: + return CHECK_ATTR(BPF_MAP_FREEZE); + case BPF_PROG_LOAD: + return CHECK_ATTR(BPF_PROG_LOAD); + case BPF_OBJ_PIN: + return CHECK_ATTR(BPF_OBJ); + case BPF_OBJ_GET: + return CHECK_ATTR(BPF_OBJ); + case BPF_PROG_ATTACH: + return CHECK_ATTR(BPF_PROG_ATTACH); + case BPF_PROG_DETACH: + return CHECK_ATTR(BPF_PROG_DETACH); + case BPF_PROG_QUERY: + return CHECK_ATTR(BPF_PROG_QUERY); + case BPF_PROG_TEST_RUN: + return CHECK_ATTR(BPF_PROG_TEST_RUN); + case BPF_PROG_GET_NEXT_ID: + return CHECK_ATTR(BPF_OBJ_GET_NEXT_ID); + case BPF_MAP_GET_NEXT_ID: + return CHECK_ATTR(BPF_OBJ_GET_NEXT_ID); + case BPF_BTF_GET_NEXT_ID: + return CHECK_ATTR(BPF_OBJ_GET_NEXT_ID); + case BPF_MAP_LOOKUP_BATCH: + return CHECK_ATTR(BPF_MAP_BATCH); + case BPF_MAP_LOOKUP_AND_DELETE_BATCH: + return CHECK_ATTR(BPF_MAP_BATCH); + case BPF_MAP_UPDATE_BATCH: + return CHECK_ATTR(BPF_MAP_BATCH); + case BPF_MAP_DELETE_BATCH: + return CHECK_ATTR(BPF_MAP_BATCH); + case BPF_PROG_GET_FD_BY_ID: + return CHECK_ATTR(BPF_PROG_GET_FD_BY_ID); + case BPF_MAP_GET_FD_BY_ID: + return CHECK_ATTR(BPF_MAP_GET_FD_BY_ID); + case BPF_OBJ_GET_INFO_BY_FD: + return CHECK_ATTR(BPF_OBJ_GET_INFO_BY_FD); + case BPF_RAW_TRACEPOINT_OPEN: + return CHECK_ATTR(BPF_RAW_TRACEPOINT_OPEN); + case BPF_BTF_LOAD: + return CHECK_ATTR(BPF_BTF_LOAD); + case BPF_BTF_GET_FD_BY_ID: + return CHECK_ATTR(BPF_BTF_GET_FD_BY_ID); + case BPF_TASK_FD_QUERY: + return CHECK_ATTR(BPF_TASK_FD_QUERY); + case BPF_MAP_LOOKUP_AND_DELETE_ELEM: + return CHECK_ATTR(BPF_MAP_LOOKUP_AND_DELETE_ELEM); + case BPF_LINK_CREATE: + return CHECK_ATTR(BPF_LINK_CREATE); + case BPF_LINK_UPDATE: + return CHECK_ATTR(BPF_LINK_UPDATE); + case BPF_LINK_GET_FD_BY_ID: + return CHECK_ATTR(BPF_LINK_GET_FD_BY_ID); + case BPF_LINK_GET_NEXT_ID: + return CHECK_ATTR(BPF_OBJ_GET_NEXT_ID); + case BPF_ENABLE_STATS: + return CHECK_ATTR(BPF_ENABLE_STATS); + case BPF_ITER_CREATE: + return CHECK_ATTR(BPF_ITER_CREATE); + case BPF_LINK_DETACH: + return CHECK_ATTR(BPF_LINK_DETACH); + case BPF_PROG_BIND_MAP: + return CHECK_ATTR(BPF_PROG_BIND_MAP); + /* + * no default case so we're warned of unhandled enum bpf_cmd + * values by -Wswitch + */ + } + + return 0; +} + static void convert_compat_bpf_attr(union bpf_attr *dest, const union compat_bpf_attr *cattr, int cmd) { @@ -5511,10 +5520,16 @@ static int copy_bpf_attr_from_user(union bpf_attr *attr, int cmd, memset(&cattr, 0, sizeof(cattr)); if (copy_from_bpfptr(&cattr, uattr, *size) != 0) return -EFAULT; + err = check_attr(cmd, &cattr); + if (err) + return -EINVAL; convert_compat_bpf_attr(attr, &cattr, cmd); } else { if (copy_from_bpfptr(attr, uattr, *size) != 0) return -EFAULT; + err = check_attr(cmd, attr); + if (err) + return -EINVAL; }

return 0;