Nit (in title): "xyz" might be misinterpreted :D bpf_*_info would be clearer.
On 12/09/2023 11:51, Zachary Leaf wrote:
[...]
+static int copy_bpf_btf_info_from_user(const union bpf_attr *attr,
struct bpf_btf_info *info,u32 *info_len)+{
- struct compat_bpf_btf_info cinfo;
- int err;
- void *select_info = in_compat64_syscall() ? &cinfo : info;
- size_t info_size = in_compat64_syscall() ? sizeof(struct compat_bpf_btf_info)
: sizeof(struct bpf_btf_info);- void __user *uinfo = u64_to_user_ptr(attr->info.info);
Nit: there is normally always an empty line between declarations and the first non-declaration statement.
- *info_len = attr->info.info_len;
- err = bpf_check_uarg_tail_zero(USER_BPFPTR(uinfo),
info_size, *info_len);
It took me a long time to figure out why this suddenly appeared - eventually I realised that it got moved from bpf_btf_get_info_by_fd(). It's a small thing but could we still have it in a separate patch? That will also make the diff in syscall.c less noisy around that function.
- if (err)
return err;- *info_len = min_t(u32, info_size, *info_len);
- memset(info, 0, sizeof(struct bpf_btf_info));
Can use sizeof(*info), similarly to the original code.
Thinking some more about the zeroing, having looked at some other conversion functions, I feel that we might be missing some. There are two situations where zeroing is crucial: 1. When copying in and the user-supplied struct is smaller than the kernel one. If we miss zeroing, then we're going to read garbage from the last struct members. 2. When copying out the entire struct. We should have set all the members at this point, but if we miss zeroing then any potential padding will be garbage. This is even worse than 1. as arbitrary kernel data should never be written to user memory.
2. was not an issue before because that padding was provided by userspace, when we copy_from_user(). Overall because we now have both a native and a compat path, it gets harder to track if the zeroing is right or not. In this case, I think we're missing 1. in the compat case, since we don't zero the compat struct and then read it to initialise members of the native struct. I sense that in general we should zero both the native and compat structs.
It's worth mentioning that because Morello Clang (possibly GCC too) supports it, we get CONFIG_INIT_STACK_ALL_ZERO=y by default (see security/Kconfig.hardening). This means that we won't actually see whether we get zeroing right or not, in fact the codegen should be unchanged. We could verify it using CONFIG_INIT_STACK_ALL_PATTERN=y, but I don't think it's worth the hassle. Let's just try our best to be consistent.
- if (copy_from_user(select_info, uinfo, *info_len))
return -EFAULT;- if (in_compat64_syscall())
convert_compat_btf_info_in(info, &cinfo);- return 0;
+}
+static int copy_bpf_btf_info_to_user(const union bpf_attr *attr,
union bpf_attr __user *uattr,struct bpf_btf_info *info,u32 *info_len)+{
- struct compat_bpf_btf_info cinfo;
- void *select_info = in_compat64_syscall() ? &cinfo : info;
- void __user *uinfo = (void __user *)attr->info.info;
That won't build, you could stick to u64_to_user_ptr() as this is temporary anyway.
- if (in_compat64_syscall())
convert_compat_btf_info_out(&cinfo, info);- if (copy_to_user(uinfo, select_info, *info_len) ||
bpf_put_uattr(uattr, *info_len, info.info_len))return -EFAULT;- return 0;
+}
int btf_get_info_by_fd(const struct btf *btf, const union bpf_attr *attr, union bpf_attr __user *uattr) {
- struct bpf_btf_info __user *uinfo; struct bpf_btf_info info;
- u32 info_copy, btf_copy;
- u32 btf_copy; void __user *ubtf; char __user *uname; u32 uinfo_len, uname_len, name_len; int ret = 0;
Nit: better remove the 0 initialiser since ret is now assigned just after.
- uinfo = u64_to_user_ptr(attr->info.info);
- uinfo_len = attr->info.info_len;
- info_copy = min_t(u32, uinfo_len, sizeof(info));
- memset(&info, 0, sizeof(info));
- if (copy_from_user(&info, uinfo, info_copy))
return -EFAULT;
- ret = copy_bpf_btf_info_from_user(attr, &info, &uinfo_len);
- if (ret)
return ret;info.id = btf->id; ubtf = u64_to_user_ptr(info.btf); @@ -7268,9 +7332,9 @@ int btf_get_info_by_fd(const struct btf *btf, } }
- if (copy_to_user(uinfo, &info, info_copy) ||
put_user(info_copy, &uattr->info.info_len))return -EFAULT;
- ret = copy_bpf_btf_info_to_user(attr, uattr, &info, &uinfo_len);
There is a semantic change here in case ret is set to something else than 0 (3 lines above). Given that these out functions only return -EFAULT as an error, we may be better off keeping the existing style in general, that is if (copy...) return -EFAULT.
[...]
+static void +convert_compat_link_info_out(struct compat_bpf_link_info *dest,
const struct bpf_link_info *info,enum bpf_link_type type)+{
- copy_field(dest, info, type);
- copy_field(dest, info, id);
- copy_field(dest, info, prog_id);
- switch (type) {
- case BPF_LINK_TYPE_RAW_TRACEPOINT:
copy_field(dest, info, raw_tracepoint.tp_name);copy_field(dest, info, raw_tracepoint.tp_name_len);return;- case BPF_LINK_TYPE_TRACING:
copy_field(dest, info, tracing.attach_type);copy_field(dest, info, tracing.target_obj_id);copy_field(dest, info, tracing.target_btf_id);return;- case BPF_LINK_TYPE_CGROUP:
copy_field(dest, info, cgroup.cgroup_id);copy_field(dest, info, cgroup.attach_type);return;- case BPF_LINK_TYPE_ITER:
copy_field(dest, info, iter.target_name);copy_field(dest, info, iter.target_name_len);/* remaining struct is fixed size integers, so identical in* purecap/compat64 - since figuring out what members are active
Nit: to avoid being too specific in generic code, we could just say "any 64-bit ABI" (excluding 32-bit because alignof(__u64) is 4 on x86_32, unlike any other ABI). This is compat64-specific so 32-bit ABIs are irrelevant.
* is non-trivial, memcpy to the end of the struct */memcpy((u8 *)dest+offsetof(struct compat_bpf_link_info, iter.map),(u8 *)info+offsetof(struct bpf_link_info, iter.map),offsetofend(struct bpf_link_info, iter.cgroup.order) -offsetof(struct bpf_link_info, iter.map.map_id));
We could slightly simplify this by referring to the structs and not the innermost members, that is iter.map instead of iter.map_id and iter.cgroup instead of iter.cgroup.order. The latter may result in a slightly larger copy size due to padding, but that's not an issue because we (should) have zeroed that padding to start with (see comment above). In any case, this memcpy() relies on any padding being initialised.
Kevin