| Message ID | Yo9VRVMeHbALyjUH@kili (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | dafd0f870eae225816d069c92695852748bee8a3 |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Use safer kvmalloc_array() where possible | expand |
On 5/26/22 3:24 AM, Dan Carpenter wrote: > The kvmalloc_array() function is safer because it has a check for > integer overflows. These sizes come from the user and I was not > able to see any bounds checking so an integer overflow seems like a > realistic concern. > > Fixes: 0dcac2725406 ("bpf: Add multi kprobe link") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > kernel/trace/bpf_trace.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index 10b157a6d73e..7a13e6ac6327 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -2263,11 +2263,11 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32 > int err = -ENOMEM; > unsigned int i; > > - syms = kvmalloc(cnt * sizeof(*syms), GFP_KERNEL); > + syms = kvmalloc_array(cnt, sizeof(*syms), GFP_KERNEL); > if (!syms) > goto error; > > - buf = kvmalloc(cnt * KSYM_NAME_LEN, GFP_KERNEL); > + buf = kvmalloc_array(cnt, KSYM_NAME_LEN, GFP_KERNEL); > if (!buf) > goto error; > > @@ -2464,7 +2464,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr For this part of change, there is a similar pending patch here: https://lore.kernel.org/bpf/399e634781822329e856103cddba975f58f0498c.1652982525.git.esyr@redhat.com/ which waits for further review. That patch tries to detect the overflow explicitly to avoid possible kernel dmesg warnings. (See function kvmalloc_node()). > return -EINVAL; > > size = cnt * sizeof(*addrs); > - addrs = kvmalloc(size, GFP_KERNEL); > + addrs = kvmalloc_array(cnt, sizeof(*addrs), GFP_KERNEL); > if (!addrs) > return -ENOMEM; > > @@ -2489,7 +2489,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr > > ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies); > if (ucookies) { > - cookies = kvmalloc(size, GFP_KERNEL); > + cookies = kvmalloc_array(cnt, sizeof(*addrs), GFP_KERNEL); > if (!cookies) { > err = -ENOMEM; > goto error;
On Thu, May 26, 2022 at 01:24:05PM +0300, Dan Carpenter wrote: > The kvmalloc_array() function is safer because it has a check for > integer overflows. These sizes come from the user and I was not > able to see any bounds checking so an integer overflow seems like a > realistic concern. > > Fixes: 0dcac2725406 ("bpf: Add multi kprobe link") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Eugene was addressing these: https://lore.kernel.org/bpf/399e634781822329e856103cddba975f58f0498c.1652982525.git.esyr@redhat.com/ I think using kvmalloc_array was one of the review comments jirka > --- > kernel/trace/bpf_trace.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index 10b157a6d73e..7a13e6ac6327 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -2263,11 +2263,11 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32 > int err = -ENOMEM; > unsigned int i; > > - syms = kvmalloc(cnt * sizeof(*syms), GFP_KERNEL); > + syms = kvmalloc_array(cnt, sizeof(*syms), GFP_KERNEL); > if (!syms) > goto error; > > - buf = kvmalloc(cnt * KSYM_NAME_LEN, GFP_KERNEL); > + buf = kvmalloc_array(cnt, KSYM_NAME_LEN, GFP_KERNEL); > if (!buf) > goto error; > > @@ -2464,7 +2464,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr > return -EINVAL; > > size = cnt * sizeof(*addrs); > - addrs = kvmalloc(size, GFP_KERNEL); > + addrs = kvmalloc_array(cnt, sizeof(*addrs), GFP_KERNEL); > if (!addrs) > return -ENOMEM; > > @@ -2489,7 +2489,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr > > ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies); > if (ucookies) { > - cookies = kvmalloc(size, GFP_KERNEL); > + cookies = kvmalloc_array(cnt, sizeof(*addrs), GFP_KERNEL); > if (!cookies) { > err = -ENOMEM; > goto error; > -- > 2.35.1 >
On Thu, May 26, 2022 at 08:31:10AM -0700, Yonghong Song wrote: > > > On 5/26/22 3:24 AM, Dan Carpenter wrote: > > The kvmalloc_array() function is safer because it has a check for > > integer overflows. These sizes come from the user and I was not > > able to see any bounds checking so an integer overflow seems like a > > realistic concern. > > > > Fixes: 0dcac2725406 ("bpf: Add multi kprobe link") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > kernel/trace/bpf_trace.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > > index 10b157a6d73e..7a13e6ac6327 100644 > > --- a/kernel/trace/bpf_trace.c > > +++ b/kernel/trace/bpf_trace.c > > @@ -2263,11 +2263,11 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32 > > int err = -ENOMEM; > > unsigned int i; > > - syms = kvmalloc(cnt * sizeof(*syms), GFP_KERNEL); > > + syms = kvmalloc_array(cnt, sizeof(*syms), GFP_KERNEL); > > if (!syms) > > goto error; > > - buf = kvmalloc(cnt * KSYM_NAME_LEN, GFP_KERNEL); > > + buf = kvmalloc_array(cnt, KSYM_NAME_LEN, GFP_KERNEL); > > if (!buf) > > goto error; > > @@ -2464,7 +2464,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr > > For this part of change, there is a similar pending patch here: > https://lore.kernel.org/bpf/399e634781822329e856103cddba975f58f0498c.1652982525.git.esyr@redhat.com/ > which waits for further review. That patch tries to detect the overflow > explicitly to avoid possible kernel dmesg warnings. (See function > kvmalloc_node()). That patch doesn't apply any more. Static checkers will insist that kvmalloc_array() is cleaner and safer than kvmalloc(n * size, and they don't care if the integer overflow is real or not. -EOVERFLOW is the wrong error code. Just return -ENOMEM. Checking for size > INT_MAX is ugly. Use a correct limit based on what the maximum reasonable size is. Or if we only want to prevent the stack dump then just pass __GFP_NOWARN. It annoyed me that size was type unsigned int. Sizes should be unsigned long. Every alloc() function takes an unsigned long so using a u32 temporary value for the size is what made this code so dangerous. If it had been: addrs = kvmalloc(cnt * sizeof(*addrs), GFP_KERNEL); instead of: size = cnt * sizeof(*addrs); addrs = kvmalloc(size, GFP_KERNEL); Then the integer overflow bug would only have affected 32 bit systems and those are pretty rare. Choosing the wrong type took a minor bug and made it affect everyone. regards, dan carpenter
Hello: This patch was applied to bpf/bpf-next.git (master) by Andrii Nakryiko <andrii@kernel.org>: On Thu, 26 May 2022 13:24:05 +0300 you wrote: > The kvmalloc_array() function is safer because it has a check for > integer overflows. These sizes come from the user and I was not > able to see any bounds checking so an integer overflow seems like a > realistic concern. > > Fixes: 0dcac2725406 ("bpf: Add multi kprobe link") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > [...] Here is the summary with links: - bpf: Use safer kvmalloc_array() where possible https://git.kernel.org/bpf/bpf-next/c/dafd0f870eae You are awesome, thank you!
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index 10b157a6d73e..7a13e6ac6327 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -2263,11 +2263,11 @@ static int copy_user_syms(struct user_syms *us, unsigned long __user *usyms, u32 int err = -ENOMEM; unsigned int i; - syms = kvmalloc(cnt * sizeof(*syms), GFP_KERNEL); + syms = kvmalloc_array(cnt, sizeof(*syms), GFP_KERNEL); if (!syms) goto error; - buf = kvmalloc(cnt * KSYM_NAME_LEN, GFP_KERNEL); + buf = kvmalloc_array(cnt, KSYM_NAME_LEN, GFP_KERNEL); if (!buf) goto error; @@ -2464,7 +2464,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr return -EINVAL; size = cnt * sizeof(*addrs); - addrs = kvmalloc(size, GFP_KERNEL); + addrs = kvmalloc_array(cnt, sizeof(*addrs), GFP_KERNEL); if (!addrs) return -ENOMEM; @@ -2489,7 +2489,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies); if (ucookies) { - cookies = kvmalloc(size, GFP_KERNEL); + cookies = kvmalloc_array(cnt, sizeof(*addrs), GFP_KERNEL); if (!cookies) { err = -ENOMEM; goto error;
The kvmalloc_array() function is safer because it has a check for integer overflows. These sizes come from the user and I was not able to see any bounds checking so an integer overflow seems like a realistic concern. Fixes: 0dcac2725406 ("bpf: Add multi kprobe link") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- kernel/trace/bpf_trace.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)