On 12/07/2023 12:19, Kevin Brodsky wrote:
On 08/06/2023 10:42, Zachary Leaf wrote:
There are two ways that the kernel writes back to __user *bpf_attr unions and each requires different compat64 handling:
- Where the union member to be written to is a __user *ptr e.g. a pointer to some memory buffer
- Writing to a normal non-ptr field e.g. __u32 prog_cnt
In the first instance, no additional conversion out or handling is required. When converting inbound each __user *ptr is simply copied from union compat_bpf_attr into a new union bpf_attr and this can be written to directly with copy_to_user().
It took me a while but I think eventually I managed to get what you're trying to say: in that case, because there is a pointer in the struct, what we're getting from userspace is a compat layout, which we have to convert into a native struct and then we operate on that.
Assuming I got this right, this prompts another question: in the second case, how is the layout different at all between compat and native? I can't really grasp these two situations as described. My initial expectation would have been more or less the opposite: if you have pointers, then the layout is different, and when writing out you will have to do something: either write to a (temporary) native struct and then convert before writing to userspace, or write directly at the right offset with the sort of macro magic you've got in this patch. I don't immediately see how the field you're writing to makes a difference, rather the fields *before* do (as they change the offset). I'm probably missing at least a couple of things though!
For the second case, the kernel writes back out directly to user memory for a specific field. This is problematic for compat64 where the field offsets are different between native/compat64. That means if compat64 is enabled and we're in a compat syscall, we need to cast uattr to correctly select the correct member field/offset to write to.
The "technically correct" approach is probably to replace all functions with a union bpf_attr __user * parameter with void __user * (at leasta 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 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 a fairly bad macro (it's not ideal but I've seen worse). 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 mostly found by grepping 'copy_to_user(&uattr' + 'put_user(.*uattr' 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.
Signed-off-by: Zachary Leaf zachary.leaf@arm.com
[...]
diff --git a/include/linux/bpf_compat.h b/include/linux/bpf_compat.h index cc12f2e3b204..3d5f6bd2cd46 100644 --- a/include/linux/bpf_compat.h +++ b/include/linux/bpf_compat.h @@ -1,6 +1,23 @@ /* SPDX-License-Identifier: GPL-2.0 */ /* Copyright (C) 2023 Arm Ltd */ +/*
- This isn't great but the cast is necessary to select the correct member
- offset when writing out to userspace for compat64
- */
+#define __PUT_USER_UATTR(TYPE, X, TO_FIELD) \
- (put_user(X, &(((TYPE)uattr)->TO_FIELD)))
A few things:
- Public identifiers are typically loosely namespaced, this one is very
generic. Adding "bpf" somewhere in the name would help.
- Macro arguments are not normally capitalised in the kernel. In fact,
for such a function-like macro, I wouldn't capitalise its name either.
- Using implicit arguments like uattr here is highly discouraged for any
macro that's part of a public API (i.e. not a macro that's used locally in a couple of functions). It saves some typing but hides interactions with the argument (it's quite common to look at how a variable is used by grepping it, and uattr wouldn't show up here). For that matter I find the existing CHECK_ATTR() pretty borderline, but at least it is local to syscall.c.
Also I think these macros could go directly in bpf.h - they are not specific to compat, they just have a different implementation in compat.
Ack to all the above.
Turns out I also missed some places where we write to userspace using copy_to_bpfptr_offset() - in v2 there's another macro called bpfptr_put_uattr() to handle these cases.
Thanks,
Zach
Kevin
+#ifdef CONFIG_COMPAT64 +#define PUT_USER_UATTR(X, TO_FIELD) \
- (in_compat_syscall() ? \
__PUT_USER_UATTR(union compat_bpf_attr __user *, X, TO_FIELD) : \__PUT_USER_UATTR(union bpf_attr __user *, X, TO_FIELD))+#else +#define PUT_USER_UATTR(X, TO_FIELD) \
- __PUT_USER_UATTR(union bpf_attr __user *, X, TO_FIELD)
+#endif
[...]