Re: [RFC PATCH] ptrace: add PTRACE_GETFD request

On 2019-12-05, Sargun Dhillon [off-list ref] wrote:

On Thu, Dec 5, 2019 at 6:38 PM Jann Horn [off-list ref] wrote:

quoted

On Fri, Dec 6, 2019 at 12:44 AM Sargun Dhillon [off-list ref] wrote:

quoted

PTRACE_GETFD is a generic ptrace API that allows the tracer to
get file descriptors from the traceee.

typo: tracee

quoted

The primary reason to use this syscall is to allow sandboxers to
take action on an FD on behalf of the tracee. For example, this
can be combined with seccomp's user notification feature to extract
a file descriptor and call privileged syscalls, like binding
a socket to a privileged port.

[...]

quoted

+/* This gets a file descriptor from a running process. It doesn't require the
+ * process to be stopped.
+ */
+#define PTRACE_GETFD   0x420f

[...]

quoted

+static int ptrace_getfd(struct task_struct *child, unsigned long fd)

I'd make the "fd" parameter of this function an "unsigned int", given
that that's also the argument type of fcheck_files().

quoted

+{
+       struct files_struct *files;
+       struct file *file;
+       int ret = 0;
+
+       files = get_files_struct(child);
+       if (!files)
+               return -ENOENT;
+
+       spin_lock(&files->file_lock);
+       file = fcheck_files(files, fd);
+       if (!file)
+               ret = -EBADF;
+       else
+               get_file(file);
+       spin_unlock(&files->file_lock);
+       put_files_struct(files);
+
+       if (ret)
+               goto out;
+
+       ret = get_unused_fd_flags(0);

You're hardcoding the flags for the fd as 0, which means that there is
no way for the caller to enable O_CLOEXEC on the fd in a way that is
race-free against a concurrent execve(). If you can't easily plumb
through an O_CLOEXEC flag from userspace to here, you should probably
hardcode O_CLOEXEC here.

I thought about making addr used for flags. It seems a little weird,
given the name, but it'll do the job. Alternatively, it could be a
point to an options struct. If we introduce options, one of the nice
things we could add is add the ability to cleanse the FD of certain
information, like cgroups.

If you do end up opting for an options struct, please make sure you use
copy_struct_from_user() or something similar so that we can painlessly
extend it in the future (if necessary). Since there isn't an additional
argument, you might want to do what perf_event_open() does and embed the
size as the first field of the options struct.

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>