SECCOMP_RET_USER_NOTIF: listener improvements

SECCOMP_RET_USER_NOTIF: listener improvements

[Christian Brauner]

[-- Attachment #1: Type: text/plain, Size: 1437 bytes --]

Hey everyone,

So I was working on making use of the seccomp listener stuff and I
stumbled upon a problem. Imagine a scenario where:

1. Task T1 installs Filter F1 and gets and listener fd for that filter FD1
2. T1 sends FD1 via SCM_RIGHTS to task T2
   T2 now holds a reference to the same underlying struct file as FD1 via FD2
3. T2 registers FD2 in an event loop and starts listening for events
4. T1 exits and wipes FD1

Now, T2 still holds a reference to the filter via FD2 which references
the same underlying file as FD1 which has the seccomp filter stashed in
private_data.
So T2 will never get notified that the filter is essentially unused and
doesn't know when to exit, i.e. it has no way of telling when T1 and all
of its children using the same filter are gone.

I think we should have a way to do this *or* alternatively have a way to
attach a process to an existing filter.

The scenario described above arises pretty naturally on container
attach. The standard way of doing this is usually
fork() +  attach_namespaces() + clone(CLONE_PARENT) where you don't
share the filter of container's init. So the seccomp context has to be
recreated. [1]

Opinions?

Christian

[1]: Note, that systemd-nspawn is creating the process by talking
     to the container's systemd and requesting it runs the programs via
     transient units but this only works if systemd is run inside the
     container and if you trust the workload.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: SECCOMP_RET_USER_NOTIF: listener improvements

[Tycho Andersen]

On Wed, Apr 24, 2019 at 05:04:26PM +0200, Christian Brauner wrote:
> Hey everyone,
> 
> So I was working on making use of the seccomp listener stuff and I
> stumbled upon a problem. Imagine a scenario where:
> 
> 1. Task T1 installs Filter F1 and gets and listener fd for that filter FD1
> 2. T1 sends FD1 via SCM_RIGHTS to task T2
>    T2 now holds a reference to the same underlying struct file as FD1 via FD2
> 3. T2 registers FD2 in an event loop and starts listening for events
> 4. T1 exits and wipes FD1
> 
> Now, T2 still holds a reference to the filter via FD2 which references
> the same underlying file as FD1 which has the seccomp filter stashed in
> private_data.
> So T2 will never get notified that the filter is essentially unused and
> doesn't know when to exit, i.e. it has no way of telling when T1 and all
> of its children using the same filter are gone.
> 
> I think we should have a way to do this

Since the only way we ever allow creating a struct file * that points
to a struct seccomp_filter *, if there is a notifier attached, the
number of tasks still being monitored by a particular filter should be
filter->usage - 1 (assuming there is a notifier attached). So we could
augment __put_seccomp_filter() to check for this and send out a
message with a SECCOMP_NOTIF_FLAG_DEAD flag or something.

> *or* alternatively have a way to attach a process to an existing
> filter.

I also think this wouldn't be too hard, since the struct file * has a
reference to the filter. So I guess the question is: which of these
makes more sense?

Tycho

Re: SECCOMP_RET_USER_NOTIF: listener improvements

[Christian Brauner]

On Wed, Apr 24, 2019 at 09:20:01AM -0600, Tycho Andersen wrote:
> On Wed, Apr 24, 2019 at 05:04:26PM +0200, Christian Brauner wrote:
> > Hey everyone,
> > 
> > So I was working on making use of the seccomp listener stuff and I
> > stumbled upon a problem. Imagine a scenario where:
> > 
> > 1. Task T1 installs Filter F1 and gets and listener fd for that filter FD1
> > 2. T1 sends FD1 via SCM_RIGHTS to task T2
> >    T2 now holds a reference to the same underlying struct file as FD1 via FD2
> > 3. T2 registers FD2 in an event loop and starts listening for events
> > 4. T1 exits and wipes FD1
> > 
> > Now, T2 still holds a reference to the filter via FD2 which references
> > the same underlying file as FD1 which has the seccomp filter stashed in
> > private_data.
> > So T2 will never get notified that the filter is essentially unused and
> > doesn't know when to exit, i.e. it has no way of telling when T1 and all
> > of its children using the same filter are gone.
> > 
> > I think we should have a way to do this
> 
> Since the only way we ever allow creating a struct file * that points
> to a struct seccomp_filter *, if there is a notifier attached, the
> number of tasks still being monitored by a particular filter should be
> filter->usage - 1 (assuming there is a notifier attached). So we could
> augment __put_seccomp_filter() to check for this and send out a
> message with a SECCOMP_NOTIF_FLAG_DEAD flag or something.
> 
> > *or* alternatively have a way to attach a process to an existing
> > filter.
> 
> I also think this wouldn't be too hard, since the struct file * has a
> reference to the filter. So I guess the question is: which of these
> makes more sense?

Or we do both... But overall I'm very much in favor of the option to
attach a task to an existing filter. Re-creating a seccomp context is
extremely brittle and error-prone. This would take a way a major pain
point.

Christian

Re: SECCOMP_RET_USER_NOTIF: listener improvements

[Christian Brauner]

On Wed, Apr 24, 2019 at 05:23:05PM +0200, Christian Brauner wrote:
> On Wed, Apr 24, 2019 at 09:20:01AM -0600, Tycho Andersen wrote:
> > On Wed, Apr 24, 2019 at 05:04:26PM +0200, Christian Brauner wrote:
> > > Hey everyone,
> > > 
> > > So I was working on making use of the seccomp listener stuff and I
> > > stumbled upon a problem. Imagine a scenario where:
> > > 
> > > 1. Task T1 installs Filter F1 and gets and listener fd for that filter FD1
> > > 2. T1 sends FD1 via SCM_RIGHTS to task T2
> > >    T2 now holds a reference to the same underlying struct file as FD1 via FD2
> > > 3. T2 registers FD2 in an event loop and starts listening for events
> > > 4. T1 exits and wipes FD1
> > > 
> > > Now, T2 still holds a reference to the filter via FD2 which references
> > > the same underlying file as FD1 which has the seccomp filter stashed in
> > > private_data.
> > > So T2 will never get notified that the filter is essentially unused and
> > > doesn't know when to exit, i.e. it has no way of telling when T1 and all
> > > of its children using the same filter are gone.
> > > 
> > > I think we should have a way to do this
> > 
> > Since the only way we ever allow creating a struct file * that points
> > to a struct seccomp_filter *, if there is a notifier attached, the
> > number of tasks still being monitored by a particular filter should be
> > filter->usage - 1 (assuming there is a notifier attached). So we could
> > augment __put_seccomp_filter() to check for this and send out a
> > message with a SECCOMP_NOTIF_FLAG_DEAD flag or something.
> > 
> > > *or* alternatively have a way to attach a process to an existing
> > > filter.
> > 
> > I also think this wouldn't be too hard, since the struct file * has a
> > reference to the filter. So I guess the question is: which of these
> > makes more sense?
> 
> Or we do both... But overall I'm very much in favor of the option to

The reason why I say both is that you might not always want to join an
filter and also you still want a way to hand-off listener fds. My main
scenario is a single watcher that watches listener fds from multiple
processes each potentially with a different filter.

> attach a task to an existing filter. Re-creating a seccomp context is
> extremely brittle and error-prone. This would take a way a major pain
> point.
> 
> Christian