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.
What you say is correct, but it's not what I was getting at.
Assume you have converted some compat struct, containing both __user ptrs and normal members, into a struct with native offsets into the kernel. You then have two types of members in the native struct after that - __user pointers, for example, the location of some memory buffer in userspace and then normal non-ptr members.
When you write out to the __user pointer you don't need to interact with or worry about the struct offsets anymore as you have a direct pointer to some user memory somewhere. You just do copy_to_user() on that __user pointer and start writing directly to the memory buffer wherever it is in userspace memory. The compat and native __user pointer is the same - it's just an address and writing to that address doesn't matter what offsets we got that address from.
If you're trying to write back to some specific field in the struct e.g. uattr->some_int in the struct, you now need to interact with or ensure you're writing at the correct offsets and do all this casting magic.
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!
I think you're seeing the distinction here as structs with pointers and structs without pointers, but really it's about how you can can write back out to some memory and where it is.
With non-ptr members, the memory you're writing to is inside the struct, so you need to ensure offsets are correct to write to correct location.
With user ptr members, the memory you're writing to is outside the struct, so you can just write directly to it, you don't need to worry about struct offsets anymore.
Hence writing out to __user pointer members doesn't require any conversion out. For everything else you need to do this magic. I'll try and rework the commit message to hopefully clear this up.
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 on all this.
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
[...]