All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1] seccomp: Release filter when copy_process() fails.
@ 2022-08-22 20:44 Kuniyuki Iwashima
  2022-08-22 21:16 ` Kees Cook
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Kuniyuki Iwashima @ 2022-08-22 20:44 UTC (permalink / raw)
  To: Kees Cook, Andy Lutomirski, Will Drewry
  Cc: Christian Brauner, Kuniyuki Iwashima, Kuniyuki Iwashima,
	linux-kernel, syzbot+ab17848fe269b573eb71, Ayushman Dutta

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 ref count.

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, let's call seccomp_filter_release() in copy_process()
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>
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 kernel/fork.c    | 2 ++
 kernel/seccomp.c | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index 90c85b17bf69..031290fc527c 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -537,6 +537,7 @@ void put_task_stack(struct task_struct *tsk)
 
 void free_task(struct task_struct *tsk)
 {
+	WARN_ON_ONCE(tsk->seccomp.filter);
 	release_user_cpus_ptr(tsk);
 	scs_release(tsk);
 
@@ -2498,6 +2499,7 @@ static __latent_entropy struct task_struct *copy_process(
 	sched_core_free(p);
 	spin_unlock(&current->sighand->siglock);
 	write_unlock_irq(&tasklist_lock);
+	seccomp_filter_release(p);
 	cgroup_cancel_fork(p, args);
 bad_fork_put_pidfd:
 	if (clone_flags & CLONE_PIDFD) {
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index e9852d1b4a5e..017c1dd5b0d4 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -563,8 +563,6 @@ void seccomp_filter_release(struct task_struct *tsk)
 {
 	struct seccomp_filter *orig = tsk->seccomp.filter;
 
-	/* We are effectively holding the siglock by not having any sighand. */
-	WARN_ON(tsk->sighand != NULL);
 
 	/* Detach task from its filter tree. */
 	tsk->seccomp.filter = NULL;
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v1] seccomp: Release filter when copy_process() fails.
  2022-08-22 20:44 [PATCH v1] seccomp: Release filter when copy_process() fails Kuniyuki Iwashima
@ 2022-08-22 21:16 ` Kees Cook
  2022-08-22 21:49   ` Kuniyuki Iwashima
  2022-09-02  3:16 ` kernel test robot
  2022-09-02  3:16 ` kernel test robot
  2 siblings, 1 reply; 7+ messages in thread
From: Kees Cook @ 2022-08-22 21:16 UTC (permalink / raw)
  To: Kuniyuki Iwashima
  Cc: Andy Lutomirski, Will Drewry, Christian Brauner,
	Kuniyuki Iwashima, linux-kernel, syzbot+ab17848fe269b573eb71,
	Ayushman Dutta

On Mon, Aug 22, 2022 at 01:44:36PM -0700, Kuniyuki Iwashima wrote:
> 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 ref count.
> 
> 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, let's call seccomp_filter_release() in copy_process()
> and add a WARN_ON_ONCE() in free_task() for future debugging.

Thanks for tracking this down! I think I'd prefer to avoid changing the
semantics around the existing seccomp refcount lifetime, so what about
just moving copy_seccomp() below the last possible error path?


diff --git a/kernel/fork.c b/kernel/fork.c
index 90c85b17bf69..e7f4e7f1e01e 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2409,12 +2409,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 +2425,14 @@ static __latent_entropy struct task_struct *copy_process(
 		goto bad_fork_cancel_cgroup;
 	}
 
+	/* No more failures 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);


Totally untested, but I think it would fix this?

-Kees

-- 
Kees Cook

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v1] seccomp: Release filter when copy_process() fails.
  2022-08-22 21:16 ` Kees Cook
@ 2022-08-22 21:49   ` Kuniyuki Iwashima
  2022-08-22 23:38     ` Kees Cook
  0 siblings, 1 reply; 7+ messages in thread
From: Kuniyuki Iwashima @ 2022-08-22 21:49 UTC (permalink / raw)
  To: keescook
  Cc: ayudutta, brauner, kuni1840, kuniyu, linux-kernel, luto,
	syzbot+ab17848fe269b573eb71, wad

