Re: [PATCH bpf-next 1/2] bpf: Fix attaching fentry/fexit/fmod_ret/lsm to modules

[Jiri Olsa <olsajiri@gmail.com>]

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
>