| Message ID | 2ec861621e283ba2a54f7e939eafed1c29f77bf4.1669216157.git.vmalik@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Fix attaching fentry/fexit/fmod_ret/lsm to modules | expand |
On Mon, Nov 28, 2022 at 08:26:29AM +0100, Viktor Malik wrote: > When attaching fentry/fexit/fmod_ret/lsm to a function located in a > module without specifying the target program, the verifier tries to find > the address to attach to in kallsyms. This is always done by searching > the entire kallsyms, not respecting the module in which the function is > located. > > This approach causes an incorrect attachment address to be computed if > the function to attach to is shadowed by a function of the same name > located earlier in kallsyms. > > Since the attachment must contain the BTF of the program to attach to, > we may extract the module name from it (if the attach target is a > module) and search for the function address in the correct module. > > Signed-off-by: Viktor Malik <vmalik@redhat.com> > --- > include/linux/btf.h | 1 + > kernel/bpf/btf.c | 5 +++++ > kernel/bpf/verifier.c | 9 ++++++++- > 3 files changed, 14 insertions(+), 1 deletion(-) > > diff --git a/include/linux/btf.h b/include/linux/btf.h > index 9ed00077db6e..bdbf3eb7083d 100644 > --- a/include/linux/btf.h > +++ b/include/linux/btf.h > @@ -187,6 +187,7 @@ u32 btf_obj_id(const struct btf *btf); > bool btf_is_kernel(const struct btf *btf); > bool btf_is_module(const struct btf *btf); > struct module *btf_try_get_module(const struct btf *btf); > +const char *btf_module_name(const struct btf *btf); > u32 btf_nr_types(const struct btf *btf); > bool btf_member_is_reg_int(const struct btf *btf, const struct btf_type *s, > const struct btf_member *m, > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index 1a59cc7ad730..211fcbb7649d 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -7192,6 +7192,11 @@ bool btf_is_module(const struct btf *btf) > return btf->kernel_btf && strcmp(btf->name, "vmlinux") != 0; > } > > +const char *btf_module_name(const struct btf *btf) > +{ > + return btf->name; > +} > + > enum { > BTF_MODULE_F_LIVE = (1 << 0), > }; > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 9528a066cfa5..acbe62a73559 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -16343,7 +16343,14 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, > else > addr = (long) tgt_prog->aux->func[subprog]->bpf_func; > } else { > - addr = kallsyms_lookup_name(tname); > + if (btf_is_module(btf)) { > + char tmodname[MODULE_NAME_LEN + KSYM_NAME_LEN + 1]; looks good.. would be nice to have module_kallsyms lookup function that takes module name and symbol separately so we won't waste stack on that.. especially when module_kallsyms_lookup_name just separates it back again and does module lookup.. but not sure how much pain it'd be jirka > + snprintf(tmodname, sizeof(tmodname), "%s:%s", > + btf_module_name(btf), tname); > + addr = module_kallsyms_lookup_name(tmodname); > + } > + else > + addr = kallsyms_lookup_name(tname); > if (!addr) { > bpf_log(log, > "The address of function %s cannot be found\n", > -- > 2.38.1 >
On Mon, Nov 28, 2022 at 1:07 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > On Mon, Nov 28, 2022 at 08:26:29AM +0100, Viktor Malik wrote: > > When attaching fentry/fexit/fmod_ret/lsm to a function located in a > > module without specifying the target program, the verifier tries to find > > the address to attach to in kallsyms. This is always done by searching > > the entire kallsyms, not respecting the module in which the function is > > located. > > > > This approach causes an incorrect attachment address to be computed if > > the function to attach to is shadowed by a function of the same name > > located earlier in kallsyms. > > > > Since the attachment must contain the BTF of the program to attach to, > > we may extract the module name from it (if the attach target is a > > module) and search for the function address in the correct module. > > > > Signed-off-by: Viktor Malik <vmalik@redhat.com> > > --- > > include/linux/btf.h | 1 + > > kernel/bpf/btf.c | 5 +++++ > > kernel/bpf/verifier.c | 9 ++++++++- > > 3 files changed, 14 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/btf.h b/include/linux/btf.h > > index 9ed00077db6e..bdbf3eb7083d 100644 > > --- a/include/linux/btf.h > > +++ b/include/linux/btf.h > > @@ -187,6 +187,7 @@ u32 btf_obj_id(const struct btf *btf); > > bool btf_is_kernel(const struct btf *btf); > > bool btf_is_module(const struct btf *btf); > > struct module *btf_try_get_module(const struct btf *btf); > > +const char *btf_module_name(const struct btf *btf); > > u32 btf_nr_types(const struct btf *btf); > > bool btf_member_is_reg_int(const struct btf *btf, const struct btf_type *s, > > const struct btf_member *m, > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > > index 1a59cc7ad730..211fcbb7649d 100644 > > --- a/kernel/bpf/btf.c > > +++ b/kernel/bpf/btf.c > > @@ -7192,6 +7192,11 @@ bool btf_is_module(const struct btf *btf) > > return btf->kernel_btf && strcmp(btf->name, "vmlinux") != 0; > > } > > > > +const char *btf_module_name(const struct btf *btf) > > +{ > > + return btf->name; > > +} > > + > > enum { > > BTF_MODULE_F_LIVE = (1 << 0), > > }; > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 9528a066cfa5..acbe62a73559 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -16343,7 +16343,14 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, > > else > > addr = (long) tgt_prog->aux->func[subprog]->bpf_func; > > } else { > > - addr = kallsyms_lookup_name(tname); > > + if (btf_is_module(btf)) { > > + char tmodname[MODULE_NAME_LEN + KSYM_NAME_LEN + 1]; > > looks good.. would be nice to have module_kallsyms lookup function that > takes module name and symbol separately so we won't waste stack on that.. > > especially when module_kallsyms_lookup_name just separates it back again > and does module lookup.. but not sure how much pain it'd be > > jirka > > > + snprintf(tmodname, sizeof(tmodname), "%s:%s", > > + btf_module_name(btf), tname); > > + addr = module_kallsyms_lookup_name(tmodname); > > + } > > + else > > + addr = kallsyms_lookup_name(tname); In addition to what Jiri suggested, we should also have brackets in the 'else' branch. if (...) { ... } else { ... } > > if (!addr) { > > bpf_log(log, > > "The address of function %s cannot be found\n", > > -- > > 2.38.1 > >
diff --git a/include/linux/btf.h b/include/linux/btf.h index 9ed00077db6e..bdbf3eb7083d 100644 --- a/include/linux/btf.h +++ b/include/linux/btf.h @@ -187,6 +187,7 @@ u32 btf_obj_id(const struct btf *btf); bool btf_is_kernel(const struct btf *btf); bool btf_is_module(const struct btf *btf); struct module *btf_try_get_module(const struct btf *btf); +const char *btf_module_name(const struct btf *btf); u32 btf_nr_types(const struct btf *btf); bool btf_member_is_reg_int(const struct btf *btf, const struct btf_type *s, const struct btf_member *m, diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 1a59cc7ad730..211fcbb7649d 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -7192,6 +7192,11 @@ bool btf_is_module(const struct btf *btf) return btf->kernel_btf && strcmp(btf->name, "vmlinux") != 0; } +const char *btf_module_name(const struct btf *btf) +{ + return btf->name; +} + enum { BTF_MODULE_F_LIVE = (1 << 0), }; diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 9528a066cfa5..acbe62a73559 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -16343,7 +16343,14 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, else addr = (long) tgt_prog->aux->func[subprog]->bpf_func; } else { - addr = kallsyms_lookup_name(tname); + if (btf_is_module(btf)) { + char tmodname[MODULE_NAME_LEN + KSYM_NAME_LEN + 1]; + snprintf(tmodname, sizeof(tmodname), "%s:%s", + btf_module_name(btf), tname); + addr = module_kallsyms_lookup_name(tmodname); + } + else + addr = kallsyms_lookup_name(tname); if (!addr) { bpf_log(log, "The address of function %s cannot be found\n",
When attaching fentry/fexit/fmod_ret/lsm to a function located in a module without specifying the target program, the verifier tries to find the address to attach to in kallsyms. This is always done by searching the entire kallsyms, not respecting the module in which the function is located. This approach causes an incorrect attachment address to be computed if the function to attach to is shadowed by a function of the same name located earlier in kallsyms. Since the attachment must contain the BTF of the program to attach to, we may extract the module name from it (if the attach target is a module) and search for the function address in the correct module. Signed-off-by: Viktor Malik <vmalik@redhat.com> --- include/linux/btf.h | 1 + kernel/bpf/btf.c | 5 +++++ kernel/bpf/verifier.c | 9 ++++++++- 3 files changed, 14 insertions(+), 1 deletion(-)