On 12/7/22 01:10, Alexei Starovoitov wrote:
On Mon, Dec 05, 2022 at 04:26:05PM +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>
Acked-by: Hao Luo <haoluo@xxxxxxxxxx>
---
include/linux/btf.h | 1 +
kernel/bpf/btf.c | 5 +++++
kernel/bpf/verifier.c | 5 ++++-
3 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/include/linux/btf.h b/include/linux/btf.h
index cbd6e4096f8c..b7b791d1f3d6 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -188,6 +188,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 c80bd8709e69..f78e8060efa6 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -7208,6 +7208,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;
+}
It feels that btf->name is leaking a bit of implementation detail.
How about doing:
struct module *btf_find_module(const struct btf *btf)
{
reutrn find_module(btf->name);
}
+
enum {
BTF_MODULE_F_LIVE = (1 << 0),
};
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1d51bd9596da..0c533db51f92 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -16483,7 +16483,10 @@ 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))
+ addr = kallsyms_lookup_name_in_module(btf_module_name(btf), tname);
and use find_kallsyms_symbol_value() here
(with preempt_disable dance).
There won't be a need for patch 1 too.
wdyt?
This makes sense to me maybe more than the current solution.
I'm not sure where it's best to do preempt_disable, though. Would you
rather do it here or inside find_kallsyms_symbol_value? Or should we
create a new wrapper for find_kallsyms_symbol_value, possibly outside of
kernel/module/internal.h?
Thanks!
+ else
+ addr = kallsyms_lookup_name(tname);
if (!addr) {
bpf_log(log,
"The address of function %s cannot be found\n",
--
2.38.1
