On 08/06/2023 10:42, Zachary Leaf wrote:
The CHECK_ATTR macro needs adapting to support union compat_bpf_attr as well as union bpf_attr.
The union bpf_attr passed in from userspace contains many structs of various sizes for each of the bpf syscall's multiplexed options. Since most of the structs do not take up the full memory footprint of the union, we should not have any data past the last struct member for any given option.
The CHECK_ATTR macro checks for non-null data in the gap between the end of the last member and the end of the union. Any non-zero data signals a malformed request from userspace so should be rejected.
Adapt the macro to be able to handle the compat64 offsets based on in_compat_syscall().
In order to check the compat64 union passed in from userspace, we now need to check before the conversion occurs i.e. in __compat_sys_bpf or we will not be able to catch invalid data from userspace here.
This presents a problem since the existing MACRO relies on the string XYZ_LAST_FIELD called inside each sub-command handler. However at the __sys_bpf and __compat_sys_bpf stage we have only int cmd to distinguish subcommands as per enum bpf_cmd, and: 1. there is no way to go cleanly from int to enum string/name 2. enum bpf_cmd does not cleanly match 1:1 in all cases for XYZ_LAST_FIELD
We can either: a) rework/refactor XYZ_LAST_FIELD system to encode by int cmd b) add a check_attr function with case statement to call the macro with the correct XYZ_LAST_FIELD argument
The latter is preferred here for being the slightly easier and cleaner approach. Rather than refactor the entire system, abstract the details away in a check_attr() function called from __sys_bpf + __sys_compat_bpf.
I've pondered on that issue for a while and I tend to agree with your approach. The move of CHECK_ATTR() from each handler to a big switch is a bit unfortunate but I don't have a better idea. The fact that we still use *_LAST_FIELD in compat64 is quite nice.
Signed-off-by: Zachary Leaf zachary.leaf@arm.com
[...]
#ifdef CONFIG_COMPAT64 static void convert_compat_bpf_attr(union bpf_attr *dest, const union compat_bpf_attr *cattr, int cmd) { @@ -5675,6 +5672,10 @@ static int __sys_bpf(int cmd, bpfptr_t uattr, unsigned int size) if (copy_from_bpfptr_with_ptr(&attr, uattr, size) != 0) return -EFAULT;
- err = check_attr(cmd, (void *)&attr);
No need for the cast, void * is implicitly convertible to and from any pointer type (in C). Same below.
Kevin
- if (err)
return -EINVAL;- return dispatch_bpf(cmd, &attr, uattr, size);
} @@ -5704,6 +5705,10 @@ static int __sys_compat_bpf(int cmd, bpfptr_t uattr, unsigned int size) if (copy_from_bpfptr_with_ptr(&cattr, uattr, size) != 0) return -EFAULT;
- err = check_attr(cmd, (void *)&cattr);
- if (err)
return -EINVAL;- convert_compat_bpf_attr(&attr, &cattr, cmd);
return dispatch_bpf(cmd, &attr, uattr, sizeof(attr));