| Message ID | 525b99881dc144b986e381eb23b12617a311f243.1652772731.git.esyr@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Fix 32-bit arch and compat support for the kprobe_multi attach type | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for Kernel LATEST on z15 with gcc |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for Kernel LATEST on ubuntu-latest with gcc |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for Kernel LATEST on ubuntu-latest with llvm-15 |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 11 this patch: 11 |
| netdev/cc_maintainers | success | CCed 12 of 12 maintainers |
| netdev/build_clang | success | Errors and warnings before: 9 this patch: 9 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 11 this patch: 11 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 52 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Tue, May 17, 2022 at 09:36:26AM +0200, Eugene Syromiatnikov wrote: > It seems that there is no reason not to support 32-bit architectures; > doing so requires a bit of rework with respect to cookies handling, > however, as the current code implicitly assumes > that sizeof(long) == sizeof(u64). > > Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com> > --- > kernel/trace/bpf_trace.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index 9c041be..a93a54f 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -2435,16 +2435,12 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr > struct bpf_link_primer link_primer; > void __user *ucookies; > unsigned long *addrs; > - u32 flags, cnt, size; > + u32 flags, cnt, size, cookies_size; > void __user *uaddrs; > u64 *cookies = NULL; > void __user *usyms; > int err; > > - /* no support for 32bit archs yet */ > - if (sizeof(u64) != sizeof(void *)) > - return -EOPNOTSUPP; > - > if (prog->expected_attach_type != BPF_TRACE_KPROBE_MULTI) > return -EINVAL; > > @@ -2454,6 +2450,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr > > uaddrs = u64_to_user_ptr(attr->link_create.kprobe_multi.addrs); > usyms = u64_to_user_ptr(attr->link_create.kprobe_multi.syms); > + ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies); > if (!!uaddrs == !!usyms) > return -EINVAL; > > @@ -2461,8 +2458,11 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr > if (!cnt) > return -EINVAL; > > - if (check_mul_overflow(cnt, (u32)sizeof(*addrs), &size)) > + if (check_mul_overflow(cnt, (u32)sizeof(*addrs), &size) || > + (ucookies && > + check_mul_overflow(cnt, (u32)sizeof(*cookies), &cookies_size))) { > return -EOVERFLOW; > + } > addrs = kvmalloc(size, GFP_KERNEL); > if (!addrs) > return -ENOMEM; > @@ -2486,14 +2486,13 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr > goto error; > } > > - ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies); > if (ucookies) { could we check all that in here? so the ucookies checks are on the one place.. also you would not need cookies_size jirka > - cookies = kvmalloc(size, GFP_KERNEL); > + cookies = kvmalloc(cookies_size, GFP_KERNEL); > if (!cookies) { > err = -ENOMEM; > goto error; > } > - if (copy_from_user(cookies, ucookies, size)) { > + if (copy_from_user(cookies, ucookies, cookies_size)) { > err = -EFAULT; > goto error; > } > -- > 2.1.4 >
On Tue, May 17, 2022 at 12:36 AM Eugene Syromiatnikov <esyr@redhat.com> wrote: > > It seems that there is no reason not to support 32-bit architectures; > doing so requires a bit of rework with respect to cookies handling, > however, as the current code implicitly assumes > that sizeof(long) == sizeof(u64). > > Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com> > --- > kernel/trace/bpf_trace.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index 9c041be..a93a54f 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -2435,16 +2435,12 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr > struct bpf_link_primer link_primer; > void __user *ucookies; > unsigned long *addrs; > - u32 flags, cnt, size; > + u32 flags, cnt, size, cookies_size; > void __user *uaddrs; > u64 *cookies = NULL; > void __user *usyms; > int err; > > - /* no support for 32bit archs yet */ > - if (sizeof(u64) != sizeof(void *)) > - return -EOPNOTSUPP; > - > if (prog->expected_attach_type != BPF_TRACE_KPROBE_MULTI) > return -EINVAL; > > @@ -2454,6 +2450,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr > > uaddrs = u64_to_user_ptr(attr->link_create.kprobe_multi.addrs); > usyms = u64_to_user_ptr(attr->link_create.kprobe_multi.syms); > + ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies); > if (!!uaddrs == !!usyms) > return -EINVAL; > > @@ -2461,8 +2458,11 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr > if (!cnt) > return -EINVAL; > > - if (check_mul_overflow(cnt, (u32)sizeof(*addrs), &size)) > + if (check_mul_overflow(cnt, (u32)sizeof(*addrs), &size) || > + (ucookies && > + check_mul_overflow(cnt, (u32)sizeof(*cookies), &cookies_size))) { > return -EOVERFLOW; > + } > addrs = kvmalloc(size, GFP_KERNEL); > if (!addrs) > return -ENOMEM; > @@ -2486,14 +2486,13 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr > goto error; > } > > - ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies); > if (ucookies) { > - cookies = kvmalloc(size, GFP_KERNEL); > + cookies = kvmalloc(cookies_size, GFP_KERNEL); same question about consistent use of kvmalloc_array() and delegating all the overflow checks to it? > if (!cookies) { > err = -ENOMEM; > goto error; > } > - if (copy_from_user(cookies, ucookies, size)) { > + if (copy_from_user(cookies, ucookies, cookies_size)) { > err = -EFAULT; > goto error; > } > -- > 2.1.4 >
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index 9c041be..a93a54f 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -2435,16 +2435,12 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr struct bpf_link_primer link_primer; void __user *ucookies; unsigned long *addrs; - u32 flags, cnt, size; + u32 flags, cnt, size, cookies_size; void __user *uaddrs; u64 *cookies = NULL; void __user *usyms; int err; - /* no support for 32bit archs yet */ - if (sizeof(u64) != sizeof(void *)) - return -EOPNOTSUPP; - if (prog->expected_attach_type != BPF_TRACE_KPROBE_MULTI) return -EINVAL; @@ -2454,6 +2450,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr uaddrs = u64_to_user_ptr(attr->link_create.kprobe_multi.addrs); usyms = u64_to_user_ptr(attr->link_create.kprobe_multi.syms); + ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies); if (!!uaddrs == !!usyms) return -EINVAL; @@ -2461,8 +2458,11 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr if (!cnt) return -EINVAL; - if (check_mul_overflow(cnt, (u32)sizeof(*addrs), &size)) + if (check_mul_overflow(cnt, (u32)sizeof(*addrs), &size) || + (ucookies && + check_mul_overflow(cnt, (u32)sizeof(*cookies), &cookies_size))) { return -EOVERFLOW; + } addrs = kvmalloc(size, GFP_KERNEL); if (!addrs) return -ENOMEM; @@ -2486,14 +2486,13 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr goto error; } - ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies); if (ucookies) { - cookies = kvmalloc(size, GFP_KERNEL); + cookies = kvmalloc(cookies_size, GFP_KERNEL); if (!cookies) { err = -ENOMEM; goto error; } - if (copy_from_user(cookies, ucookies, size)) { + if (copy_from_user(cookies, ucookies, cookies_size)) { err = -EFAULT; goto error; }
It seems that there is no reason not to support 32-bit architectures; doing so requires a bit of rework with respect to cookies handling, however, as the current code implicitly assumes that sizeof(long) == sizeof(u64). Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com> --- kernel/trace/bpf_trace.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)