lkml.org 
[lkml]   [2020]   [May]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 2/3] seccomp: Introduce addfd ioctl to seccomp user notifier
    > 
    > I mean, yes, that's certainly better, but it just seems a shame that
    > everyone has to do the get_unused/put_unused dance just because of how
    > SCM_RIGHTS does this weird put_user() in the middle.
    >
    > Can anyone clarify the expected failure mode from SCM_RIGHTS? Can we
    > move the put_user() after instead? I think cleanup would just be:
    > replace_fd(fd, NULL, 0)
    >
    > So:
    >
    > (updated to skip sock updates on failure; thank you Christian!)
    >
    > int file_receive(int fd, unsigned long flags, struct file *file)
    > {
    > struct socket *sock;
    > int ret;
    >
    > ret = security_file_receive(file);
    > if (ret)
    > return ret;
    >
    > /* Install the file. */
    > if (fd == -1) {
    > ret = get_unused_fd_flags(flags);
    > if (ret >= 0)
    > fd_install(ret, get_file(file));
    > } else {
    > ret = replace_fd(fd, file, flags);
    > }
    >
    > /* Bump the sock usage counts. */
    > if (ret >= 0) {
    > sock = sock_from_file(addfd->file, &err);
    > if (sock) {
    > sock_update_netprioidx(&sock->sk->sk_cgrp_data);
    > sock_update_classid(&sock->sk->sk_cgrp_data);
    > }
    > }
    >
    > return ret;
    > }
    >
    > scm_detach_fds()
    > ...
    > for (i=0, cmfptr=(__force int __user *)CMSG_DATA(cm); i<fdmax;
    > i++, cmfptr++)
    > {
    > int new_fd;
    >
    > err = file_receive(-1, MSG_CMSG_CLOEXEC & msg->msg_flags
    > ? O_CLOEXEC : 0, fp[i]);
    > if (err < 0)
    > break;
    > new_fd = err;
    >
    Isn't the "right" way to do this to allocate a bunch of file descriptors,
    and fill up the user buffer with them, and then install the files? This
    seems to like half-install the file descriptors and then error out.

    I know that's the current behaviour, but that seems like a bad idea. Do
    we really want to perpetuate this half-broken state? I guess that some
    userspace programs could be depending on this -- and their recovery
    semantics could rely on this. I mean this is 10+ year old code.

    > err = put_user(err, cmfptr);
    > if (err) {
    > /*
    > * If we can't notify userspace that it got the
    > * fd, we need to unwind and remove it again.
    > */
    > replace_fd(new_fd, NULL, 0);
    > break;
    > }
    > }
    > ...
    >
    >
    >
    > --
    > Kees Cook

    \
     
     \ /
      Last update: 2020-05-30 05:59    [W:3.744 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site