netdev - Re: [RFC] bpf: Fix crash on mm_init trampoline attachment

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YJFM/iLKb1EWCYEx@krava>
Date:   Tue, 4 May 2021 15:32:46 +0200
From:   Jiri Olsa <jolsa@...hat.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Jiri Olsa <jolsa@...nel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andriin@...com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>
Subject: Re: [RFC] bpf: Fix crash on mm_init trampoline attachment

On Mon, May 03, 2021 at 03:45:28PM -0700, Andrii Nakryiko wrote:
> On Fri, Apr 30, 2021 at 6:48 AM Jiri Olsa <jolsa@...nel.org> wrote:
> >
> > There are 2 mm_init functions in kernel.
> >
> > One in kernel/fork.c:
> >   static struct mm_struct *mm_init(struct mm_struct *mm,
> >                                    struct task_struct *p,
> >                                    struct user_namespace *user_ns)
> >
> > And another one in init/main.c:
> >   static void __init mm_init(void)
> >
> > The BTF data will get the first one, which is most likely
> > (in my case) mm_init from init/main.c without arguments.
> >
> > Then in runtime when we want to attach to 'mm_init' the
> > kalsyms contains address of the one from kernel/fork.c.
> >
> > So we have function model with no arguments and using it
> > to attach function with 3 arguments.. as result the trampoline
> > will not save function's arguments and we get crash because
> > trampoline changes argument registers:
> >
> >   BUG: unable to handle page fault for address: 0000607d87a1d558
> >   #PF: supervisor write access in kernel mode
> >   #PF: error_code(0x0002) - not-present page
> >   PGD 0 P4D 0
> >   Oops: 0002 [#1] SMP PTI
> >   CPU: 6 PID: 936 Comm: systemd Not tainted 5.12.0-rc4qemu+ #191
> >   RIP: 0010:mm_init+0x223/0x2a0
> >   ...
> >   Call Trace:
> >    ? bpf_trampoline_6442453476_0+0x3b/0x1000
> >    dup_mm+0x66/0x5f0
> >    ? __lock_task_sighand+0x3a/0x70
> >    copy_process+0x17d0/0x1b50
> >    kernel_clone+0x97/0x3c0
> >    __do_sys_clone+0x60/0x80
> >    do_syscall_64+0x33/0x40
> >    entry_SYSCALL_64_after_hwframe+0x44/0xae
> >   RIP: 0033:0x7f1dc9b3201f
> >
> > I think there might be more cases like this, but I don't have
> > an idea yet how to solve this in generic way. The rename in
> > this change fix it for this instance.
> 
> Just retroactively renaming functions and waiting for someone else to
> report similar issues is probably not the best strategy. Should
> resolve_btfids detect all name duplicates and emit warnings for them?
> It would be good to also remove such name-conflicting FUNCs from BTF
> (though currently it's not easy). And fail if such a function is
> referenced from .BTF_ids section.
> 
> Thoughts?

I guess we can do more checks, but I think the problem is the BTF
data vs address we get from kallsyms based on function name

we can easily get conflict address for another function with
different args

not sure how to ensure we have the proper address..  storing the
address in BTF data?

> 
> >
> > Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> > ---
> >  init/main.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/init/main.c b/init/main.c
> > index 53b278845b88..bc1bfe57daf7 100644
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -818,7 +818,7 @@ static void __init report_meminit(void)
> >  /*
> >   * Set up kernel memory allocators
> >   */
> > -static void __init mm_init(void)
> > +static void __init init_mem(void)
> >  {
> >         /*
> >          * page_ext requires contiguous pages,
> > @@ -905,7 +905,7 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
> >         vfs_caches_init_early();
> >         sort_main_extable();
> >         trap_init();
> > -       mm_init();
> > +       init_mem();
> 
> nit: given trap_init and ftrace_init, mem_init probably would be a better name?

it's taken ;-)

jirka