In seccomp_set_mode_filter() with TSYNC | NEW_LISTENER, we first initialize
the listener fd, then check to see if we can actually use it later in
seccomp_may_assign_mode(), which can fail if anyone else in our thread
group has installed a filter and caused some divergence. If we can't, we
partially clean up the newly allocated file: we put the fd, put the file,
but don't actually clean up the *memory* that was allocated at
filter->notif. Let's clean that up too.
To accomplish this, let's hoist the actual "detach a notifier from a
filter" code to its own helper out of seccomp_notify_release(), so that in
case anyone adds stuff to init_listener(), they only have to add the
cleanup code in one spot. This does a bit of extra locking and such on the
failure path when the filter is not attached, but it's a slow failure path
anyway.
Fixes: 51891498f2da ("seccomp: allow TSYNC and USER_NOTIF together")
Reported-by: [email protected]
Signed-off-by: Tycho Andersen <[email protected]>
---
kernel/seccomp.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 3ee59ce0a323..bb0dd9ae699a 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -1109,13 +1109,12 @@ static long seccomp_set_mode_strict(void)
}
#ifdef CONFIG_SECCOMP_FILTER
-static int seccomp_notify_release(struct inode *inode, struct file *file)
+static void seccomp_notify_detach(struct seccomp_filter *filter)
{
- struct seccomp_filter *filter = file->private_data;
struct seccomp_knotif *knotif;
if (!filter)
- return 0;
+ return;
mutex_lock(&filter->notify_lock);
@@ -1142,6 +1141,13 @@ static int seccomp_notify_release(struct inode *inode, struct file *file)
kfree(filter->notif);
filter->notif = NULL;
mutex_unlock(&filter->notify_lock);
+}
+
+static int seccomp_notify_release(struct inode *inode, struct file *file)
+{
+ struct seccomp_filter *filter = file->private_data;
+
+ seccomp_notify_detach(filter);
__put_seccomp_filter(filter);
return 0;
}
@@ -1581,6 +1587,7 @@ static long seccomp_set_mode_filter(unsigned int flags,
listener_f->private_data = NULL;
fput(listener_f);
put_unused_fd(listener);
+ seccomp_notify_detach(prepared);
} else {
fd_install(listener, listener_f);
ret = listener;
base-commit: f75aef392f869018f78cfedf3c320a6b3fcfda6b
--
2.25.1
I've changed my e-mail address to tycho.pizza, so let's reflect that in
these files.
Signed-off-by: Tycho Andersen <[email protected]>
---
.mailmap | 1 +
MAINTAINERS | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/.mailmap b/.mailmap
index 332c7833057f..50096b96c85d 100644
--- a/.mailmap
+++ b/.mailmap
@@ -308,6 +308,7 @@ Tony Luck <[email protected]>
TripleX Chung <[email protected]> <[email protected]>
TripleX Chung <[email protected]> <[email protected]>
Tsuneo Yoshioka <[email protected]>
+Tycho Andersen <[email protected]> <[email protected]>
Uwe Kleine-König <[email protected]>
Uwe Kleine-König <[email protected]>
Uwe Kleine-König <[email protected]>
diff --git a/MAINTAINERS b/MAINTAINERS
index e4647c84c987..2c60f3ec2496 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9776,7 +9776,7 @@ F: drivers/scsi/53c700*
LEAKING_ADDRESSES
M: Tobin C. Harding <[email protected]>
-M: Tycho Andersen <[email protected]>
+M: Tycho Andersen <[email protected]>
L: [email protected]
S: Maintained
T: git git://git.kernel.org/pub/scm/linux/kernel/git/tobin/leaks.git
--
2.25.1
Hi Kees,
On Tue, Sep 01, 2020 at 07:40:17PM -0600, Tycho Andersen wrote:
> I've changed my e-mail address to tycho.pizza, so let's reflect that in
> these files.
Hopefully you can pick this one up too? :D
Thanks,
Tycho
On Tue, Sep 01, 2020 at 07:40:17PM -0600, Tycho Andersen wrote:
> I've changed my e-mail address to tycho.pizza, so let's reflect that in
> these files.
>
> Signed-off-by: Tycho Andersen <[email protected]>
> ---
> .mailmap | 1 +
> MAINTAINERS | 2 +-
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/.mailmap b/.mailmap
> index 332c7833057f..50096b96c85d 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -308,6 +308,7 @@ Tony Luck <[email protected]>
> TripleX Chung <[email protected]> <[email protected]>
> TripleX Chung <[email protected]> <[email protected]>
> Tsuneo Yoshioka <[email protected]>
> +Tycho Andersen <[email protected]> <[email protected]>
> Uwe Kleine-König <[email protected]>
> Uwe Kleine-König <[email protected]>
> Uwe Kleine-König <[email protected]>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e4647c84c987..2c60f3ec2496 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9776,7 +9776,7 @@ F: drivers/scsi/53c700*
>
> LEAKING_ADDRESSES
> M: Tobin C. Harding <[email protected]>
> -M: Tycho Andersen <[email protected]>
> +M: Tycho Andersen <[email protected]>
Honestly, I'm just acking this because I truly belive we need more pizza
in the kernel:
Acked-by: Christian Brauner <[email protected]>
Christian
On Tue, Sep 01, 2020 at 07:40:16PM -0600, Tycho Andersen wrote:
> In seccomp_set_mode_filter() with TSYNC | NEW_LISTENER, we first initialize
> the listener fd, then check to see if we can actually use it later in
> seccomp_may_assign_mode(), which can fail if anyone else in our thread
> group has installed a filter and caused some divergence. If we can't, we
> partially clean up the newly allocated file: we put the fd, put the file,
> but don't actually clean up the *memory* that was allocated at
> filter->notif. Let's clean that up too.
>
> To accomplish this, let's hoist the actual "detach a notifier from a
> filter" code to its own helper out of seccomp_notify_release(), so that in
> case anyone adds stuff to init_listener(), they only have to add the
> cleanup code in one spot. This does a bit of extra locking and such on the
> failure path when the filter is not attached, but it's a slow failure path
> anyway.
>
> Fixes: 51891498f2da ("seccomp: allow TSYNC and USER_NOTIF together")
> Reported-by: [email protected]
> Signed-off-by: Tycho Andersen <[email protected]>
> ---
This looks sane to me!
Acked-by: Christian Brauner <[email protected]>
One thing I noticed when checking the failure paths. In init_listener we
allocate the notifier by directly storing it into filter->notif and if
anon_inode_getfile() fails we simply kfree(filter->notif) but don't NULL
it and leave filter->notif pointing to freed memory.
Since we have a few places where we check filter->notif whether it is
initialized or not maybe we should NULL filter->notif if init_listener()
fails or alloc the notifier into a tmp variable and only assign it to
filter->notif after we can't fail anymore in init_listener().
Just a thought since the error path in seccomp_set_mode_filter() is a
little more complex. :)
Thanks!
Christian
On Wed, Sep 02, 2020 at 11:08:49AM +0200, Christian Brauner wrote:
> On Tue, Sep 01, 2020 at 07:40:16PM -0600, Tycho Andersen wrote:
> > In seccomp_set_mode_filter() with TSYNC | NEW_LISTENER, we first initialize
> > the listener fd, then check to see if we can actually use it later in
> > seccomp_may_assign_mode(), which can fail if anyone else in our thread
> > group has installed a filter and caused some divergence. If we can't, we
> > partially clean up the newly allocated file: we put the fd, put the file,
> > but don't actually clean up the *memory* that was allocated at
> > filter->notif. Let's clean that up too.
> >
> > To accomplish this, let's hoist the actual "detach a notifier from a
> > filter" code to its own helper out of seccomp_notify_release(), so that in
> > case anyone adds stuff to init_listener(), they only have to add the
> > cleanup code in one spot. This does a bit of extra locking and such on the
> > failure path when the filter is not attached, but it's a slow failure path
> > anyway.
> >
> > Fixes: 51891498f2da ("seccomp: allow TSYNC and USER_NOTIF together")
> > Reported-by: [email protected]
> > Signed-off-by: Tycho Andersen <[email protected]>
> > ---
>
> This looks sane to me!
> Acked-by: Christian Brauner <[email protected]>
>
> One thing I noticed when checking the failure paths. In init_listener we
> allocate the notifier by directly storing it into filter->notif and if
> anon_inode_getfile() fails we simply kfree(filter->notif) but don't NULL
> it and leave filter->notif pointing to freed memory.
> Since we have a few places where we check filter->notif whether it is
> initialized or not maybe we should NULL filter->notif if init_listener()
> fails or alloc the notifier into a tmp variable and only assign it to
> filter->notif after we can't fail anymore in init_listener().
>
> Just a thought since the error path in seccomp_set_mode_filter() is a
> little more complex. :)
Yeah it seems like we definitely should, and maybe this is what Kees
was talking about and I missed it.
I'll send a patch on top of this shortly.
Tycho
On Tue, 1 Sep 2020 19:40:16 -0600, Tycho Andersen wrote:
> In seccomp_set_mode_filter() with TSYNC | NEW_LISTENER, we first initialize
> the listener fd, then check to see if we can actually use it later in
> seccomp_may_assign_mode(), which can fail if anyone else in our thread
> group has installed a filter and caused some divergence. If we can't, we
> partially clean up the newly allocated file: we put the fd, put the file,
> but don't actually clean up the *memory* that was allocated at
> filter->notif. Let's clean that up too.
>
> [...]
Applied, thanks!
[1/2] seccomp: don't leak memory when filter install races
https://git.kernel.org/kees/c/a566a9012acd
[2/2] mailmap, MAINTAINERS: move to tycho.pizza
https://git.kernel.org/kees/c/19d1d49f2a8c
Best regards,
--
Kees Cook