Re: [PATCH v3 1/4] seccomp: add a return code to trap to userspace

[Tycho Andersen]

Hi Eric,

On Thu, Jun 14, 2018 at 04:53:51PM -0500, Eric W. Biederman wrote:
> >> static void seccomp_do_user_notification(...)
> >> {
> >>     ...
> >>     n.pid = get_task_pid(current, PIDTYPE_PID);
> >>     ...
> >> remove_list:
> >>     list_del(&n.list);
> >>     put_pid(n.pid);
> >>     ...
> >> }
> >> ...
> >> static ssize_t seccomp_notify_read(...)
> >> {
> >>     ...
> >>     unotif.pid = pid_vnr(knotif->pid);
> >>     ...
> >> }
> >> 
> >> I like holding the pid reference because it's what we do elsewhere when pid
> >> namespaces
> >> are a concern and it more precisely specifies what the knotif content needs
> >> to convey.
> >> Otherwise I don't think it makes a difference.
> >
> > Great, thanks, I'll do this. I guess we need a put_pid() here too.
> 
> A)  We know that the task is stopped.  Unless there is something
>     like SIGKILL that can make the task move you don't need to
>     take a reference to anything.

Yes, agreed. (I think the task can't die, because even if it gets an
interrupt, we hold the ->notify_lock here, so it'll block waiting for
that to remove itself from the notification queue.)

> B)  pid_vnr is the wrong answer.  When you create the struct file
>     and intialize the filter you need to capture the calling processes
>     pid namespace.  The you can use "pid_nr_ns(knotif->pid, filter->pid_ns);".
>     That will work consistently even if the file descriptor is passed
>     between processes.

We want the pid of the tracee in the tracer's namespace, so I'm not so
sure. Doesn't your code above give us the pid in the namespace of the
task that happened to create the struct file (which may be unrelated
to the namespace of the tracer)?

Tycho