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: <CAEf4BzbEjvccUDabpTiPOiXK=vfcmHaXjeaTL8gCr08=6fBqhg@mail.gmail.com>
Date:   Mon, 3 May 2021 15:45:28 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Jiri Olsa <jolsa@...nel.org>
Cc:     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 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?

>
> 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?

>
>         ftrace_init();
>
> --
> 2.30.2
>