Fix kprobe_multi interface issues for 5.18
Subsystems (6)
v3 cover Fix kprobe_multi interface issues for 5.18
Hello.
While [1] seems to require additional work[2] due to changes in the interface (and it has already been re-targeted for bpf-next), I would like to ask to consider the following two patches, that fix possible out-of-bounds write and properly disable the interface for 32-bit compat user space, for the 5.18 release. Thank you.
[1] https://lore.kernel.org/lkml/[email protected]/
[2] https://lore.kernel.org/lkml/YoTXiAk1EpZ0rLKE@krava/
Eugene Syromiatnikov (2):
bpf_trace: check size for overflow in bpf_kprobe_multi_link_attach
bpf_trace: bail out from bpf_kprobe_multi_link_attach when in compat
kernel/trace/bpf_trace.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
Signature · 1 line
2.1.4
PATCH v3 1/2 bpf_trace: check size for overflow in bpf_kprobe_multi_link_attach
Check that size would not overflow before calculation (and return -EOVERFLOW if it will), to prevent potential out-of-bounds write with the following copy_from_user. Add the same check
to kprobe_multi_resolve_syms in case it will be called from elsewhere in the future.
--- kernel/trace/bpf_trace.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
kernel/trace/bpf_trace.c +8 -5
@@ -2352,13 +2352,15 @@ static int kprobe_multi_resolve_syms(const void __user *usyms, u32 cnt, unsigned long *addrs) { - unsigned long addr, size; + unsigned long addr, sym_size; +u32 size;const char __user **syms;int err = -ENOMEM ;unsigned int i;char *func; - size = cnt *sizeof (*syms); +if (check_mul_overflow (cnt, (u32 )sizeof (*syms), &size)) +return -EOVERFLOW ; syms =kvzalloc (size,GFP_KERNEL );if (!syms)return -ENOMEM ;
@@ -2382,9 +2384,9 @@ kprobe_multi_resolve_syms(const void __user *usyms, u32 cnt, addr =kallsyms_lookup_name (func);if (!addr) goto error; -if (!kallsyms_lookup_size_offset (addr, &size,NULL )) +if (!kallsyms_lookup_size_offset (addr, &sym_size,NULL )) goto error; - addr =ftrace_location_range (addr, addr + size -1 ); + addr =ftrace_location_range (addr, addr + sym_size -1 );if (!addr) goto error; addrs[i] = addr;
@@ -2429,7 +2431,8 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prif (!cnt)return -EINVAL ; - size = cnt *sizeof (*addrs); +if (check_mul_overflow (cnt, (u32 )sizeof (*addrs), &size)) +return -EOVERFLOW ; addrs =kvmalloc (size,GFP_KERNEL );if (!addrs)return -ENOMEM ;
Signature · 1 line
2.1.4
Re: [PATCH bpf v3 1/2] bpf_trace: check size for overflow in bpf_kprobe_multi_link_attach
Quoted 21 lines · Eugene Syromiatnikov
Check that size would not overflow before calculation (and return
-EOVERFLOWif it will), to prevent potential out-of-bounds write with the followingcopy_from_user. Add the same check
tokprobe_multi_resolve_symsin case it will be called from elsewhere in the future.Fixes 0dcac272540613d4 ("bpf: Add multi kprobe link")Signed-off-by Eugene Syromiatnikov <[email protected]>--- kernel/trace/bpf_trace.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)kernel/trace/bpf_trace.c +0
-1@@ -2352,13 +2352,15 @@ static int kprobe_multi_resolve_syms(const void __user *usyms, u32 cnt, unsigned long *addrs) { - unsigned long addr, size;
kernel/trace/bpf_trace.c static int · line 2363
Quoted 10 lines
kernel/trace/bpf_trace.c +4
-1+unsigned long addr, sym_size; +u32 size;const char __user **syms;int err = -ENOMEM ;unsigned int i;char *func; - size = cnt *sizeof (*syms); +if (check_mul_overflow (cnt, (u32 )sizeof (*syms), &size)) +return -EOVERFLOW ;
In mm/util.c kvmalloc_node(), we have
/* Don't even allow crazy sizes */
if (unlikely(size > INT_MAX)) {
WARN_ON_ONCE(!(flags & __GFP_NOWARN));
return NULL;
}Basically the maximum size to be allocated in INT_MAX.
Here, we have 'size' as u32, which means if the size is 0xffff0000, the check_mul_overflow will return false (no overflow) but kvzalloc will still have a warning.
I think we should change the type of 'size' to be 'int' which should catch the above case and be consistent with what kvmalloc_node() intends to warn.
Quoted 25 lines
kernel/trace/bpf_trace.c +4
-3syms =kvzalloc (size,GFP_KERNEL );if (!syms)return -ENOMEM ;@@ -2382,9 +2384,9 @@ kprobe_multi_resolve_syms(const void __user *usyms, u32 cnt, addr =kallsyms_lookup_name (func);if (!addr) goto error; -if (!kallsyms_lookup_size_offset (addr, &size,NULL )) +if (!kallsyms_lookup_size_offset (addr, &sym_size,NULL )) goto error; - addr =ftrace_location_range (addr, addr + size -1 ); + addr =ftrace_location_range (addr, addr + sym_size -1 );if (!addr) goto error; addrs[i] = addr;@@ -2429,7 +2431,8 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prif (!cnt)return -EINVAL ; - size = cnt *sizeof (*addrs); +if (check_mul_overflow (cnt, (u32 )sizeof (*addrs), &size)) +return -EOVERFLOW ; addrs =kvmalloc (size,GFP_KERNEL );if (!addrs)return -ENOMEM ;
Re: [PATCH bpf v3 1/2] bpf_trace: check size for overflow in bpf_kprobe_multi_link_attach
Quoted 15 lines · Yonghong Song
kernel/trace/bpf_trace.c static int · line 2363
Quoted 3 lines · Eugene Syromiatnikov
kernel/trace/bpf_trace.c +2
-1- size = cnt *sizeof (*syms); +if (check_mul_overflow (cnt, (u32 )sizeof (*syms), &size)) +return -EOVERFLOW ;In
mm/util.ckvmalloc_node(), we have/* Don't even allow crazy sizes */ if (unlikely(size > INT_MAX)) { WARN_ON_ONCE(!(flags & __GFP_NOWARN)); return NULL; }Basically the maximum size to be allocated in
INT_MAX.
Quoted 7 lines
Here, we have 'size' as u32, which means if the size is
0xffff0000, thecheck_mul_overflowwill return false (no overflow) but kvzalloc will still have a warning.I think we should change the type of 'size' to be 'int' which should catch the above case and be consistent with what
kvmalloc_node()intends to warn.
Huh, it's a bitmore complicated as check_mul_overflow requires types to match; what do you think about
+ if (check_mul_overflow(cnt, (u32)sizeof(*syms), &size) || size > INT_MAX)
?
Re: [PATCH bpf v3 1/2] bpf_trace: check size for overflow in bpf_kprobe_multi_link_attach
Quoted 29 lines · Eugene Syromiatnikov
Quoted 15 lines · Yonghong Song
kernel/trace/bpf_trace.c static int · line 2363
Quoted 3 lines · Eugene Syromiatnikov
kernel/trace/bpf_trace.c +2
-1- size = cnt *sizeof (*syms); +if (check_mul_overflow (cnt, (u32 )sizeof (*syms), &size)) +return -EOVERFLOW ;In
mm/util.ckvmalloc_node(), we have/* Don't even allow crazy sizes */ if (unlikely(size > INT_MAX)) { WARN_ON_ONCE(!(flags & __GFP_NOWARN)); return NULL; }Basically the maximum size to be allocated in
INT_MAX.Quoted 7 lines
Here, we have 'size' as u32, which means if the size is
0xffff0000, thecheck_mul_overflowwill return false (no overflow) but kvzalloc will still have a warning.I think we should change the type of 'size' to be 'int' which should catch the above case and be consistent with what
kvmalloc_node()intends to warn.Huh, it's a bitmore complicated as
check_mul_overflowrequires types to match; what do you think about(quoted diff) +1
-0+ if (check_mul_overflow(cnt, (u32)sizeof(*syms), &size) || size > INT_MAX)
Quoted 1 line
?
This works for me.
PATCH v3 2/2 bpf_trace: bail out from bpf_kprobe_multi_link_attach when in compat
In reply to Eugene SyromiatnikovSince bpf_kprobe_multi_link_attach doesn't support 32-bit kernels for whatever reason, having it enabled for compat processes on 64-bit kernels makes even less sense due to discrepances in the type sizes that it does not handle.
--- kernel/trace/bpf_trace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/trace/bpf_trace.c +1 -1
@@ -2412,7 +2412,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *print err;/* no support for 32bit archs yet */ -if (sizeof (u64) !=sizeof (void *)) +if (sizeof (u64) !=sizeof (void *) ||in_compat_syscall ())return -EOPNOTSUPP ;if (prog->expected_attach_type !=BPF_TRACE_KPROBE_MULTI )
Signature · 1 line
2.1.4
Re: [PATCH bpf v3 2/2] bpf_trace: bail out from bpf_kprobe_multi_link_attach when in compat
Quoted 4 lines · Eugene Syromiatnikov
Since
bpf_kprobe_multi_link_attachdoesn't support 32-bit kernels for whatever reason, having it enabled for compat processes on 64-bit kernels makes even less sense due to discrepances in the type sizes that it does not handle.
If I understand correctly, the reason is due to
in libbpf we have
struct bpf_link_create_opts {
size_t sz; /* size of this struct for forward/backward
compatibility */
__u32 flags;
union bpf_iter_link_info *iter_info;
__u32 iter_info_len;
__u32 target_btf_id;
union {
struct {
__u64 bpf_cookie;
} perf_event;
struct {
__u32 flags;
__u32 cnt;
const char **syms;
const unsigned long *addrs;
const __u64 *cookies;
} kprobe_multi;
};
size_t :0;
};Note that we have const unsigned long *addrs;
If we have 32-bit user space application and 64bit kernel, and we will have userspace 32-bit pointers and kernel as 64bit pointers and current kernel doesn't handle 32-bit user pointer properly.
Consider this may involve libbpf uapi change, maybe we should change "const unsigned long *addrs;" to "const __u64 *addrs;" considering we haven't freeze libbpf UAPI yet.
Otherwise, we stick to current code with this patch, it will make it difficult to support 32-bit app with 64-bit kernel for kprobe_multi in the future due to uapi issues.
WDYT?
Quoted 20 lines
Fixes 0dcac272540613d4 ("bpf: Add multi kprobe link")Signed-off-by Eugene Syromiatnikov <[email protected]>--- kernel/trace/bpf_trace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)kernel/trace/bpf_trace.c +1
-1@@ -2412,7 +2412,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *print err;/* no support for 32bit archs yet */ -if (sizeof (u64) !=sizeof (void *)) +if (sizeof (u64) !=sizeof (void *) ||in_compat_syscall ())return -EOPNOTSUPP ;if (prog->expected_attach_type !=BPF_TRACE_KPROBE_MULTI )
Re: [PATCH bpf v3 2/2] bpf_trace: bail out from bpf_kprobe_multi_link_attach when in compat
Quoted 44 lines · Yonghong Song
Quoted 4 lines · Eugene Syromiatnikov
Since
bpf_kprobe_multi_link_attachdoesn't support 32-bit kernels for whatever reason, having it enabled for compat processes on 64-bit kernels makes even less sense due to discrepances in the type sizes that it does not handle.If I understand correctly, the reason is due to
in libbpf we have
structbpf_link_create_opts{
size_tsz; /* size of this struct for forward/backward compatibility
*/__u32 flags; union bpf_iter_link_info *iter_info; __u32 iter_info_len; __u32 target_btf_id; union { struct { __u64 bpf_cookie; } perf_event; struct { __u32 flags; __u32 cnt; const char **syms; const unsigned long *addrs; const __u64 *cookies; } kprobe_multi; }; size_t :0; };Note that we have
const unsigned long *addrs;If we have 32-bit user space application and 64bit kernel, and we will have userspace 32-bit pointers and kernel as 64bit pointers and current kernel doesn't handle 32-bit user pointer properly.
Consider this may involve libbpf uapi change, maybe we should change "const unsigned long *addrs;" to "const
__u64*addrs;" considering we haven't freeze libbpf UAPI yet.
Quoted 6 lines
Otherwise, we stick to current code with this patch, it will make it difficult to support 32-bit app with 64-bit kernel for
kprobe_multiin the future due to uapi issues.WDYT?
As 32 bit arches are "unsupported" currently, the change would be more a semantic one rather then practical; I don't mind having it here (basically, the tools/* part of [1]), though (assuming it is still possible to get it in 5.18).
Quoted 19 lines
Quoted 18 lines
Fixes 0dcac272540613d4 ("bpf: Add multi kprobe link")Signed-off-by Eugene Syromiatnikov <[email protected]>--- kernel/trace/bpf_trace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)kernel/trace/bpf_trace.c +1
-1@@ -2412,7 +2412,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *print err;/* no support for 32bit archs yet */ -if (sizeof (u64) !=sizeof (void *)) +if (sizeof (u64) !=sizeof (void *) ||in_compat_syscall ())return -EOPNOTSUPP ;if (prog->expected_attach_type !=BPF_TRACE_KPROBE_MULTI )
Re: [PATCH bpf v3 2/2] bpf_trace: bail out from bpf_kprobe_multi_link_attach when in compat
Quoted 50 lines · Eugene Syromiatnikov
Quoted 49 lines · Yonghong Song
Quoted 4 lines · Eugene Syromiatnikov
Since
bpf_kprobe_multi_link_attachdoesn't support 32-bit kernels for whatever reason, having it enabled for compat processes on 64-bit kernels makes even less sense due to discrepances in the type sizes that it does not handle.If I understand correctly, the reason is due to
in libbpf we have
structbpf_link_create_opts{
size_tsz; /* size of this struct for forward/backward compatibility
*/__u32 flags; union bpf_iter_link_info *iter_info; __u32 iter_info_len; __u32 target_btf_id; union { struct { __u64 bpf_cookie; } perf_event; struct { __u32 flags; __u32 cnt; const char **syms; const unsigned long *addrs; const __u64 *cookies; } kprobe_multi; }; size_t :0; };Note that we have
const unsigned long *addrs;If we have 32-bit user space application and 64bit kernel, and we will have userspace 32-bit pointers and kernel as 64bit pointers and current kernel doesn't handle 32-bit user pointer properly.
Consider this may involve libbpf uapi change, maybe we should change "const unsigned long *addrs;" to "const
__u64*addrs;" considering we haven't freeze libbpf UAPI yet.Otherwise, we stick to current code with this patch, it will make it difficult to support 32-bit app with 64-bit kernel for
kprobe_multiin the future due to uapi issues.
Quoted 8 lines
Quoted 1 line
WDYT?
As 32 bit arches are "unsupported" currently, the change would be more a semantic one rather then practical; I don't mind having it here (basically, the
tools/*part of [1]), though (assuming it is still possible to get it in 5.18).
I think for patch [1], we only need libbpf and selftest change, no kernel change is needed since we
explicitly does not support 32bit kernel in the beginning of function bpf_kprobe_multi_link_attach():
/* no support for 32bit archs yet */
if (sizeof(u64) != sizeof(void *))
return -EOPNOTSUPP;and in kernel, address (pointer) size will be considered long (64bit) which is exactly the libbpf change did that.
Quoted 21 lines
Quoted 20 lines
Quoted 19 lines
Fixes 0dcac272540613d4 ("bpf: Add multi kprobe link")Signed-off-by Eugene Syromiatnikov <[email protected]>--- kernel/trace/bpf_trace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)kernel/trace/bpf_trace.c +1
-1@@ -2412,7 +2412,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *print err;/* no support for 32bit archs yet */ -if (sizeof (u64) !=sizeof (void *)) +if (sizeof (u64) !=sizeof (void *) ||in_compat_syscall ())return -EOPNOTSUPP ;if (prog->expected_attach_type !=BPF_TRACE_KPROBE_MULTI )