lkml.org 
[lkml]   [2019]   [Dec]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC PATCH] ptrace: add PTRACE_GETFD request
    On 2019-12-05, Sargun Dhillon <sargun@sargun.me> wrote:
    > On Thu, Dec 5, 2019 at 6:38 PM Jann Horn <jannh@google.com> wrote:
    > >
    > > On Fri, Dec 6, 2019 at 12:44 AM Sargun Dhillon <sargun@sargun.me> wrote:
    > > > PTRACE_GETFD is a generic ptrace API that allows the tracer to
    > > > get file descriptors from the traceee.
    > >
    > > typo: tracee
    > >
    > > > 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.
    > > [...]
    > > > +/* This gets a file descriptor from a running process. It doesn't require the
    > > > + * process to be stopped.
    > > > + */
    > > > +#define PTRACE_GETFD 0x420f
    > > [...]
    > > > +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().
    > >
    > > > +{
    > > > + 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/>
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2019-12-06 07:53    [W:2.845 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site