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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 20 Jun 2018 08:41:25 -0600
From:   Tycho Andersen <tycho@...ho.ws>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Matthew Helsley <matt.helsley@...il.com>,
        Kees Cook <keescook@...omium.org>,
        lkml <linux-kernel@...r.kernel.org>,
        containers@...ts.linux-foundation.org,
        Oleg Nesterov <oleg@...hat.com>,
        Akihiro Suda <suda.akihiro@....ntt.co.jp>,
        Tyler Hicks <tyhicks@...onical.com>,
        Christian Brauner <christian.brauner@...ntu.com>,
        Andy Lutomirski <luto@...capital.net>,
        "Tobin C . Harding" <me@...in.cc>
Subject: Re: [PATCH v3 1/4] seccomp: add a return code to trap to userspace

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