On Sat, May 30, 2020 at 01:10:55AM +0000, Sargun Dhillon wrote:
> // And then SCM reads:
> for (i=0, cmfptr=(__force int __user *)CMSG_DATA(cm); i<fdmax;
> i++, cmfptr++)
> {
> int new_fd;
> err = get_unused_fd_flags(MSG_CMSG_CLOEXEC & msg->msg_flags
> ? O_CLOEXEC : 0);
> if (err < 0)
> break;
> new_fd = err;
> err = put_user(new_fd, cmfptr);
> if (err) {
> put_unused_fd(new_fd);
> break;
> }
>
> err = file_receive(new_fd, fp[i]);
> if (err) {
> put_unused_fd(new_fd);
> break;
> }
> }
>
> And our code reads:
>
>
> static void seccomp_handle_addfd(struct seccomp_kaddfd *addfd)
> {
> int ret, err;
>
> /*
> * Remove the notification, and reset the list pointers, indicating
> * that it has been handled.
> */
> list_del_init(&addfd->list);
>
> if (addfd->fd == -1) {
> ret = get_unused_fd_flags(addfd->flags);
> if (ret < 0)
> goto err;
>
> err = file_receive(ret, addfd->file);
> if (err) {
> put_unused_fd(ret);
> ret = err;
> }
> } else {
> ret = file_receive_replace(addfd->fd, addfd->flags,
> addfd->file);
> }
>
> err:
> addfd->ret = ret;
> complete(&addfd->completion);
> }
>
>
> And the pidfd getfd code reads:
>
> static int pidfd_getfd(struct pid *pid, int fd)
> {
> struct task_struct *task;
> struct file *file;
> int ret, err;
>
> 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 = get_unused_fd_flags(O_CLOEXEC);
> if (ret >= 0) {
> err = file_receive(ret, file);
> if (err) {
> put_unused_fd(ret);
> ret = err;
> }
> }
>
> fput(file);
> return ret;
> }
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;
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
