On Thu, Feb 16, 2023 at 11:32:41AM +0100, Viktor Malik wrote:
> This resolves two problems with attachment of fentry/fexit/fmod_ret/lsm
> to functions located in modules:
>
> 1. 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. Such 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.
>
> 2. If the address to attach to is located in a module, the module
> reference is only acquired in register_fentry. If the module is
> unloaded between the place where the address is found
> (bpf_check_attach_target in the verifier) and register_fentry, it is
> possible that another module is loaded to the same address which may
> lead to potential errors.
>
> Since the attachment must contain the BTF of the program to attach to,
> we extract the module from it and search for the function address in the
> correct module (resolving problem no. 1). Then, the module reference is
> taken directly in bpf_check_attach_target and stored in the bpf program
> (in bpf_prog_aux). The reference is only released when the program is
> unloaded (resolving problem no. 2).
>
> Signed-off-by: Viktor Malik <vmalik@xxxxxxxxxx>
> ---
> include/linux/bpf.h | 1 +
> kernel/bpf/syscall.c | 2 ++
> kernel/bpf/trampoline.c | 27 ---------------------------
> kernel/bpf/verifier.c | 20 +++++++++++++++++++-
> kernel/module/internal.h | 5 +++++
> 5 files changed, 27 insertions(+), 28 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 4385418118f6..2aadc78fe3c1 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1330,6 +1330,7 @@ struct bpf_prog_aux {
> * main prog always has linfo_idx == 0
> */
> u32 linfo_idx;
> + struct module *mod;
> u32 num_exentries;
> struct exception_table_entry *extable;
> union {
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index cda8d00f3762..5b8227e08182 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -2064,6 +2064,8 @@ static void bpf_prog_put_deferred(struct work_struct *work)
> prog = aux->prog;
> perf_event_bpf_event(prog, PERF_BPF_EVENT_PROG_UNLOAD, 0);
> bpf_audit_prog(prog, BPF_AUDIT_UNLOAD);
> + if (aux->mod)
> + module_put(aux->mod);
you can call just module_put, there's != NULL check inside
also should we call it from __bpf_prog_put_noref instead
to cover bpf_prog_load error path?
> bpf_prog_free_id(prog);
> __bpf_prog_put_noref(prog, true);
> }
> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index d0ed7d6f5eec..ebb20bf252c7 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -172,26 +172,6 @@ static struct bpf_trampoline *bpf_trampoline_lookup(u64 key)
> return tr;
> }
>
> -static int bpf_trampoline_module_get(struct bpf_trampoline *tr)
> -{
> - struct module *mod;
> - int err = 0;
> -
> - preempt_disable();
> - mod = __module_text_address((unsigned long) tr->func.addr);
> - if (mod && !try_module_get(mod))
> - err = -ENOENT;
> - preempt_enable();
> - tr->mod = mod;
> - return err;
> -}
> -
> -static void bpf_trampoline_module_put(struct bpf_trampoline *tr)
> -{
> - module_put(tr->mod);
> - tr->mod = NULL;
> -}
> -
> static int unregister_fentry(struct bpf_trampoline *tr, void *old_addr)
> {
> void *ip = tr->func.addr;
> @@ -202,8 +182,6 @@ static int unregister_fentry(struct bpf_trampoline *tr, void *old_addr)
> else
> ret = bpf_arch_text_poke(ip, BPF_MOD_CALL, old_addr, NULL);
>
> - if (!ret)
> - bpf_trampoline_module_put(tr);
> return ret;
> }
>
> @@ -238,9 +216,6 @@ static int register_fentry(struct bpf_trampoline *tr, void *new_addr)
> tr->func.ftrace_managed = true;
> }
>
> - if (bpf_trampoline_module_get(tr))
> - return -ENOENT;
> -
> if (tr->func.ftrace_managed) {
> ftrace_set_filter_ip(tr->fops, (unsigned long)ip, 0, 1);
> ret = register_ftrace_direct_multi(tr->fops, (long)new_addr);
> @@ -248,8 +223,6 @@ static int register_fentry(struct bpf_trampoline *tr, void *new_addr)
> ret = bpf_arch_text_poke(ip, BPF_MOD_CALL, NULL, new_addr);
> }
>
> - if (ret)
> - bpf_trampoline_module_put(tr);
> return ret;
> }
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 388245e8826e..6a19bd450558 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -24,6 +24,7 @@
> #include <linux/bpf_lsm.h>
> #include <linux/btf_ids.h>
> #include <linux/poison.h>
> +#include "../module/internal.h"
>
> #include "disasm.h"
>
> @@ -16868,6 +16869,7 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
> const char *tname;
> struct btf *btf;
> long addr = 0;
> + struct module *mod = NULL;
>
> if (!btf_id) {
> bpf_log(log, "Tracing programs must provide btf_id\n");
> @@ -17041,7 +17043,17 @@ 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)) {
> + preempt_disable();
btf_try_get_module takes mutex, so you can't preempt_disable in here,
I got this when running the test:
[ 691.916989][ T2585] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:580
> + mod = btf_try_get_module(btf);
> + if (mod)
> + addr = find_kallsyms_symbol_value(mod, tname);
> + else
> + addr = 0;
> + preempt_enable();
> + } else {
> + addr = kallsyms_lookup_name(tname);
> + }
> if (!addr) {
> bpf_log(log,
> "The address of function %s cannot be found\n",
> @@ -17105,6 +17117,12 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
> tgt_info->tgt_addr = addr;
> tgt_info->tgt_name = tname;
> tgt_info->tgt_type = t;
> + if (mod) {
> + if (!prog->aux->mod)
> + prog->aux->mod = mod;
can this actually happen? would it be better to have bpf_check_attach_target
just to take take the module ref and return it in tgt_info->tgt_mod and it'd
be up to caller to decide what to do with that
thanks,
jirka
> + else
> + module_put(mod);
> + }
> return 0;
> }
>
> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index 2e2bf236f558..5cb103a46018 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -256,6 +256,11 @@ static inline bool sect_empty(const Elf_Shdr *sect)
> static inline void init_build_id(struct module *mod, const struct load_info *info) { }
> static inline void layout_symtab(struct module *mod, struct load_info *info) { }
> static inline void add_kallsyms(struct module *mod, const struct load_info *info) { }
> +static inline unsigned long find_kallsyms_symbol_value(struct module *mod
> + const char *name)
> +{
> + return 0;
> +}
> #endif /* CONFIG_KALLSYMS */
>
> #ifdef CONFIG_SYSFS
> --
> 2.39.1
>
