When user specifies symbols and cookies for kprobe_multi link
interface it's very likely the cookies will be misplaced and
returned to wrong functions (via get_attach_cookie helper).
The reason is that to resolve the provided functions we sort
them before passing them to ftrace_lookup_symbols, but we do
not do the same sort on the cookie values.
Fixing this by using sort_r function with custom swap callback
that swaps cookie values as well.
Fixes: 0236fec57a15 ("bpf: Resolve symbols with ftrace_lookup_symbols for kprobe multi link")
Signed-off-by: Jiri Olsa <[email protected]>
---
kernel/trace/bpf_trace.c | 65 ++++++++++++++++++++++++++++++----------
1 file changed, 50 insertions(+), 15 deletions(-)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 10b157a6d73e..e5c423b835ab 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2423,7 +2423,12 @@ kprobe_multi_link_handler(struct fprobe *fp, unsigned long entry_ip,
kprobe_multi_link_prog_run(link, entry_ip, regs);
}
-static int symbols_cmp(const void *a, const void *b)
+struct multi_symbols_sort {
+ const char **funcs;
+ u64 *cookies;
+};
+
+static int symbols_cmp_r(const void *a, const void *b, const void *priv)
{
const char **str_a = (const char **) a;
const char **str_b = (const char **) b;
@@ -2431,6 +2436,25 @@ static int symbols_cmp(const void *a, const void *b)
return strcmp(*str_a, *str_b);
}
+static void symbols_swap_r(void *a, void *b, int size, const void *priv)
+{
+ const struct multi_symbols_sort *data = priv;
+ const char **name_a = a, **name_b = b;
+ u64 *cookie_a, *cookie_b;
+
+ cookie_a = data->cookies + (name_a - data->funcs);
+ cookie_b = data->cookies + (name_b - data->funcs);
+
+ /* swap name_a/name_b and cookie_a/cookie_b values */
+ swap(*name_a, *name_b);
+ swap(*cookie_a, *cookie_b);
+}
+
+static int symbols_cmp(const void *a, const void *b)
+{
+ return symbols_cmp_r(a, b, NULL);
+}
+
int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
{
struct bpf_kprobe_multi_link *link = NULL;
@@ -2468,6 +2492,19 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
if (!addrs)
return -ENOMEM;
+ ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
+ if (ucookies) {
+ cookies = kvmalloc(size, GFP_KERNEL);
+ if (!cookies) {
+ err = -ENOMEM;
+ goto error;
+ }
+ if (copy_from_user(cookies, ucookies, size)) {
+ err = -EFAULT;
+ goto error;
+ }
+ }
+
if (uaddrs) {
if (copy_from_user(addrs, uaddrs, size)) {
err = -EFAULT;
@@ -2480,26 +2517,24 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
if (err)
goto error;
- sort(us.syms, cnt, sizeof(*us.syms), symbols_cmp, NULL);
+ if (cookies) {
+ struct multi_symbols_sort data = {
+ .cookies = cookies,
+ .funcs = us.syms,
+ };
+
+ sort_r(us.syms, cnt, sizeof(*us.syms), symbols_cmp_r,
+ symbols_swap_r, &data);
+ } else {
+ sort(us.syms, cnt, sizeof(*us.syms), symbols_cmp, NULL);
+ }
+
err = ftrace_lookup_symbols(us.syms, cnt, addrs);
free_user_syms(&us);
if (err)
goto error;
}
- ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
- if (ucookies) {
- cookies = kvmalloc(size, GFP_KERNEL);
- if (!cookies) {
- err = -ENOMEM;
- goto error;
- }
- if (copy_from_user(cookies, ucookies, size)) {
- err = -EFAULT;
- goto error;
- }
- }
-
link = kzalloc(sizeof(*link), GFP_KERNEL);
if (!link) {
err = -ENOMEM;
--
2.35.3
> On May 27, 2022, at 1:56 PM, Jiri Olsa <[email protected]> wrote:
>
> When user specifies symbols and cookies for kprobe_multi link
> interface it's very likely the cookies will be misplaced and
> returned to wrong functions (via get_attach_cookie helper).
>
> The reason is that to resolve the provided functions we sort
> them before passing them to ftrace_lookup_symbols, but we do
> not do the same sort on the cookie values.
>
> Fixing this by using sort_r function with custom swap callback
> that swaps cookie values as well.
>
> Fixes: 0236fec57a15 ("bpf: Resolve symbols with ftrace_lookup_symbols for kprobe multi link")
> Signed-off-by: Jiri Olsa <[email protected]
Acked-by: Song Liu <[email protected]>
On Thu, Jun 02, 2022 at 04:02:28PM -0700, Andrii Nakryiko wrote:
> On Thu, Jun 2, 2022 at 4:01 PM Andrii Nakryiko
> <[email protected]> wrote:
> >
> > On Fri, May 27, 2022 at 1:57 PM Jiri Olsa <[email protected]> wrote:
> > >
> > > When user specifies symbols and cookies for kprobe_multi link
> > > interface it's very likely the cookies will be misplaced and
> > > returned to wrong functions (via get_attach_cookie helper).
> > >
> > > The reason is that to resolve the provided functions we sort
> > > them before passing them to ftrace_lookup_symbols, but we do
> > > not do the same sort on the cookie values.
> > >
> > > Fixing this by using sort_r function with custom swap callback
> > > that swaps cookie values as well.
> > >
> > > Fixes: 0236fec57a15 ("bpf: Resolve symbols with ftrace_lookup_symbols for kprobe multi link")
> > > Signed-off-by: Jiri Olsa <[email protected]>
> > > ---
> > > kernel/trace/bpf_trace.c | 65 ++++++++++++++++++++++++++++++----------
> > > 1 file changed, 50 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > index 10b157a6d73e..e5c423b835ab 100644
> > > --- a/kernel/trace/bpf_trace.c
> > > +++ b/kernel/trace/bpf_trace.c
> > > @@ -2423,7 +2423,12 @@ kprobe_multi_link_handler(struct fprobe *fp, unsigned long entry_ip,
> > > kprobe_multi_link_prog_run(link, entry_ip, regs);
> > > }
> > >
> > > -static int symbols_cmp(const void *a, const void *b)
> > > +struct multi_symbols_sort {
> > > + const char **funcs;
> > > + u64 *cookies;
> > > +};
> > > +
> > > +static int symbols_cmp_r(const void *a, const void *b, const void *priv)
> > > {
> > > const char **str_a = (const char **) a;
> > > const char **str_b = (const char **) b;
> > > @@ -2431,6 +2436,25 @@ static int symbols_cmp(const void *a, const void *b)
> > > return strcmp(*str_a, *str_b);
> > > }
> > >
> > > +static void symbols_swap_r(void *a, void *b, int size, const void *priv)
> > > +{
> > > + const struct multi_symbols_sort *data = priv;
> > > + const char **name_a = a, **name_b = b;
> > > + u64 *cookie_a, *cookie_b;
> > > +
> > > + cookie_a = data->cookies + (name_a - data->funcs);
> > > + cookie_b = data->cookies + (name_b - data->funcs);
> > > +
> > > + /* swap name_a/name_b and cookie_a/cookie_b values */
> > > + swap(*name_a, *name_b);
> > > + swap(*cookie_a, *cookie_b);
> > > +}
> > > +
> > > +static int symbols_cmp(const void *a, const void *b)
> > > +{
> > > + return symbols_cmp_r(a, b, NULL);
> > > +}
> > > +
> > > int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> > > {
> > > struct bpf_kprobe_multi_link *link = NULL;
> > > @@ -2468,6 +2492,19 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> > > if (!addrs)
> > > return -ENOMEM;
> > >
> > > + ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
> > > + if (ucookies) {
> > > + cookies = kvmalloc(size, GFP_KERNEL);
>
> oh, and you'll have to rebase anyways after kvmalloc_array patch
true, that kvmalloc_array change went to bpf-next/master,
but as Song mentioned this patchset should probably go for bpf/master?
I'm fine either way, let me know ;-)
thanks,
jirka
On Thu, Jun 2, 2022 at 4:01 PM Andrii Nakryiko
<[email protected]> wrote:
>
> On Fri, May 27, 2022 at 1:57 PM Jiri Olsa <[email protected]> wrote:
> >
> > When user specifies symbols and cookies for kprobe_multi link
> > interface it's very likely the cookies will be misplaced and
> > returned to wrong functions (via get_attach_cookie helper).
> >
> > The reason is that to resolve the provided functions we sort
> > them before passing them to ftrace_lookup_symbols, but we do
> > not do the same sort on the cookie values.
> >
> > Fixing this by using sort_r function with custom swap callback
> > that swaps cookie values as well.
> >
> > Fixes: 0236fec57a15 ("bpf: Resolve symbols with ftrace_lookup_symbols for kprobe multi link")
> > Signed-off-by: Jiri Olsa <[email protected]>
> > ---
> > kernel/trace/bpf_trace.c | 65 ++++++++++++++++++++++++++++++----------
> > 1 file changed, 50 insertions(+), 15 deletions(-)
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 10b157a6d73e..e5c423b835ab 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -2423,7 +2423,12 @@ kprobe_multi_link_handler(struct fprobe *fp, unsigned long entry_ip,
> > kprobe_multi_link_prog_run(link, entry_ip, regs);
> > }
> >
> > -static int symbols_cmp(const void *a, const void *b)
> > +struct multi_symbols_sort {
> > + const char **funcs;
> > + u64 *cookies;
> > +};
> > +
> > +static int symbols_cmp_r(const void *a, const void *b, const void *priv)
> > {
> > const char **str_a = (const char **) a;
> > const char **str_b = (const char **) b;
> > @@ -2431,6 +2436,25 @@ static int symbols_cmp(const void *a, const void *b)
> > return strcmp(*str_a, *str_b);
> > }
> >
> > +static void symbols_swap_r(void *a, void *b, int size, const void *priv)
> > +{
> > + const struct multi_symbols_sort *data = priv;
> > + const char **name_a = a, **name_b = b;
> > + u64 *cookie_a, *cookie_b;
> > +
> > + cookie_a = data->cookies + (name_a - data->funcs);
> > + cookie_b = data->cookies + (name_b - data->funcs);
> > +
> > + /* swap name_a/name_b and cookie_a/cookie_b values */
> > + swap(*name_a, *name_b);
> > + swap(*cookie_a, *cookie_b);
> > +}
> > +
> > +static int symbols_cmp(const void *a, const void *b)
> > +{
> > + return symbols_cmp_r(a, b, NULL);
> > +}
> > +
> > int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> > {
> > struct bpf_kprobe_multi_link *link = NULL;
> > @@ -2468,6 +2492,19 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> > if (!addrs)
> > return -ENOMEM;
> >
> > + ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
> > + if (ucookies) {
> > + cookies = kvmalloc(size, GFP_KERNEL);
oh, and you'll have to rebase anyways after kvmalloc_array patch
> > + if (!cookies) {
> > + err = -ENOMEM;
> > + goto error;
> > + }
> > + if (copy_from_user(cookies, ucookies, size)) {
> > + err = -EFAULT;
> > + goto error;
> > + }
> > + }
> > +
> > if (uaddrs) {
> > if (copy_from_user(addrs, uaddrs, size)) {
> > err = -EFAULT;
> > @@ -2480,26 +2517,24 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> > if (err)
> > goto error;
> >
> > - sort(us.syms, cnt, sizeof(*us.syms), symbols_cmp, NULL);
> > + if (cookies) {
> > + struct multi_symbols_sort data = {
> > + .cookies = cookies,
> > + .funcs = us.syms,
> > + };
> > +
> > + sort_r(us.syms, cnt, sizeof(*us.syms), symbols_cmp_r,
> > + symbols_swap_r, &data);
> > + } else {
> > + sort(us.syms, cnt, sizeof(*us.syms), symbols_cmp, NULL);
> > + }
>
> maybe just always do sort_r, swap callback can just check if cookie
> array is NULL and if not, additionally swap cookies? why have all
> these different callbacks and complicate the code unnecessarily?
>
> > +
> > err = ftrace_lookup_symbols(us.syms, cnt, addrs);
> > free_user_syms(&us);
> > if (err)
> > goto error;
> > }
> >
> > - ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
> > - if (ucookies) {
> > - cookies = kvmalloc(size, GFP_KERNEL);
> > - if (!cookies) {
> > - err = -ENOMEM;
> > - goto error;
> > - }
> > - if (copy_from_user(cookies, ucookies, size)) {
> > - err = -EFAULT;
> > - goto error;
> > - }
> > - }
> > -
> > link = kzalloc(sizeof(*link), GFP_KERNEL);
> > if (!link) {
> > err = -ENOMEM;
> > --
> > 2.35.3
> >
On Fri, May 27, 2022 at 1:57 PM Jiri Olsa <[email protected]> wrote:
>
> When user specifies symbols and cookies for kprobe_multi link
> interface it's very likely the cookies will be misplaced and
> returned to wrong functions (via get_attach_cookie helper).
>
> The reason is that to resolve the provided functions we sort
> them before passing them to ftrace_lookup_symbols, but we do
> not do the same sort on the cookie values.
>
> Fixing this by using sort_r function with custom swap callback
> that swaps cookie values as well.
>
> Fixes: 0236fec57a15 ("bpf: Resolve symbols with ftrace_lookup_symbols for kprobe multi link")
> Signed-off-by: Jiri Olsa <[email protected]>
> ---
> kernel/trace/bpf_trace.c | 65 ++++++++++++++++++++++++++++++----------
> 1 file changed, 50 insertions(+), 15 deletions(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 10b157a6d73e..e5c423b835ab 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2423,7 +2423,12 @@ kprobe_multi_link_handler(struct fprobe *fp, unsigned long entry_ip,
> kprobe_multi_link_prog_run(link, entry_ip, regs);
> }
>
> -static int symbols_cmp(const void *a, const void *b)
> +struct multi_symbols_sort {
> + const char **funcs;
> + u64 *cookies;
> +};
> +
> +static int symbols_cmp_r(const void *a, const void *b, const void *priv)
> {
> const char **str_a = (const char **) a;
> const char **str_b = (const char **) b;
> @@ -2431,6 +2436,25 @@ static int symbols_cmp(const void *a, const void *b)
> return strcmp(*str_a, *str_b);
> }
>
> +static void symbols_swap_r(void *a, void *b, int size, const void *priv)
> +{
> + const struct multi_symbols_sort *data = priv;
> + const char **name_a = a, **name_b = b;
> + u64 *cookie_a, *cookie_b;
> +
> + cookie_a = data->cookies + (name_a - data->funcs);
> + cookie_b = data->cookies + (name_b - data->funcs);
> +
> + /* swap name_a/name_b and cookie_a/cookie_b values */
> + swap(*name_a, *name_b);
> + swap(*cookie_a, *cookie_b);
> +}
> +
> +static int symbols_cmp(const void *a, const void *b)
> +{
> + return symbols_cmp_r(a, b, NULL);
> +}
> +
> int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> {
> struct bpf_kprobe_multi_link *link = NULL;
> @@ -2468,6 +2492,19 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> if (!addrs)
> return -ENOMEM;
>
> + ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
> + if (ucookies) {
> + cookies = kvmalloc(size, GFP_KERNEL);
> + if (!cookies) {
> + err = -ENOMEM;
> + goto error;
> + }
> + if (copy_from_user(cookies, ucookies, size)) {
> + err = -EFAULT;
> + goto error;
> + }
> + }
> +
> if (uaddrs) {
> if (copy_from_user(addrs, uaddrs, size)) {
> err = -EFAULT;
> @@ -2480,26 +2517,24 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> if (err)
> goto error;
>
> - sort(us.syms, cnt, sizeof(*us.syms), symbols_cmp, NULL);
> + if (cookies) {
> + struct multi_symbols_sort data = {
> + .cookies = cookies,
> + .funcs = us.syms,
> + };
> +
> + sort_r(us.syms, cnt, sizeof(*us.syms), symbols_cmp_r,
> + symbols_swap_r, &data);
> + } else {
> + sort(us.syms, cnt, sizeof(*us.syms), symbols_cmp, NULL);
> + }
maybe just always do sort_r, swap callback can just check if cookie
array is NULL and if not, additionally swap cookies? why have all
these different callbacks and complicate the code unnecessarily?
> +
> err = ftrace_lookup_symbols(us.syms, cnt, addrs);
> free_user_syms(&us);
> if (err)
> goto error;
> }
>
> - ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
> - if (ucookies) {
> - cookies = kvmalloc(size, GFP_KERNEL);
> - if (!cookies) {
> - err = -ENOMEM;
> - goto error;
> - }
> - if (copy_from_user(cookies, ucookies, size)) {
> - err = -EFAULT;
> - goto error;
> - }
> - }
> -
> link = kzalloc(sizeof(*link), GFP_KERNEL);
> if (!link) {
> err = -ENOMEM;
> --
> 2.35.3
>
On Thu, Jun 02, 2022 at 04:01:07PM -0700, Andrii Nakryiko wrote:
> On Fri, May 27, 2022 at 1:57 PM Jiri Olsa <[email protected]> wrote:
> >
> > When user specifies symbols and cookies for kprobe_multi link
> > interface it's very likely the cookies will be misplaced and
> > returned to wrong functions (via get_attach_cookie helper).
> >
> > The reason is that to resolve the provided functions we sort
> > them before passing them to ftrace_lookup_symbols, but we do
> > not do the same sort on the cookie values.
> >
> > Fixing this by using sort_r function with custom swap callback
> > that swaps cookie values as well.
> >
> > Fixes: 0236fec57a15 ("bpf: Resolve symbols with ftrace_lookup_symbols for kprobe multi link")
> > Signed-off-by: Jiri Olsa <[email protected]>
> > ---
> > kernel/trace/bpf_trace.c | 65 ++++++++++++++++++++++++++++++----------
> > 1 file changed, 50 insertions(+), 15 deletions(-)
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 10b157a6d73e..e5c423b835ab 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -2423,7 +2423,12 @@ kprobe_multi_link_handler(struct fprobe *fp, unsigned long entry_ip,
> > kprobe_multi_link_prog_run(link, entry_ip, regs);
> > }
> >
> > -static int symbols_cmp(const void *a, const void *b)
> > +struct multi_symbols_sort {
> > + const char **funcs;
> > + u64 *cookies;
> > +};
> > +
> > +static int symbols_cmp_r(const void *a, const void *b, const void *priv)
> > {
> > const char **str_a = (const char **) a;
> > const char **str_b = (const char **) b;
> > @@ -2431,6 +2436,25 @@ static int symbols_cmp(const void *a, const void *b)
> > return strcmp(*str_a, *str_b);
> > }
> >
> > +static void symbols_swap_r(void *a, void *b, int size, const void *priv)
> > +{
> > + const struct multi_symbols_sort *data = priv;
> > + const char **name_a = a, **name_b = b;
> > + u64 *cookie_a, *cookie_b;
> > +
> > + cookie_a = data->cookies + (name_a - data->funcs);
> > + cookie_b = data->cookies + (name_b - data->funcs);
> > +
> > + /* swap name_a/name_b and cookie_a/cookie_b values */
> > + swap(*name_a, *name_b);
> > + swap(*cookie_a, *cookie_b);
> > +}
> > +
> > +static int symbols_cmp(const void *a, const void *b)
> > +{
> > + return symbols_cmp_r(a, b, NULL);
> > +}
> > +
> > int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> > {
> > struct bpf_kprobe_multi_link *link = NULL;
> > @@ -2468,6 +2492,19 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> > if (!addrs)
> > return -ENOMEM;
> >
> > + ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
> > + if (ucookies) {
> > + cookies = kvmalloc(size, GFP_KERNEL);
> > + if (!cookies) {
> > + err = -ENOMEM;
> > + goto error;
> > + }
> > + if (copy_from_user(cookies, ucookies, size)) {
> > + err = -EFAULT;
> > + goto error;
> > + }
> > + }
> > +
> > if (uaddrs) {
> > if (copy_from_user(addrs, uaddrs, size)) {
> > err = -EFAULT;
> > @@ -2480,26 +2517,24 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> > if (err)
> > goto error;
> >
> > - sort(us.syms, cnt, sizeof(*us.syms), symbols_cmp, NULL);
> > + if (cookies) {
> > + struct multi_symbols_sort data = {
> > + .cookies = cookies,
> > + .funcs = us.syms,
> > + };
> > +
> > + sort_r(us.syms, cnt, sizeof(*us.syms), symbols_cmp_r,
> > + symbols_swap_r, &data);
> > + } else {
> > + sort(us.syms, cnt, sizeof(*us.syms), symbols_cmp, NULL);
> > + }
>
> maybe just always do sort_r, swap callback can just check if cookie
> array is NULL and if not, additionally swap cookies? why have all
> these different callbacks and complicate the code unnecessarily?
right, good idea.. will change
thanks,
jirka
>
> > +
> > err = ftrace_lookup_symbols(us.syms, cnt, addrs);
> > free_user_syms(&us);
> > if (err)
> > goto error;
> > }
> >
> > - ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
> > - if (ucookies) {
> > - cookies = kvmalloc(size, GFP_KERNEL);
> > - if (!cookies) {
> > - err = -ENOMEM;
> > - goto error;
> > - }
> > - if (copy_from_user(cookies, ucookies, size)) {
> > - err = -EFAULT;
> > - goto error;
> > - }
> > - }
> > -
> > link = kzalloc(sizeof(*link), GFP_KERNEL);
> > if (!link) {
> > err = -ENOMEM;
> > --
> > 2.35.3
> >
On Fri, Jun 3, 2022 at 3:23 AM Jiri Olsa <[email protected]> wrote:
>
> On Thu, Jun 02, 2022 at 04:02:28PM -0700, Andrii Nakryiko wrote:
> > On Thu, Jun 2, 2022 at 4:01 PM Andrii Nakryiko
> > <[email protected]> wrote:
> > >
> > > On Fri, May 27, 2022 at 1:57 PM Jiri Olsa <[email protected]> wrote:
> > > >
> > > > When user specifies symbols and cookies for kprobe_multi link
> > > > interface it's very likely the cookies will be misplaced and
> > > > returned to wrong functions (via get_attach_cookie helper).
> > > >
> > > > The reason is that to resolve the provided functions we sort
> > > > them before passing them to ftrace_lookup_symbols, but we do
> > > > not do the same sort on the cookie values.
> > > >
> > > > Fixing this by using sort_r function with custom swap callback
> > > > that swaps cookie values as well.
> > > >
> > > > Fixes: 0236fec57a15 ("bpf: Resolve symbols with ftrace_lookup_symbols for kprobe multi link")
> > > > Signed-off-by: Jiri Olsa <[email protected]>
> > > > ---
> > > > kernel/trace/bpf_trace.c | 65 ++++++++++++++++++++++++++++++----------
> > > > 1 file changed, 50 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > > index 10b157a6d73e..e5c423b835ab 100644
> > > > --- a/kernel/trace/bpf_trace.c
> > > > +++ b/kernel/trace/bpf_trace.c
> > > > @@ -2423,7 +2423,12 @@ kprobe_multi_link_handler(struct fprobe *fp, unsigned long entry_ip,
> > > > kprobe_multi_link_prog_run(link, entry_ip, regs);
> > > > }
> > > >
> > > > -static int symbols_cmp(const void *a, const void *b)
> > > > +struct multi_symbols_sort {
> > > > + const char **funcs;
> > > > + u64 *cookies;
> > > > +};
> > > > +
> > > > +static int symbols_cmp_r(const void *a, const void *b, const void *priv)
> > > > {
> > > > const char **str_a = (const char **) a;
> > > > const char **str_b = (const char **) b;
> > > > @@ -2431,6 +2436,25 @@ static int symbols_cmp(const void *a, const void *b)
> > > > return strcmp(*str_a, *str_b);
> > > > }
> > > >
> > > > +static void symbols_swap_r(void *a, void *b, int size, const void *priv)
> > > > +{
> > > > + const struct multi_symbols_sort *data = priv;
> > > > + const char **name_a = a, **name_b = b;
> > > > + u64 *cookie_a, *cookie_b;
> > > > +
> > > > + cookie_a = data->cookies + (name_a - data->funcs);
> > > > + cookie_b = data->cookies + (name_b - data->funcs);
> > > > +
> > > > + /* swap name_a/name_b and cookie_a/cookie_b values */
> > > > + swap(*name_a, *name_b);
> > > > + swap(*cookie_a, *cookie_b);
> > > > +}
> > > > +
> > > > +static int symbols_cmp(const void *a, const void *b)
> > > > +{
> > > > + return symbols_cmp_r(a, b, NULL);
> > > > +}
> > > > +
> > > > int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> > > > {
> > > > struct bpf_kprobe_multi_link *link = NULL;
> > > > @@ -2468,6 +2492,19 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> > > > if (!addrs)
> > > > return -ENOMEM;
> > > >
> > > > + ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
> > > > + if (ucookies) {
> > > > + cookies = kvmalloc(size, GFP_KERNEL);
> >
> > oh, and you'll have to rebase anyways after kvmalloc_array patch
>
> true, that kvmalloc_array change went to bpf-next/master,
> but as Song mentioned this patchset should probably go for bpf/master?
>
> I'm fine either way, let me know ;-)
>
I've moved kvmalloc_array() fix to bpf tree (it is an actual fix
against potential overflow after all), so please base everything on
bpf tree.
> thanks,
> jirka