* Re: [Phishing Risk] [PATCH] kprobe: fix kretprobe stack backtrace
2021-06-25 10:24 ` [Phishing Risk] [PATCH] kprobe: fix kretprobe stack backtrace Muchun Song
@ 2021-06-28 13:34 ` Masami Hiramatsu
0 siblings, 0 replies; 2+ messages in thread
From: Masami Hiramatsu @ 2021-06-28 13:34 UTC (permalink / raw)
To: Muchun Song
Cc: Qiang Wang, Naveen N . Rao, Anil S Keshavamurthy, David Miller,
Masami Hiramatsu, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, kpsingh, LKML, Networking, bpf, Chengming Zhou,
Xiongchun duan
On Fri, 25 Jun 2021 18:24:12 +0800
Muchun Song <songmuchun@bytedance.com> wrote:
> On Fri, Jun 25, 2021 at 4:49 PM Qiang Wang
> <wangqiang.wq.frank@bytedance.com> wrote:
> >
> > We found that we couldn't get the correct kernel stack from
> > kretprobe. For example:
> >
> > bpftrace -e 'kr:submit_bio {print(kstack)}'
> > Attaching 1 probe...
> >
> > kretprobe_trampoline+0
> >
> > kretprobe_trampoline+0
> >
> > The problem is caused by the wrong instruction register which
> > points to the address of kretprobe_trampoline in regs.
> > So we set the real return address in instruction register.
> > Finally, we tested and successfully fixed it.
> >
> > bpftrace -e 'kr:submit_bio {print(kstack)}'
> > Attaching 1 probe...
> >
> > ext4_mpage_readpages+475
> > read_pages+139
> > page_cache_ra_unbounded+417
> > filemap_get_pages+245
> > filemap_read+169
> > __kernel_read+327
> > bprm_execve+648
> > do_execveat_common.isra.39+409
> > __x64_sys_execve+50
> > do_syscall_64+54
> > entry_SYSCALL_64_after_hwframe+68
> >
> > Reported-by: Chengming Zhou <zhouchengming@bytedance.com>
> > Signed-off-by: Qiang Wang <wangqiang.wq.frank@bytedance.com>
>
> Seems like a bug. Maybe we should add a "Fixes" tag here.
No, that is not a bug in the kretprobes. If you carefully check
kretprobes provided the rp->addr as ip address. BPF just did not
use it.
Anyway, I already made a same patch in the below series.
https://lore.kernel.org/bpf/162400000592.506599.4695807810528866713.stgit@devnote2/
and you can see that the series is including below 3 patches for that change.
https://lore.kernel.org/bpf/162399997853.506599.13701157683968161733.stgit@devnote2/
https://lore.kernel.org/bpf/162399998747.506599.1115560529431673586.stgit@devnote2/
https://lore.kernel.org/bpf/162399999702.506599.16339931387573094059.stgit@devnote2/
Without these patches, this change will break other arch.
Thanks,
>
> > ---
> > kernel/kprobes.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index 745f08fdd..1130381ca 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -1899,6 +1899,9 @@ unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,
> > current->kretprobe_instances.first = node->next;
> > node->next = NULL;
> >
> > + /* Kretprobe handler expects address is the real return address */
> > + instruction_pointer_set(regs, (unsigned long)correct_ret_addr);
> > +
> > /* Run them.. */
> > while (first) {
> > ri = container_of(first, struct kretprobe_instance, llist);
> > --
> > 2.20.1
> >
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 2+ messages in thread