On 15/09/2023 16:02, Kevin Brodsky wrote:
On 12/09/2023 11:51, Zachary Leaf wrote:
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.
Much clearer now, thanks!
This patch actually simplifies the existing code quite a bit, regardless of compat. It would be interesting to try and upstream these helpers (without the compat handling naturally). If they were accepted, it would make our patches more robust when rebasing (besides the obvious reduction in diff).
Possibly, at least for bpfptr_put_uattr() and bpf_field_exists().
Then the copy_to_user()'s can be replaced with put_user() where appropriate.
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>
I was going to say "no longer needed", but then realised that in fact these new macros all need bpf_compat.h for the definition of union compat_bpf_attr. Unless there's a particular issue with that, I think we should just have that #include in bpf.h, as it doesn't make much sense to have the macros there otherwise.
Sure, makes sense.
#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) \
The naming is a bit strange, though to be fair it's not easy to come up with something meaningful. Maybe bpf_has_field?
There is some precedent in libbpf/userspace, see tools/lib/bpf/bpf_core_read.h - there's bpf_core_field_exists() to check a similar thing but the other way around i.e. userspace checks if kernel has a field.
This is the exact opposite, kernel is checking if userspace has a field.
Also see: https://nakryiko.com/posts/bpf-core-reference-guide/#bpf-core-field-exists
- (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) \
The order of arguments should probably be the same in both bpf_put_uattr() and bpfptr_put_uattr(). I'm not sure which makes more sense, both look plausible.
Better to make it match closer with put_user() I think, since that is what it's trying to replicate
e.g. - if (put_user(0, &uattr->batch.count)) + if (bpf_put_uattr(0, uattr, batch.count))
Thanks, Zach
Kevin