| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20240710071724.dada783db147d4aab6980041@kernel.org>
Date: Wed, 10 Jul 2024 07:17:24 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: "Masami Hiramatsu (Google)" <mhiramat@...nel.org>
Cc: LKML <linux-kernel@...r.kernel.org>, Linux trace kernel
<linux-trace-kernel@...r.kernel.org>, Steven Rostedt <rostedt@...dmis.org>,
Andrii Nakryiko <andrii@...nel.org>, Francis Laniel
<flaniel@...ux.microsoft.com>, Nikolay Kuratov <kniv@...dex-team.ru>,
bpf@...r.kernel.org
Subject: Re: [PATCH for-next v4] tracing/kprobes: Add symbol counting check
when module loads
On Tue, 9 Jul 2024 09:04:36 +0900
"Masami Hiramatsu (Google)" <mhiramat@...nel.org> wrote:
> From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
>
> Currently, kprobe event checks whether the target symbol name is unique
> or not, so that it does not put a probe on an unexpected place. But this
> skips the check if the target is on a module because the module may not
> be loaded.
>
> To fix this issue, this patch checks the number of probe target symbols
> in a target module when the module is loaded. If the probe is not on the
> unique name symbols in the module, it will be rejected at that point.
>
> Note that the symbol which has a unique name in the target module,
> it will be accepted even if there are same-name symbols in the
> kernel or other modules,
>
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@...nel.org>
Instead of version up, let me send a fix.
Thanks,
> ---
> Changes in v4:
> - Hide find_module() in try_module_get_by_name().
> - Add bpf ML.
> Changes in v3:
> - Update the patch description.
> - Update for latest probe/for-next
> Updated from last October post, which was dropped by test failure:
> https://lore.kernel.org/linux-trace-kernel/169854904604.132316.12500381416261460174.stgit@devnote2/
> Changes in v2:
> - Fix to skip checking uniqueness if the target module is not loaded.
> - Fix register_module_trace_kprobe() to pass correct symbol name.
> - Fix to call __register_trace_kprobe() from module callback.
> ---
> kernel/trace/trace_kprobe.c | 138 +++++++++++++++++++++++++++++--------------
> 1 file changed, 94 insertions(+), 44 deletions(-)
>
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 7fd0f8576e4c..61a6da808203 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -678,6 +678,21 @@ static int register_trace_kprobe(struct trace_kprobe *tk)
> }
>
> #ifdef CONFIG_MODULES
> +static int validate_module_probe_symbol(const char *modname, const char *symbol);
> +
> +static int register_module_trace_kprobe(struct module *mod, struct trace_kprobe *tk)
> +{
> + const char *p;
> + int ret = 0;
> +
> + p = strchr(trace_kprobe_symbol(tk), ':');
> + if (p)
> + ret = validate_module_probe_symbol(module_name(mod), p + 1);
> + if (!ret)
> + ret = __register_trace_kprobe(tk);
> + return ret;
> +}
> +
> /* Module notifier call back, checking event on the module */
> static int trace_kprobe_module_callback(struct notifier_block *nb,
> unsigned long val, void *data)
> @@ -696,7 +711,7 @@ static int trace_kprobe_module_callback(struct notifier_block *nb,
> if (trace_kprobe_within_module(tk, mod)) {
> /* Don't need to check busy - this should have gone. */
> __unregister_trace_kprobe(tk);
> - ret = __register_trace_kprobe(tk);
> + ret = register_module_trace_kprobe(mod, tk);
> if (ret)
> pr_warn("Failed to re-register probe %s on %s: %d\n",
> trace_probe_name(&tk->tp),
> @@ -747,17 +762,81 @@ static int count_mod_symbols(void *data, const char *name, unsigned long unused)
> return 0;
> }
>
> -static unsigned int number_of_same_symbols(char *func_name)
> +static unsigned int number_of_same_symbols(const char *mod, const char *func_name)
> {
> struct sym_count_ctx ctx = { .count = 0, .name = func_name };
>
> - kallsyms_on_each_match_symbol(count_symbols, func_name, &ctx.count);
> + if (!mod)
> + kallsyms_on_each_match_symbol(count_symbols, func_name, &ctx.count);
>
> - module_kallsyms_on_each_symbol(NULL, count_mod_symbols, &ctx);
> + module_kallsyms_on_each_symbol(mod, count_mod_symbols, &ctx);
>
> return ctx.count;
> }
>
> +static int validate_module_probe_symbol(const char *modname, const char *symbol)
> +{
> + unsigned int count = number_of_same_symbols(modname, symbol);
> +
> + if (count > 1) {
> + /*
> + * Users should use ADDR to remove the ambiguity of
> + * using KSYM only.
> + */
> + return -EADDRNOTAVAIL;
> + } else if (count == 0) {
> + /*
> + * We can return ENOENT earlier than when register the
> + * kprobe.
> + */
> + return -ENOENT;
> + }
> + return 0;
> +}
> +
> +#ifdef CONFIG_MODULES
> +/* Return NULL if the module is not loaded or under unloading. */
> +static struct module *try_module_get_by_name(const char *name)
> +{
> + struct module *mod;
> +
> + rcu_read_lock_sched();
> + mod = find_module(name);
> + if (mod && !try_module_get(mod))
> + mod = NULL;
> + rcu_read_unlock_sched();
> +
> + return mod;
> +}
> +#else
> +#define try_module_get_by_name(name) (NULL)
> +#endif
> +
> +static int validate_probe_symbol(char *symbol)
> +{
> + struct module *mod = NULL;
> + char *modname = NULL, *p;
> + int ret = 0;
> +
> + p = strchr(symbol, ':');
> + if (p) {
> + modname = symbol;
> + symbol = p + 1;
> + *p = '\0';
> + mod = try_module_get_by_name(modname);
> + if (!mod)
> + goto out;
> + }
> +
> + ret = validate_module_probe_symbol(modname, symbol);
> +out:
> + if (p)
> + *p = ':';
> + if (mod)
> + module_put(mod);
> + return ret;
> +}
> +
> static int trace_kprobe_entry_handler(struct kretprobe_instance *ri,
> struct pt_regs *regs);
>
> @@ -881,6 +960,14 @@ static int __trace_kprobe_create(int argc, const char *argv[])
> trace_probe_log_err(0, BAD_PROBE_ADDR);
> goto parse_error;
> }
> + ret = validate_probe_symbol(symbol);
> + if (ret) {
> + if (ret == -EADDRNOTAVAIL)
> + trace_probe_log_err(0, NON_UNIQ_SYMBOL);
> + else
> + trace_probe_log_err(0, BAD_PROBE_ADDR);
> + goto parse_error;
> + }
> if (is_return)
> ctx.flags |= TPARG_FL_RETURN;
> ret = kprobe_on_func_entry(NULL, symbol, offset);
> @@ -893,31 +980,6 @@ static int __trace_kprobe_create(int argc, const char *argv[])
> }
> }
>
> - if (symbol && !strchr(symbol, ':')) {
> - unsigned int count;
> -
> - count = number_of_same_symbols(symbol);
> - if (count > 1) {
> - /*
> - * Users should use ADDR to remove the ambiguity of
> - * using KSYM only.
> - */
> - trace_probe_log_err(0, NON_UNIQ_SYMBOL);
> - ret = -EADDRNOTAVAIL;
> -
> - goto error;
> - } else if (count == 0) {
> - /*
> - * We can return ENOENT earlier than when register the
> - * kprobe.
> - */
> - trace_probe_log_err(0, BAD_PROBE_ADDR);
> - ret = -ENOENT;
> -
> - goto error;
> - }
> - }
> -
> trace_probe_log_set_index(0);
> if (event) {
> ret = traceprobe_parse_event_name(&event, &group, gbuf,
> @@ -1835,21 +1897,9 @@ create_local_trace_kprobe(char *func, void *addr, unsigned long offs,
> char *event;
>
> if (func) {
> - unsigned int count;
> -
> - count = number_of_same_symbols(func);
> - if (count > 1)
> - /*
> - * Users should use addr to remove the ambiguity of
> - * using func only.
> - */
> - return ERR_PTR(-EADDRNOTAVAIL);
> - else if (count == 0)
> - /*
> - * We can return ENOENT earlier than when register the
> - * kprobe.
> - */
> - return ERR_PTR(-ENOENT);
> + ret = validate_probe_symbol(func);
> + if (ret)
> + return ERR_PTR(ret);
> }
>
> /*
>
--
Masami Hiramatsu (Google) <mhiramat@...nel.org>
Powered by blists - more mailing lists