On 23/11/2022 14:03, Kevin Brodsky wrote:
On 15/11/2022 10: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,
Realistically I think we have to do this, the approach in patch 8 is really unappealing.>
move the hooks inside each option where compat_bpf_attr has been converted into a bpf_attr.
Seeing this patch makes me slightly amend my recommendation in patch 5. Because security_bpf() needs to get the native layout regardless of the subcommand, the least invasive approach is to separate out the layout conversion from the calling of each subcommand handler. In other words, the big switch in __sys_bpf() along with the calling of security_bpf() could move to a new function, say bpf_dispatch(), and then we would get the following paths:
- Native: SYSCALL_DEFINE3(bpf) -> __sys_bpf() -> bpf_dispatch() ->
bpf_prog_load()
- Compat: COMPAT_SYSCALL_DEFINE3(bpf) -> __sys_compat_bpf() ->
bpf_dispatch() -> bpf_prog_load()
__sys_bpf() would do the same thing as before this series in terms of loading the union. __sys_compat_bpf() would load the compat union, have a big switch to convert the struct layout for each subcommand, then call the common dispatcher.
AFAICT this approach is both the most minimal (diff-wise) and the most readable, but maybe I'm missing something!
Not missing anything I think. The original idea here was to avoid a giant conversion of the entire union due to the amount of subcommands and different structs inside it. So break up the conversion and keep it near to where it's used. In hindsight a big con of this approach is it requires changes inside each subcommand and ends up being more invasive, especially with the bpf_security problem here. Then you end up with a bunch of in_compat_syscalls() inside every subcommand.
So I agree this approach above is nice, we front-load all the compat handling, all the compat handling is abstracted/hidden away pretty cleanly and resolved early. From there everything else pretty much should remain untouched. __sys_compat_bpf() is going to be large and unpretty, but as discussed offline the conversions should be relatively straightforward; there shouldn't be much subcommand specific "knowledge" that would benefit from it being closer to where it's used.
I think this should be better overall. I'll give this a go in a v2 with the other comments.
Thanks for the review.
Zach
Kevin
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);
- 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);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); }
linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org