On Sat, Dec 28, 2019 at 5:12 AM Christian Brauner
<christian.brauner@xxxxxxxxxx> wrote:
>
> On Thu, Dec 26, 2019 at 06:03:36PM +0000, Sargun Dhillon wrote:
> > This syscall allows for the retrieval of file descriptors from other
> > processes, based on their pidfd. This is possible using ptrace, and
> > injection of parasitic code to inject code which leverages SCM_RIGHTS
> > to move file descriptors between a tracee and a tracer. Unfortunately,
> > ptrace comes with a high cost of requiring the process to be stopped,
> > and breaks debuggers. This does not require stopping the process under
> > manipulation.
> >
> > One reason to use this is to allow sandboxers to take actions on file
> > descriptors on the behalf of another process. For example, this can be
> > combined with seccomp-bpf's user notification to do on-demand fd
> > extraction and take privileged actions. One such privileged action
> > is binding a socket to a privileged port.
> >
> > This also adds the syscall to all architectures at the same time.
> >
> > /* prototype */
> > /* flags is currently reserved and should be set to 0 */
> > int sys_pidfd_getfd(int pidfd, int fd, unsigned int flags);
> >
> > /* testing */
> > Ran self-test suite on x86_64
>
> Fyi, I'm likely going to rewrite/add parts of/to this once I apply.
>
> A few comments below.
>
> > diff --git a/kernel/pid.c b/kernel/pid.c
> > index 2278e249141d..4a551f947869 100644
> > --- a/kernel/pid.c
> > +++ b/kernel/pid.c
> > @@ -578,3 +578,106 @@ void __init pid_idr_init(void)
> > init_pid_ns.pid_cachep = KMEM_CACHE(pid,
> > SLAB_HWCACHE_ALIGN | SLAB_PANIC | SLAB_ACCOUNT);
> > }
> > +
> > +static struct file *__pidfd_fget(struct task_struct *task, int fd)
> > +{
> > + struct file *file;
> > + int ret;
> > +
> > + ret = mutex_lock_killable(&task->signal->cred_guard_mutex);
> > + if (ret)
> > + return ERR_PTR(ret);
> > +
> > + if (!ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS)) {
> > + file = ERR_PTR(-EPERM);
> > + goto out;
> > + }
> > +
> > + file = fget_task(task, fd);
> > + if (!file)
> > + file = ERR_PTR(-EBADF);
> > +
> > +out:
> > + mutex_unlock(&task->signal->cred_guard_mutex);
> > + return file;
> > +}
>
> Looking at this code now a bit closer, ptrace_may_access() and
> fget_task() both take task_lock(task) so this currently does:
>
> task_lock();
> /* check access */
> task_unlock();
>
> task_lock();
> /* get fd */
> task_unlock();
>
> which doesn't seem great.
>
> I would prefer if we could do:
> task_lock();
> /* check access */
> /* get fd */
> task_unlock();
>
> But ptrace_may_access() doesn't export an unlocked variant so _shrug_.
Right, it seems intentional that __ptrace_may_access isn't exported. We
can always change that later?
>
> But we can write this a little cleaner without the goto as:
>
> static struct file *__pidfd_fget(struct task_struct *task, int fd)
> {
> struct file *file;
> int ret;
>
> ret = mutex_lock_killable(&task->signal->cred_guard_mutex);
> if (ret)
> return ERR_PTR(ret);
>
> if (ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS))
> file = fget_task(task, fd);
> else
> file = ERR_PTR(-EPERM);
> mutex_unlock(&task->signal->cred_guard_mutex);
>
> return file ?: ERR_PTR(-EBADF);
> }
>
> If you don't like the ?: just do:
>
> if (!file)
> return ERR_PTR(-EBADF);
>
> return file;
>
> though I prefer the shorter ?: syntax which is perfect for shortcutting
> returns.
>
> > +
> > +static int pidfd_getfd(struct pid *pid, int fd)
> > +{
> > + struct task_struct *task;
> > + struct file *file;
> > + int ret, retfd;
> > +
> > + task = get_pid_task(pid, PIDTYPE_PID);
> > + if (!task)
> > + return -ESRCH;
> > +
> > + file = __pidfd_fget(task, fd);
> > + put_task_struct(task);
> > + if (IS_ERR(file))
> > + return PTR_ERR(file);
> > +
> > + retfd = get_unused_fd_flags(O_CLOEXEC);
> > + if (retfd < 0) {
> > + ret = retfd;
> > + goto out;
> > + }
> > +
> > + /*
> > + * security_file_receive must come last since it may have side effects
> > + * and cannot be reversed.
> > + */
> > + ret = security_file_receive(file);
>
> So I don't understand the comment here. Can you explain what the side
> effects are?
The LSM can modify the LSM blob, or emit an (audit) event, even though
the operation as a whole failed. Smack will report that file_receive
successfully happened even though it could not have happened,
because we were unable to provision a file descriptor.
Apparmor does similar, and also manipulates the LSM blob,
although that is undone by closing the file.
> security_file_receive() is called in two places: net/core/scm.c and
> net/compat.c. In both places it is called _before_ get_unused_fd_flags()
> so I don't know what's special here that would prevent us from doing the
> same. If there's no actual reason, please rewrite this functions as:
>
> static int pidfd_getfd(struct pid *pid, int fd)
> {
> int ret;
> struct task_struct *task;
> struct file *file;
>
> task = get_pid_task(pid, PIDTYPE_PID);
> if (!task)
> return -ESRCH;
>
> file = __pidfd_fget(task, fd);
> put_task_struct(task);
> if (IS_ERR(file))
> return PTR_ERR(file);
>
> ret = security_file_receive(file);
> if (ret) {
> fput(file);
> return ret;
> }
>
> ret = get_unused_fd_flags(O_CLOEXEC);
> if (ret < 0)
> fput(file);
> else
> fd_install(ret, file);
>
> return ret;
> }