From:   Kees Cook <keescook@chromium.org>
Date:   Mon, 22 Aug 2022 14:16:03 -0700
> On Mon, Aug 22, 2022 at 01:44:36PM -0700, Kuniyuki Iwashima wrote:
> > 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 ref count.
> > 
> > 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, let's call seccomp_filter_release() in copy_process()
> > and add a WARN_ON_ONCE() in free_task() for future debugging.
> 
> Thanks for tracking this down! I think I'd prefer to avoid changing the
> semantics around the existing seccomp refcount lifetime, so what about
> just moving copy_seccomp() below the last possible error path?

Actually, I also thought of it but avoid it because it means we move the
signal check relatively earlier than before, so would-be-killed processes
could consume more resouces.

What do you think about this?

> 
> 
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 90c85b17bf69..e7f4e7f1e01e 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2409,12 +2409,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 +2425,14 @@ static __latent_entropy struct task_struct *copy_process(
>  		goto bad_fork_cancel_cgroup;
>  	}
>  
> +	/* No more failures 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);
> 
> 
> Totally untested, but I think it would fix this?
> 
> -Kees
> 
> -- 
> Kees Cook

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1] seccomp: Release filter when copy_process() fails.
  2022-08-22 21:49   ` Kuniyuki Iwashima
@ 2022-08-22 23:38     ` Kees Cook
  2022-08-23  0:00       ` Kuniyuki Iwashima
  0 siblings, 1 reply; 7+ messages in thread
From: Kees Cook @ 2022-08-22 23:38 UTC (permalink / raw)
  To: Kuniyuki Iwashima
  Cc: ayudutta, brauner, kuni1840, linux-kernel, luto,
	syzbot+ab17848fe269b573eb71, wad

On Mon, Aug 22, 2022 at 02:49:35PM -0700, Kuniyuki Iwashima wrote:
> From:   Kees Cook <keescook@chromium.org>
> Date:   Mon, 22 Aug 2022 14:16:03 -0700
> > On Mon, Aug 22, 2022 at 01:44:36PM -0700, Kuniyuki Iwashima wrote:
> > > 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 ref count.
> > > 
> > > 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, let's call seccomp_filter_release() in copy_process()
> > > and add a WARN_ON_ONCE() in free_task() for future debugging.
> > 
> > Thanks for tracking this down! I think I'd prefer to avoid changing the
> > semantics around the existing seccomp refcount lifetime, so what about
> > just moving copy_seccomp() below the last possible error path?
> 
> Actually, I also thought of it but avoid it because it means we move the
> signal check relatively earlier than before, so would-be-killed processes
> could consume more resouces.
> 
> What do you think about this?

There's no allocation happening in copy_seccomp(), just reference
counts being added. Given the lock that is held, the ordering here
doesn't matter as far as I can tell, except for the fact that
copy_seccomp() expects to go through full thread death if something goes
wrong. So, simply moving it later should do the trick here.

-Kees

> 
> > 
> > 
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index 90c85b17bf69..e7f4e7f1e01e 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -2409,12 +2409,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 +2425,14 @@ static __latent_entropy struct task_struct *copy_process(
> >  		goto bad_fork_cancel_cgroup;
> >  	}
> >  
> > +	/* No more failures 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);
> > 
> > 
> > Totally untested, but I think it would fix this?
> > 
> > -Kees
> > 
> > -- 
> > Kees Cook

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1] seccomp: Release filter when copy_process() fails.
  2022-08-22 23:38     ` Kees Cook
@ 2022-08-23  0:00       ` Kuniyuki Iwashima
  0 siblings, 0 replies; 7+ messages in thread
From: Kuniyuki Iwashima @ 2022-08-23  0:00 UTC (permalink / raw)
  To: keescook
  Cc: ayudutta, brauner, kuni1840, kuniyu, linux-kernel, luto,
	syzbot+ab17848fe269b573eb71, wad

From:   Kees Cook <keescook@chromium.org>
Date:   Mon, 22 Aug 2022 16:38:07 -0700
> On Mon, Aug 22, 2022 at 02:49:35PM -0700, Kuniyuki Iwashima wrote:
> > From:   Kees Cook <keescook@chromium.org>
> > Date:   Mon, 22 Aug 2022 14:16:03 -0700
> > > On Mon, Aug 22, 2022 at 01:44:36PM -0700, Kuniyuki Iwashima wrote:
> > > > 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 ref count.
> > > > 
> > > > 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, let's call seccomp_filter_release() in copy_process()
> > > > and add a WARN_ON_ONCE() in free_task() for future debugging.
> > > 
> > > Thanks for tracking this down! I think I'd prefer to avoid changing the
> > > semantics around the existing seccomp refcount lifetime, so what about
> > > just moving copy_seccomp() below the last possible error path?
> > 
> > Actually, I also thought of it but avoid it because it means we move the
> > signal check relatively earlier than before, so would-be-killed processes
> > could consume more resouces.
> > 
> > What do you think about this?
> 
> There's no allocation happening in copy_seccomp(), just reference
> counts being added. Given the lock that is held, the ordering here
> doesn't matter as far as I can tell, except for the fact that
> copy_seccomp() expects to go through full thread death if something goes
> wrong. So, simply moving it later should do the trick here.

Ok, I'm fine with that change.
I'll test it again and post v2 with WARN_ON_ONCE().
Thank you!


> 
> -Kees
> 
> > 
> > > 
> > > 
> > > diff --git a/kernel/fork.c b/kernel/fork.c
> > > index 90c85b17bf69..e7f4e7f1e01e 100644
> > > --- a/kernel/fork.c
> > > +++ b/kernel/fork.c
> > > @@ -2409,12 +2409,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 +2425,14 @@ static __latent_entropy struct task_struct *copy_process(
> > >  		goto bad_fork_cancel_cgroup;
> > >  	}
> > >  
> > > +	/* No more failures 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);
> > > 
> > > 
> > > Totally untested, but I think it would fix this?
> > > 
> > > -Kees
> > > 
> > > -- 
> > > Kees Cook
> 
> -- 
> Kees Cook

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1] seccomp: Release filter when copy_process() fails.
  2022-08-22 20:44 [PATCH v1] seccomp: Release filter when copy_process() fails Kuniyuki Iwashima
  2022-08-22 21:16 ` Kees Cook
@ 2022-09-02  3:16 ` kernel test robot
  2022-09-02  3:16 ` kernel test robot
  2 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2022-09-02  3:16 UTC (permalink / raw)
  To: Kuniyuki Iwashima, Kees Cook, Andy Lutomirski, Will Drewry
  Cc: llvm, kbuild-all, Christian Brauner, Kuniyuki Iwashima,
	linux-kernel, syzbot+ab17848fe269b573eb71, Ayushman Dutta

Hi Kuniyuki,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kees/for-next/pstore]
[also build test ERROR on linus/master v6.0-rc3]
[cannot apply to kees/for-next/seccomp next-20220901]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Kuniyuki-Iwashima/seccomp-Release-filter-when-copy_process-fails/20220823-044710
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/pstore
config: s390-buildonly-randconfig-r006-20220901 (https://download.01.org/0day-ci/archive/20220902/202209021045.YbjuMbic-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project c55b41d5199d2394dd6cdb8f52180d8b81d809d4)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install s390 cross compiling tool for clang build
        # apt-get install binutils-s390x-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/ea0fdd2f2e9a09bd5428bd59e81bac66e949f578
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Kuniyuki-Iwashima/seccomp-Release-filter-when-copy_process-fails/20220823-044710
        git checkout ea0fdd2f2e9a09bd5428bd59e81bac66e949f578
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from kernel/fork.c:79:
   In file included from include/linux/tty.h:12:
   In file included from include/linux/tty_port.h:5:
   In file included from include/linux/kfifo.h:42:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
                                                             ^
   include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
   #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
                                                        ^
   In file included from kernel/fork.c:79:
   In file included from include/linux/tty.h:12:
   In file included from include/linux/tty_port.h:5:
   In file included from include/linux/kfifo.h:42:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
   #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
                                                        ^
   In file included from kernel/fork.c:79:
   In file included from include/linux/tty.h:12:
   In file included from include/linux/tty_port.h:5:
   In file included from include/linux/kfifo.h:42:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
>> kernel/fork.c:540:28: error: no member named 'filter' in 'struct seccomp'
           WARN_ON_ONCE(tsk->seccomp.filter);
                        ~~~~~~~~~~~~ ^
   include/asm-generic/bug.h:104:25: note: expanded from macro 'WARN_ON_ONCE'
           int __ret_warn_on = !!(condition);                      \
                                  ^~~~~~~~~
   kernel/fork.c:854:20: warning: no previous prototype for function 'arch_task_cache_init' [-Wmissing-prototypes]
   void __init __weak arch_task_cache_init(void) { }
                      ^
   kernel/fork.c:854:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void __init __weak arch_task_cache_init(void) { }
   ^
   static 
   13 warnings and 1 error generated.


vim +540 kernel/fork.c

   537	
   538	void free_task(struct task_struct *tsk)
   539	{
 > 540		WARN_ON_ONCE(tsk->seccomp.filter);
   541		release_user_cpus_ptr(tsk);
   542		scs_release(tsk);
   543	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1] seccomp: Release filter when copy_process() fails.
  2022-08-22 20:44 [PATCH v1] seccomp: Release filter when copy_process() fails Kuniyuki Iwashima
  2022-08-22 21:16 ` Kees Cook
  2022-09-02  3:16 ` kernel test robot
@ 2022-09-02  3:16 ` kernel test robot
  2 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2022-09-02  3:16 UTC (permalink / raw)
  To: Kuniyuki Iwashima, Kees Cook, Andy Lutomirski, Will Drewry
  Cc: kbuild-all, Christian Brauner, Kuniyuki Iwashima, linux-kernel,
	syzbot+ab17848fe269b573eb71, Ayushman Dutta

Hi Kuniyuki,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kees/for-next/pstore]
[also build test ERROR on linus/master v6.0-rc3]
[cannot apply to kees/for-next/seccomp next-20220901]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Kuniyuki-Iwashima/seccomp-Release-filter-when-copy_process-fails/20220823-044710
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/pstore
config: arc-randconfig-r043-20220901 (https://download.01.org/0day-ci/archive/20220902/202209021116.hpxQOERU-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/ea0fdd2f2e9a09bd5428bd59e81bac66e949f578
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Kuniyuki-Iwashima/seccomp-Release-filter-when-copy_process-fails/20220823-044710
        git checkout ea0fdd2f2e9a09bd5428bd59e81bac66e949f578
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   kernel/fork.c:163:13: warning: no previous prototype for 'arch_release_task_struct' [-Wmissing-prototypes]
     163 | void __weak arch_release_task_struct(struct task_struct *tsk)
         |             ^~~~~~~~~~~~~~~~~~~~~~~~
   In file included from arch/arc/include/asm/bug.h:30,
                    from include/linux/bug.h:5,
                    from include/linux/mmdebug.h:5,
                    from include/linux/gfp.h:5,
                    from include/linux/slab.h:15,
                    from kernel/fork.c:16:
   kernel/fork.c: In function 'free_task':
>> kernel/fork.c:540:34: error: 'struct seccomp' has no member named 'filter'
     540 |         WARN_ON_ONCE(tsk->seccomp.filter);
         |                                  ^
   include/asm-generic/bug.h:166:32: note: in definition of macro 'WARN_ON'
     166 |         int __ret_warn_on = !!(condition);                              \
         |                                ^~~~~~~~~
   kernel/fork.c:540:9: note: in expansion of macro 'WARN_ON_ONCE'
     540 |         WARN_ON_ONCE(tsk->seccomp.filter);
         |         ^~~~~~~~~~~~
   kernel/fork.c: At top level:
   kernel/fork.c:854:20: warning: no previous prototype for 'arch_task_cache_init' [-Wmissing-prototypes]
     854 | void __init __weak arch_task_cache_init(void) { }
         |                    ^~~~~~~~~~~~~~~~~~~~
   kernel/fork.c:949:12: warning: no previous prototype for 'arch_dup_task_struct' [-Wmissing-prototypes]
     949 | int __weak arch_dup_task_struct(struct task_struct *dst,
         |            ^~~~~~~~~~~~~~~~~~~~


vim +540 kernel/fork.c

   537	
   538	void free_task(struct task_struct *tsk)
   539	{
 > 540		WARN_ON_ONCE(tsk->seccomp.filter);
   541		release_user_cpus_ptr(tsk);
   542		scs_release(tsk);
   543	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-09-02  3:16 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-22 20:44 [PATCH v1] seccomp: Release filter when copy_process() fails Kuniyuki Iwashima
2022-08-22 21:16 ` Kees Cook
2022-08-22 21:49   ` Kuniyuki Iwashima
2022-08-22 23:38     ` Kees Cook
2022-08-23  0:00       ` Kuniyuki Iwashima
2022-09-02  3:16 ` kernel test robot
2022-09-02  3:16 ` kernel test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.