Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Kuniyuki Iwashima <kuniyu@amazon.com>
To:     <kuniyu@amazon.com>
CC:     <ayudutta@amazon.com>, <brauner@kernel.org>,
        <keescook@chromium.org>, <kuni1840@gmail.com>,
        <linux-kernel@vger.kernel.org>, <luto@amacapital.net>,
        <syzbot+ab17848fe269b573eb71@syzkaller.appspotmail.com>,
        <wad@chromium.org>
Subject: [PATCH v3] seccomp: Move copy_seccomp() to no failure path.
Date:   Mon, 29 Aug 2022 10:49:45 -0700
Message-ID: <20220829174945.4398-1-kuniyu@amazon.com>
In-Reply-To: <20220823154532.82913-1-kuniyu@amazon.com>
References: <20220823154532.82913-1-kuniyu@amazon.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
Precedence: bulk

Hi Kees,

Could you take a look at this?

Thank you.


From:   Kuniyuki Iwashima <kuniyu@amazon.com>
Date:   Tue, 23 Aug 2022 08:45:32 -0700
> Our syzbot instance reported memory leaks in do_seccomp() [0], similar
> to the report [1].  It shows that we miss freeing struct seccomp_filter
> and some objects included in it.
> 
> We can reproduce the issue with the program below [2] which calls one
> seccomp() and two clone() syscalls.
> 
> The first clone()d child exits earlier than its parent and sends a
> signal to kill it during the second clone(), more precisely before the
> fatal_signal_pending() test in copy_process().  When the parent receives
> the signal, it has to destroy the embryonic process and return -EINTR to
> user space.  In the failure path, we have to call seccomp_filter_release()
> to decrement the filter's refcount.
> 
> Initially, we called it in free_task() called from the failure path, but
> the commit 3a15fb6ed92c ("seccomp: release filter after task is fully
> dead") moved it to release_task() to notify user space as early as possible
> that the filter is no longer used.
> 
> To keep the change and current seccomp refcount semantics, let's move
> copy_seccomp() just after the signal check and add a WARN_ON_ONCE() in
> free_task() for future debugging.
> 
> [0]:
> unreferenced object 0xffff8880063add00 (size 256):
>   comm "repro_seccomp", pid 230, jiffies 4294687090 (age 9.914s)
>   hex dump (first 32 bytes):
>     01 00 00 00 01 00 00 00 00 00 00 00 00 00 00 00  ................
>     ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff  ................
>   backtrace:
>     do_seccomp (./include/linux/slab.h:600 ./include/linux/slab.h:733 kernel/seccomp.c:666 kernel/seccomp.c:708 kernel/seccomp.c:1871 kernel/seccomp.c:1991)
>     do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80)
>     entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120)
> unreferenced object 0xffffc90000035000 (size 4096):
>   comm "repro_seccomp", pid 230, jiffies 4294687090 (age 9.915s)
>   hex dump (first 32 bytes):
>     01 00 00 00 00 00 00 00 00 00 00 00 05 00 00 00  ................
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     __vmalloc_node_range (mm/vmalloc.c:3226)
>     __vmalloc_node (mm/vmalloc.c:3261 (discriminator 4))
>     bpf_prog_alloc_no_stats (kernel/bpf/core.c:91)
>     bpf_prog_alloc (kernel/bpf/core.c:129)
>     bpf_prog_create_from_user (net/core/filter.c:1414)
>     do_seccomp (kernel/seccomp.c:671 kernel/seccomp.c:708 kernel/seccomp.c:1871 kernel/seccomp.c:1991)
>     do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80)
>     entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120)
> unreferenced object 0xffff888003fa1000 (size 1024):
>   comm "repro_seccomp", pid 230, jiffies 4294687090 (age 9.915s)
>   hex dump (first 32 bytes):
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     bpf_prog_alloc_no_stats (./include/linux/slab.h:600 ./include/linux/slab.h:733 kernel/bpf/core.c:95)
>     bpf_prog_alloc (kernel/bpf/core.c:129)
>     bpf_prog_create_from_user (net/core/filter.c:1414)
>     do_seccomp (kernel/seccomp.c:671 kernel/seccomp.c:708 kernel/seccomp.c:1871 kernel/seccomp.c:1991)
>     do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80)
>     entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120)
> unreferenced object 0xffff888006360240 (size 16):
>   comm "repro_seccomp", pid 230, jiffies 4294687090 (age 9.915s)
>   hex dump (first 16 bytes):
>     01 00 37 00 76 65 72 6c e0 83 01 06 80 88 ff ff  ..7.verl........
>   backtrace:
>     bpf_prog_store_orig_filter (net/core/filter.c:1137)
>     bpf_prog_create_from_user (net/core/filter.c:1428)
>     do_seccomp (kernel/seccomp.c:671 kernel/seccomp.c:708 kernel/seccomp.c:1871 kernel/seccomp.c:1991)
>     do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80)
>     entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120)
> unreferenced object 0xffff8880060183e0 (size 8):
>   comm "repro_seccomp", pid 230, jiffies 4294687090 (age 9.915s)
>   hex dump (first 8 bytes):
>     06 00 00 00 00 00 ff 7f                          ........
>   backtrace:
>     kmemdup (mm/util.c:129)
>     bpf_prog_store_orig_filter (net/core/filter.c:1144)
>     bpf_prog_create_from_user (net/core/filter.c:1428)
>     do_seccomp (kernel/seccomp.c:671 kernel/seccomp.c:708 kernel/seccomp.c:1871 kernel/seccomp.c:1991)
>     do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80)
>     entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120)
> 
> [1]: https://syzkaller.appspot.com/bug?id=2809bb0ac77ad9aa3f4afe42d6a610aba594a987
> 
> [2]:
> #define _GNU_SOURCE
> #include <sched.h>
> #include <signal.h>
> #include <unistd.h>
> #include <sys/syscall.h>
> #include <linux/filter.h>
> #include <linux/seccomp.h>
> 
> void main(void)
> {
> 	struct sock_filter filter[] = {
> 		BPF_STMT(BPF_RET | BPF_K, SECCOMP_RET_ALLOW),
> 	};
> 	struct sock_fprog fprog = {
> 		.len = sizeof(filter) / sizeof(filter[0]),
> 		.filter = filter,
> 	};
> 	long i, pid;
> 
> 	syscall(__NR_seccomp, SECCOMP_SET_MODE_FILTER, 0, &fprog);
> 
> 	for (i = 0; i < 2; i++) {
> 		pid = syscall(__NR_clone, CLONE_NEWNET | SIGKILL, NULL, NULL, 0);
> 		if (pid == 0)
> 			return;
> 	}
> }
> 
> Fixes: 3a15fb6ed92c ("seccomp: release filter after task is fully dead")
> Reported-by: syzbot+ab17848fe269b573eb71@syzkaller.appspotmail.com
> Reported-by: Ayushman Dutta <ayudutta@amazon.com>
> Suggested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> ---
> v3:
>   * Fix build failure for CONFIG_SECCOMP=n case
> 
> v2: https://lore.kernel.org/lkml/20220823004806.38681-1-kuniyu@amazon.com/
>   * Move copy_seccomp() after no failure path instead of adding
>     seccomp_filter_release() in the failure path.
> 
> v1: https://lore.kernel.org/lkml/20220822204436.26631-1-kuniyu@amazon.com/
> ---
>  kernel/fork.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 90c85b17bf69..6ac1cc62f197 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -537,6 +537,9 @@ void put_task_stack(struct task_struct *tsk)
>  
>  void free_task(struct task_struct *tsk)
>  {
> +#ifdef CONFIG_SECCOMP
> +	WARN_ON_ONCE(tsk->seccomp.filter);
> +#endif
>  	release_user_cpus_ptr(tsk);
>  	scs_release(tsk);
>  
> @@ -2409,12 +2412,6 @@ static __latent_entropy struct task_struct *copy_process(
>  
>  	spin_lock(&current->sighand->siglock);
>  
> -	/*
> -	 * Copy seccomp details explicitly here, in case they were changed
> -	 * before holding sighand lock.
> -	 */
> -	copy_seccomp(p);
> -
>  	rv_task_fork(p);
>  
>  	rseq_fork(p, clone_flags);
> @@ -2431,6 +2428,14 @@ static __latent_entropy struct task_struct *copy_process(
>  		goto bad_fork_cancel_cgroup;
>  	}
>  
> +	/* No more failure paths after this point. */
> +
> +	/*
> +	 * Copy seccomp details explicitly here, in case they were changed
> +	 * before holding sighand lock.
> +	 */
> +	copy_seccomp(p);
> +
>  	init_task_pid_links(p);
>  	if (likely(p->pid)) {
>  		ptrace_init_task(p, (clone_flags & CLONE_PTRACE) || trace);
> -- 
> 2.30.2