| 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: <20220306124819.2dfd845ad2dc37720c6d0827@kernel.org>
Date: Sun, 6 Mar 2022 12:48:19 +0900
From: Masami Hiramatsu <mhiramat@...nel.org>
To: Peter Zijlstra <peterz@...radead.org>
Cc: Steven Rostedt <rostedt@...dmis.org>,
"Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>,
alexei.starovoitov@...il.com, alyssa.milburn@...el.com,
andrew.cooper3@...rix.com, hjl.tools@...il.com,
joao@...rdrivepizza.com, jpoimboe@...hat.com,
keescook@...omium.org, linux-kernel@...r.kernel.org,
mark.rutland@....com, mbenes@...e.cz,
Masami Hiramatsu <mhiramat@...nel.org>,
ndesaulniers@...gle.com, samitolvanen@...gle.com, x86@...nel.org
Subject: Re: [PATCH v2 12/39] x86/ibt,ftrace: Search for __fentry__ location
On Thu, 3 Mar 2022 16:59:03 +0100
Peter Zijlstra <peterz@...radead.org> wrote:
> On Thu, Mar 03, 2022 at 09:34:13AM -0500, Steven Rostedt wrote:
> > On Thu, 3 Mar 2022 14:04:52 +0100
> > Peter Zijlstra <peterz@...radead.org> wrote:
> >
> > > > @@ -1596,7 +1596,7 @@ static int check_ftrace_location(struct kprobe *p)
> > > > {
> > > > unsigned long ftrace_addr;
> > > >
> > > > - ftrace_addr = ftrace_location((unsigned long)p->addr);
> > > > + ftrace_addr = ftrace_location_range((unsigned long)p->addr, (unsigned long)p->addr);
> > >
> > > Yes, although perhaps a new helper. I'll go ponder during lunch.
> >
> > Is there more places to add that to make it worth creating a helper?
>
> This is what I ended up with, I've looked at all ftrace_location() sites
> there are, seems to work too, both the built-in boot time ftrace tests
> and the selftests work splat-less.
>
> I should update the Changelog some though.
>
> Naveen also mentioned register_ftrace_direct() could be further cleaned
> up, but I didn't want to do too much in once go.
>
> ---
>
> Subject: x86/ibt,ftrace: Search for __fentry__ location
> From: Peter Zijlstra <peterz@...radead.org>
> Date: Wed Feb 23 10:01:38 CET 2022
>
> Have ftrace_location() search the symbol for the __fentry__ location
> when it isn't at func+0 and use this for {,un}register_ftrace_direct().
>
> This avoids a whole bunch of assumptions about __fentry__ being at
> func+0.
>
> Suggested-by: Steven Rostedt <rostedt@...dmis.org>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> ---
> arch/x86/kernel/kprobes/core.c | 11 +---------
> kernel/bpf/trampoline.c | 20 +++----------------
> kernel/kprobes.c | 8 +------
> kernel/trace/ftrace.c | 43 +++++++++++++++++++++++++++++++++--------
> 4 files changed, 43 insertions(+), 39 deletions(-)
>
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -193,17 +193,10 @@ static unsigned long
> __recover_probed_insn(kprobe_opcode_t *buf, unsigned long addr)
> {
> struct kprobe *kp;
> - unsigned long faddr;
> + bool faddr;
>
> kp = get_kprobe((void *)addr);
> - faddr = ftrace_location(addr);
> - /*
> - * Addresses inside the ftrace location are refused by
> - * arch_check_ftrace_location(). Something went terribly wrong
> - * if such an address is checked here.
> - */
> - if (WARN_ON(faddr && faddr != addr))
> - return 0UL;
> + faddr = ftrace_location(addr) == addr;
OK, this looks good to me.
> /*
> * Use the current code if it is not modified by Kprobe
> * and it cannot be modified by ftrace.
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -117,18 +117,6 @@ static void bpf_trampoline_module_put(st
> tr->mod = NULL;
> }
>
> -static int is_ftrace_location(void *ip)
> -{
> - long addr;
> -
> - addr = ftrace_location((long)ip);
> - if (!addr)
> - return 0;
> - if (WARN_ON_ONCE(addr != (long)ip))
> - return -EFAULT;
> - return 1;
> -}
> -
> static int unregister_fentry(struct bpf_trampoline *tr, void *old_addr)
> {
> void *ip = tr->func.addr;
> @@ -160,12 +148,12 @@ static int modify_fentry(struct bpf_tram
> static int register_fentry(struct bpf_trampoline *tr, void *new_addr)
> {
> void *ip = tr->func.addr;
> + unsigned long faddr;
> int ret;
>
> - ret = is_ftrace_location(ip);
> - if (ret < 0)
> - return ret;
> - tr->func.ftrace_managed = ret;
> + faddr = ftrace_location((unsigned long)ip);
> + if (faddr)
> + tr->func.ftrace_managed = true;
>
> if (bpf_trampoline_module_get(tr))
> return -ENOENT;
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1562,14 +1562,10 @@ static inline int warn_kprobe_rereg(stru
>
> static int check_ftrace_location(struct kprobe *p)
> {
> - unsigned long ftrace_addr;
> + unsigned long addr = (unsigned long)p->addr;
>
> - ftrace_addr = ftrace_location((unsigned long)p->addr);
> - if (ftrace_addr) {
> + if (ftrace_location(addr) == addr) {
> #ifdef CONFIG_KPROBES_ON_FTRACE
> - /* Given address is not on the instruction boundary */
> - if ((unsigned long)p->addr != ftrace_addr)
> - return -EILSEQ;
OK, so this means we only use the ftrace if the kprobe puts the
sym+ftrace-offset. Thus if there is ENDBR at the first instruction,
kprobe will use int3, right?
I agree with this, but later I have to add another patch to use ftrace
for the kprobes on symbol+0. But anyway, that is another issue.
So this looks good to me.
Acked-by: Masami Hiramatsu <mhiramat@...nel.org>
Thank you!
> p->flags |= KPROBE_FLAG_FTRACE;
> #else /* !CONFIG_KPROBES_ON_FTRACE */
> return -EINVAL;
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1568,17 +1568,34 @@ unsigned long ftrace_location_range(unsi
> }
>
> /**
> - * ftrace_location - return true if the ip giving is a traced location
> + * ftrace_location - return the ftrace location
> * @ip: the instruction pointer to check
> *
> - * Returns rec->ip if @ip given is a pointer to a ftrace location.
> - * That is, the instruction that is either a NOP or call to
> - * the function tracer. It checks the ftrace internal tables to
> - * determine if the address belongs or not.
> + * If @ip matches the ftrace location, return @ip.
> + * If @ip matches sym+0, return sym's ftrace location.
> + * Otherwise, return 0.
> */
> unsigned long ftrace_location(unsigned long ip)
> {
> - return ftrace_location_range(ip, ip);
> + struct dyn_ftrace *rec;
> + unsigned long offset;
> + unsigned long size;
> +
> + rec = lookup_rec(ip, ip);
> + if (!rec) {
> + if (!kallsyms_lookup_size_offset(ip, &size, &offset))
> + goto out;
> +
> + /* map sym+0 to __fentry__ */
> + if (!offset)
> + rec = lookup_rec(ip, ip + size - 1);
> + }
> +
> + if (rec)
> + return rec->ip;
> +
> +out:
> + return 0;
> }
>
> /**
> @@ -4962,7 +4979,8 @@ ftrace_match_addr(struct ftrace_hash *ha
> {
> struct ftrace_func_entry *entry;
>
> - if (!ftrace_location(ip))
> + ip = ftrace_location(ip);
> + if (!ip)
> return -EINVAL;
>
> if (remove) {
> @@ -5110,11 +5128,16 @@ int register_ftrace_direct(unsigned long
> struct ftrace_func_entry *entry;
> struct ftrace_hash *free_hash = NULL;
> struct dyn_ftrace *rec;
> - int ret = -EBUSY;
> + int ret = -ENODEV;
>
> mutex_lock(&direct_mutex);
>
> + ip = ftrace_location(ip);
> + if (!ip)
> + goto out_unlock;
> +
> /* See if there's a direct function at @ip already */
> + ret = -EBUSY;
> if (ftrace_find_rec_direct(ip))
> goto out_unlock;
>
> @@ -5222,6 +5245,10 @@ int unregister_ftrace_direct(unsigned lo
>
> mutex_lock(&direct_mutex);
>
> + ip = ftrace_location(ip);
> + if (!ip)
> + goto out_unlock;
> +
> entry = find_direct_entry(&ip, NULL);
> if (!entry)
> goto out_unlock;
--
Masami Hiramatsu <mhiramat@...nel.org>
Powered by blists - more mailing lists