| 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: <Zet8ykBtJhfVJuYR@gmail.com>
Date: Fri, 8 Mar 2024 13:02:02 -0800
From: Calvin Owens <jcalvinowens@...il.com>
To: Christophe Leroy <christophe.leroy@...roup.eu>
Cc: Luis Chamberlain <mcgrof@...nel.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Alexei Starovoitov <ast@...nel.org>,
Steven Rostedt <rostedt@...dmis.org>,
Daniel Borkmann <daniel@...earbox.net>,
Andrii Nakryiko <andrii@...nel.org>,
Masami Hiramatsu <mhiramat@...nel.org>,
Naveen N Rao <naveen.n.rao@...ux.ibm.com>,
Anil S Keshavamurthy <anil.s.keshavamurthy@...el.com>,
David S Miller <davem@...emloft.net>,
Thomas Gleixner <tglx@...utronix.de>,
"bpf@...r.kernel.org" <bpf@...r.kernel.org>,
"linux-modules@...r.kernel.org" <linux-modules@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC][PATCH 3/4] kprobes: Allow kprobes with CONFIG_MODULES=n
On Thursday 03/07 at 22:16 +0000, Christophe Leroy wrote:
>
>
> Le 06/03/2024 à 21:05, Calvin Owens a écrit :
> > [Vous ne recevez pas souvent de courriers de jcalvinowens@...il.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
> >
> > If something like this is merged down the road, it can go in at leisure
> > once the module_alloc change is in: it's a one-way dependency.
>
> Too many #ifdef, please reorganise stuff to avoid that and avoid
> changing prototypes based of CONFIG_MODULES.
>
> Other few comments below.
TBH the ugliness here was just me trying not to trigger -Wunused, but
that was silly: as you point out below, it's unncessary. I'll clean it
up.
> >
> > Signed-off-by: Calvin Owens <jcalvinowens@...il.com>
> > ---
> > arch/Kconfig | 2 +-
> > kernel/kprobes.c | 22 ++++++++++++++++++++++
> > kernel/trace/trace_kprobe.c | 11 +++++++++++
> > 3 files changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/Kconfig b/arch/Kconfig
> > index cfc24ced16dd..e60ce984d095 100644
> > --- a/arch/Kconfig
> > +++ b/arch/Kconfig
> > @@ -52,8 +52,8 @@ config GENERIC_ENTRY
> >
> > config KPROBES
> > bool "Kprobes"
> > - depends on MODULES
> > depends on HAVE_KPROBES
> > + select MODULE_ALLOC
> > select KALLSYMS
> > select TASKS_RCU if PREEMPTION
> > help
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index 9d9095e81792..194270e17d57 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -1556,8 +1556,12 @@ static bool is_cfi_preamble_symbol(unsigned long addr)
> > str_has_prefix("__pfx_", symbuf);
> > }
> >
> > +#if IS_ENABLED(CONFIG_MODULES)
> > static int check_kprobe_address_safe(struct kprobe *p,
> > struct module **probed_mod)
> > +#else
> > +static int check_kprobe_address_safe(struct kprobe *p)
> > +#endif
>
> A bit ugly to have to change the prototype, why not just keep probed_mod
> at all time ?
>
> When CONFIG_MODULES is not selected, __module_text_address() returns
> NULL so it should work without that many #ifdefs.
>
> > {
> > int ret;
> >
> > @@ -1580,6 +1584,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
> > goto out;
> > }
> >
> > +#if IS_ENABLED(CONFIG_MODULES)
> > /* Check if 'p' is probing a module. */
> > *probed_mod = __module_text_address((unsigned long) p->addr);
> > if (*probed_mod) {
> > @@ -1603,6 +1608,8 @@ static int check_kprobe_address_safe(struct kprobe *p,
> > ret = -ENOENT;
> > }
> > }
> > +#endif
> > +
> > out:
> > preempt_enable();
> > jump_label_unlock();
> > @@ -1614,7 +1621,9 @@ int register_kprobe(struct kprobe *p)
> > {
> > int ret;
> > struct kprobe *old_p;
> > +#if IS_ENABLED(CONFIG_MODULES)
> > struct module *probed_mod;
> > +#endif
> > kprobe_opcode_t *addr;
> > bool on_func_entry;
> >
> > @@ -1633,7 +1642,11 @@ int register_kprobe(struct kprobe *p)
> > p->nmissed = 0;
> > INIT_LIST_HEAD(&p->list);
> >
> > +#if IS_ENABLED(CONFIG_MODULES)
> > ret = check_kprobe_address_safe(p, &probed_mod);
> > +#else
> > + ret = check_kprobe_address_safe(p);
> > +#endif
> > if (ret)
> > return ret;
> >
> > @@ -1676,8 +1689,10 @@ int register_kprobe(struct kprobe *p)
> > out:
> > mutex_unlock(&kprobe_mutex);
> >
> > +#if IS_ENABLED(CONFIG_MODULES)
> > if (probed_mod)
> > module_put(probed_mod);
> > +#endif
> >
> > return ret;
> > }
> > @@ -2482,6 +2497,7 @@ int kprobe_add_area_blacklist(unsigned long start, unsigned long end)
> > return 0;
> > }
> >
> > +#if IS_ENABLED(CONFIG_MODULES)
> > /* Remove all symbols in given area from kprobe blacklist */
> > static void kprobe_remove_area_blacklist(unsigned long start, unsigned long end)
> > {
> > @@ -2499,6 +2515,7 @@ static void kprobe_remove_ksym_blacklist(unsigned long entry)
> > {
> > kprobe_remove_area_blacklist(entry, entry + 1);
> > }
> > +#endif
> >
> > int __weak arch_kprobe_get_kallsym(unsigned int *symnum, unsigned long *value,
> > char *type, char *sym)
> > @@ -2564,6 +2581,7 @@ static int __init populate_kprobe_blacklist(unsigned long *start,
> > return ret ? : arch_populate_kprobe_blacklist();
> > }
> >
> > +#if IS_ENABLED(CONFIG_MODULES)
> > static void add_module_kprobe_blacklist(struct module *mod)
> > {
> > unsigned long start, end;
> > @@ -2665,6 +2683,7 @@ static struct notifier_block kprobe_module_nb = {
> > .notifier_call = kprobes_module_callback,
> > .priority = 0
> > };
> > +#endif /* IS_ENABLED(CONFIG_MODULES) */
> >
> > void kprobe_free_init_mem(void)
> > {
> > @@ -2724,8 +2743,11 @@ static int __init init_kprobes(void)
> > err = arch_init_kprobes();
> > if (!err)
> > err = register_die_notifier(&kprobe_exceptions_nb);
> > +
> > +#if IS_ENABLED(CONFIG_MODULES)
> > if (!err)
> > err = register_module_notifier(&kprobe_module_nb);
> > +#endif
> >
> > kprobes_initialized = (err == 0);
> > kprobe_sysctls_init();
> > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > index c4c6e0e0068b..dd4598f775b9 100644
> > --- a/kernel/trace/trace_kprobe.c
> > +++ b/kernel/trace/trace_kprobe.c
> > @@ -102,6 +102,7 @@ static nokprobe_inline bool trace_kprobe_has_gone(struct trace_kprobe *tk)
> > return kprobe_gone(&tk->rp.kp);
> > }
> >
> > +#if IS_ENABLED(CONFIG_MODULES)
> > static nokprobe_inline bool trace_kprobe_within_module(struct trace_kprobe *tk,
> > struct module *mod)
> > {
> > @@ -129,6 +130,12 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> >
> > return ret;
> > }
> > +#else
> > +static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
> > +{
> > + return true;
> > +}
> > +#endif
> >
> > static bool trace_kprobe_is_busy(struct dyn_event *ev)
> > {
> > @@ -670,6 +677,7 @@ static int register_trace_kprobe(struct trace_kprobe *tk)
> > return ret;
> > }
> >
> > +#if IS_ENABLED(CONFIG_MODULES)
> > /* Module notifier call back, checking event on the module */
> > static int trace_kprobe_module_callback(struct notifier_block *nb,
> > unsigned long val, void *data)
> > @@ -704,6 +712,7 @@ static struct notifier_block trace_kprobe_module_nb = {
> > .notifier_call = trace_kprobe_module_callback,
> > .priority = 1 /* Invoked after kprobe module callback */
> > };
> > +#endif /* IS_ENABLED(CONFIG_MODULES) */
> >
> > static int count_symbols(void *data, unsigned long unused)
> > {
> > @@ -1897,8 +1906,10 @@ static __init int init_kprobe_trace_early(void)
> > if (ret)
> > return ret;
> >
> > +#if IS_ENABLED(CONFIG_MODULES)
> > if (register_module_notifier(&trace_kprobe_module_nb))
> > return -EINVAL;
> Why a #if here ?
>
> If CONFIG_MODULES is not selected, register_module_notifier() return
> always 0.
>
> > +#endif
> >
> > return 0;
> > }
> > --
> > 2.43.0
> >
> >
Powered by blists - more mailing lists