From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-yb1-f194.google.com ([209.85.219.194]:42929 "EHLO mail-yb1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726783AbeI1EUX (ORCPT ); Fri, 28 Sep 2018 00:20:23 -0400 Received: by mail-yb1-f194.google.com with SMTP id p74-v6so1786447ybc.9 for ; Thu, 27 Sep 2018 14:59:58 -0700 (PDT) Received: from mail-yw1-f49.google.com (mail-yw1-f49.google.com. [209.85.161.49]) by smtp.gmail.com with ESMTPSA id r12-v6sm1228193ywg.67.2018.09.27.14.59.54 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 27 Sep 2018 14:59:54 -0700 (PDT) Received: by mail-yw1-f49.google.com with SMTP id v1-v6so1768379ywv.6 for ; Thu, 27 Sep 2018 14:59:54 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20180927151119.9989-5-tycho@tycho.ws> References: <20180927151119.9989-1-tycho@tycho.ws> <20180927151119.9989-5-tycho@tycho.ws> From: Kees Cook Date: Thu, 27 Sep 2018 14:59:53 -0700 Message-ID: Subject: Re: [PATCH v7 4/6] files: add a replace_fd_files() function To: Tycho Andersen Cc: LKML , Linux Containers , Linux API , Andy Lutomirski , Oleg Nesterov , "Eric W . Biederman" , "Serge E . Hallyn" , Christian Brauner , Tyler Hicks , Akihiro Suda , Jann Horn , "linux-fsdevel@vger.kernel.org" , Alexander Viro Content-Type: text/plain; charset="UTF-8" Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu, Sep 27, 2018 at 8:11 AM, Tycho Andersen wrote: > Similar to fd_install/__fd_install, we want to be able to replace an fd of > an arbitrary struct files_struct, not just current's. We'll use this in the > next patch to implement the seccomp ioctl that allows inserting fds into a > stopped process' context. > > v7: new in v7 > > Signed-off-by: Tycho Andersen > CC: Alexander Viro > CC: Kees Cook > CC: Andy Lutomirski > CC: Oleg Nesterov > CC: Eric W. Biederman > CC: "Serge E. Hallyn" > CC: Christian Brauner > CC: Tyler Hicks > CC: Akihiro Suda > --- > fs/file.c | 22 +++++++++++++++------- > include/linux/file.h | 8 ++++++++ > 2 files changed, 23 insertions(+), 7 deletions(-) > > diff --git a/fs/file.c b/fs/file.c > index 7ffd6e9d103d..3b3c5aadaadb 100644 > --- a/fs/file.c > +++ b/fs/file.c > @@ -850,24 +850,32 @@ __releases(&files->file_lock) > } > > int replace_fd(unsigned fd, struct file *file, unsigned flags) > +{ > + return replace_fd_task(current, fd, file, flags); > +} > + > +/* > + * Same warning as __alloc_fd()/__fd_install() here. > + */ > +int replace_fd_task(struct task_struct *task, unsigned fd, > + struct file *file, unsigned flags) > { > int err; > - struct files_struct *files = current->files; Same feedback as Jann: on a purely "smaller diff" note, this could just be s/current/task/ here and all the other s/files/task->files/ would go away... > > if (!file) > - return __close_fd(files, fd); > + return __close_fd(task->files, fd); > > - if (fd >= rlimit(RLIMIT_NOFILE)) > + if (fd >= task_rlimit(task, RLIMIT_NOFILE)) > return -EBADF; > > - spin_lock(&files->file_lock); > - err = expand_files(files, fd); > + spin_lock(&task->files->file_lock); > + err = expand_files(task->files, fd); > if (unlikely(err < 0)) > goto out_unlock; > - return do_dup2(files, file, fd, flags); > + return do_dup2(task->files, file, fd, flags); > > out_unlock: > - spin_unlock(&files->file_lock); > + spin_unlock(&task->files->file_lock); > return err; > } > > diff --git a/include/linux/file.h b/include/linux/file.h > index 6b2fb032416c..f94277fee038 100644 > --- a/include/linux/file.h > +++ b/include/linux/file.h > @@ -11,6 +11,7 @@ > #include > > struct file; > +struct task_struct; > > extern void fput(struct file *); > > @@ -79,6 +80,13 @@ static inline void fdput_pos(struct fd f) > > extern int f_dupfd(unsigned int from, struct file *file, unsigned flags); > extern int replace_fd(unsigned fd, struct file *file, unsigned flags); > +/* > + * Warning! This is only safe if you know the owner of the files_struct is > + * stopped outside syscall context. It's a very bad idea to use this unless you > + * have similar guarantees in your code. > + */ > +extern int replace_fd_task(struct task_struct *task, unsigned fd, > + struct file *file, unsigned flags); Perhaps call this __replace_fd() to indicate the "please don't use this unless you're very sure"ness of it? > extern void set_close_on_exec(unsigned int fd, int flag); > extern bool get_close_on_exec(unsigned int fd); > extern int get_unused_fd_flags(unsigned flags); > -- > 2.17.1 > If I can get an Ack from Al, that would be very nice. :) -Kees -- Kees Cook Pixel Security