On 16/11/2022 16:49, Tudor Cretu wrote:
On 15-11-2022 09:08, Zachary Leaf wrote:
With the addition of compat64 syscall support to eBPF, attr is now a void* either to a union bpf_attr or compat_bpf_attr. This breaks the Linux Security Module (LSM) hook security_bpf that takes a bpf_attr param.
In order to keep the LSM hook and not break existing security modules, move the hooks inside each option where compat_bpf_attr has been converted into a bpf_attr.
Signed-off-by: Zachary Leaf zachary.leaf@arm.com
include/linux/security.h | 4 ++-- kernel/bpf/syscall.c | 12 ++++++------ security/security.c | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/include/linux/security.h b/include/linux/security.h index f0ad6571d067..25b3ef71f495 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -1965,7 +1965,7 @@ struct bpf_map; struct bpf_prog; struct bpf_prog_aux; #ifdef CONFIG_SECURITY -extern int security_bpf(int cmd, void *attr, unsigned int size); +extern int security_bpf(int cmd, union bpf_attr *attr, unsigned int size); extern int security_bpf_map(struct bpf_map *map, fmode_t fmode); extern int security_bpf_prog(struct bpf_prog *prog); extern int security_bpf_map_alloc(struct bpf_map *map); @@ -1973,7 +1973,7 @@ extern void security_bpf_map_free(struct bpf_map *map); extern int security_bpf_prog_alloc(struct bpf_prog_aux *aux); extern void security_bpf_prog_free(struct bpf_prog_aux *aux); #else -static inline int security_bpf(int cmd, void *attr, +static inline int security_bpf(int cmd, union bpf_attr *attr, unsigned int size) { return 0; diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 910544bcae6e..aa4ebdb92a18 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -2252,7 +2252,7 @@ static void convert_compat_bpf_prog_load(const void *vattr, union bpf_attr *dest /* last field in 'union bpf_attr' used by this command */ #define BPF_PROG_LOAD_LAST_FIELD core_relo_rec_size -static int bpf_prog_load(void *vattr, bpfptr_t uattr) +static int bpf_prog_load(void *vattr, bpfptr_t uattr, size_t size) { enum bpf_prog_type type; struct bpf_prog *prog, *dst_prog = NULL; @@ -2277,6 +2277,10 @@ static int bpf_prog_load(void *vattr, bpfptr_t uattr) #endif type = attr->prog_type; + err = security_bpf(BPF_PROG_LOAD, attr, size);
I am not sure how security_bpf works, but is "size" correct here in compat64 anymore? attr would be populated with some more size than the size received from userspace if there were compat_ptr converted. Maybe size needs to be recalculated?
Ah yeah. I think this is just plainly wrong. Will fix.
In any case, I think this version is more suitable than PATCH 8.
Seems to be the consensus so far.
Thanks, Zach
Thanks, Tudor
+ if (err < 0) + return err;
if (attr->prog_flags & ~(BPF_F_STRICT_ALIGNMENT | BPF_F_ANY_ALIGNMENT | BPF_F_TEST_STATE_FREQ | @@ -4703,10 +4707,6 @@ static int __sys_bpf(int cmd, bpfptr_t uattr, unsigned int size) if (copy_from_bpfptr_with_ptr(vattr, uattr, size) != 0) return -EFAULT; - err = security_bpf(cmd, vattr, size); - if (err < 0) - return err;
switch (cmd) { case BPF_MAP_CREATE: err = map_create(&attr); @@ -4727,7 +4727,7 @@ static int __sys_bpf(int cmd, bpfptr_t uattr, unsigned int size) err = map_freeze(&attr); break; case BPF_PROG_LOAD: - err = bpf_prog_load(vattr, uattr); + err = bpf_prog_load(vattr, uattr, size); break; case BPF_OBJ_PIN: err = bpf_obj_pin(&attr); diff --git a/security/security.c b/security/security.c index 2c5e8cf47248..b7cf5cbfdc67 100644 --- a/security/security.c +++ b/security/security.c @@ -2587,7 +2587,7 @@ int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule) #endif /* CONFIG_AUDIT */ #ifdef CONFIG_BPF_SYSCALL -int security_bpf(int cmd, void *attr, unsigned int size) +int security_bpf(int cmd, union bpf_attr *attr, unsigned int size) { return call_int_hook(bpf, 0, cmd, attr, size); }