On Mon, Nov 28, 2022 at 1:07 AM Jiri Olsa <olsajiri@xxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
> >
