On 18/11/2022 13:05, Zachary Leaf wrote:
On 16/11/2022 15:32, Tudor Cretu wrote:
On 15-11-2022 09:08, Zachary Leaf wrote:
The bpf syscall on a native 64 bit system originally did not need a separate compat handler for compat32. Since the union bpf_attr stored pointers as __u64 and was 8B aligned it results in the same struct offsets/memory layout for both 64/32b.
On some new architectures, e.g. Morello PCuABI, userspace pointers are 128 bit (plus 1 out-of-band tag bit), so storing pointers as __u64 is not possible. bpf_attr is therefore extended to hold 128b pointers on these archs, and remains __u64 on those that do not.
In order to support a 64b compat layer (compat64) in PCuABI, add a new union compat_bpf_attr with __u64 members. This retains the offsets and alignment of the original bpf_attr struct.
In order to handle compat64, use generic void* in __sys_bpf(...) to represent either bpf_attr or compat_bpf_attr. Then in each of the multiplexed options e.g. BPF_PROG_LOAD, if in a compat syscall, convert compat_bpf_atr into a native bpf_attr struct and handle as normal.
Since the bpf_attr union contains a number of anonymous structs, converting each option inside the option handlers e.g. bpf_prof_load() seems to be the best way to do it. After the conversion there should be minimal additional compat handling/in_compat_syscall conditions etc.
Currently unrestricted capabilities are created in the kernel here with compat_ptr so some care needs to be taken to check none of these are returned to userspace at any point.
This patch should be before PATCH 4 I think. In this way, the compat64 wouldn't be broken between PATCH 4 and PATCH 5.
Sure, but then this uses compat_ptr() to add capabilities to pointers when converting to the new bpf_attr. How does that work without first amending bpf_attr to accept capabilities? i.e. using compat_ptr and putting it in a __u64 member. Would compat64 actually work?
Seems to me it would be broken either way, but I might be thinking about it the wrong way.
Have a look at how Tudor handled this in patch 3/4 of the io_uring series. Patch 3 splits out the compat path and adds completely "flat" conversion (i.e. even pointer members are assigned as-is). Then patch 4 modifies native to use __kernel_uintptr_t for pointer members, and makes use of compat_ptr() to convert them from the compat struct. I did ponder on this for a while but in the end I think this is the best we can do.
Signed-off-by: Zachary Leaf zachary.leaf@arm.com
re: compat_ptr, there is probably another case to be made for having some kind of kernel only flag for capabilities here, to avoid them being returned to userspace by accident (as discussed for make_user_ptr_unsafe)
Forgot to comment on this - I don't see how such pointers would end up returned to userspace here. Besides, even if pointers created by compat_ptr() were returned to the caller, it would hardly be a concern as such pointers are (will be) derived from the caller's DDC, so no escalation would be happening here.
include/uapi/linux/bpf.h | 39 ++++++++++++++++++++++ kernel/bpf/syscall.c | 71 +++++++++++++++++++++++++++++++++++----- 2 files changed, 102 insertions(+), 8 deletions(-)
[...]
-static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr) +static int bpf_prog_load(void *vattr, bpfptr_t uattr) { - enum bpf_prog_type type = attr->prog_type; + enum bpf_prog_type type; struct bpf_prog *prog, *dst_prog = NULL; struct btf *attach_btf = NULL; int err; char license[128]; bool is_gpl; + union bpf_attr *attr = NULL; +#ifdef CONFIG_COMPAT + union bpf_attr compat_attr; +#endif
if (CHECK_ATTR(BPF_PROG_LOAD)) return -EINVAL;
+#ifdef CONFIG_COMPAT
I think we should use CONFIG_COMPAT64 for newly added compat code. Maybe, I'm not sure, leaving this here as I meant to change CONFIG_COMPAT -> CONFIG_COMPAT64 in the io_uring series as well.
Yeah, that was something I meant to ask about io_uring as well. CONFIG_COMPAT64 implies CONFIG_CHERI_PURECAP_UABI, so in that case the normal native 64/compat32 can apply without using the extra conversion structs etc.
In that case CONFIG_COMPAT64 is more appropriate here and in io_uring.
There's nothing functionally wrong with using CONFIG_COMPAT, but as Arnd pointed out on Tudor's patches [1], this is unnecessary overhead in compat32, so it seems preferable to use CONFIG_COMPAT64. This is conceptually different from existing compat handlers (that need to handle actual pointers in structs for instance): here we know that the layout is the same for all ABIs except PCuABI. Arguably the most semantically correct check would be CONFIG_COMPAT && CONFIG_CHERI_PURECAP_UABI, but it's probably OK to take a shortcut and assume that a layout conversion is needed in compat64.
Kevin
[1] https://git.morello-project.org/tudcre01/linux/-/commit/4441ec802dd1229c5743...