[v5,3/7] fs: Add fd_install_received() wrapper for __fd_install_received()
diff mbox series

Message ID 20200617220327.3731559-4-keescook@chromium.org
State In Next
Commit 4ab6bcc3ad3b25fd0383f4bfe999142dc876e432
Headers show
Series
  • Add seccomp notifier ioctl that enables adding fds
Related show

Commit Message

Kees Cook June 17, 2020, 10:03 p.m. UTC
For both pidfd and seccomp, the __user pointer is not used. Update
__fd_install_received() to make writing to ufd optional via a NULL check.
However, for the fd_install_received_user() wrapper, ufd is NULL checked
so an -EFAULT can be returned to avoid changing the SCM_RIGHTS interface
behavior. Add new wrapper fd_install_received() for pidfd and seccomp
that does not use the ufd argument. For the new helper, the new fd needs
to be returned on success. Update the existing callers to handle it.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/file.c            | 22 ++++++++++++++--------
 include/linux/file.h |  7 +++++++
 net/compat.c         |  2 +-
 net/core/scm.c       |  2 +-
 4 files changed, 23 insertions(+), 10 deletions(-)

Comments

Sargun Dhillon June 18, 2020, 5:49 a.m. UTC | #1
On Wed, Jun 17, 2020 at 03:03:23PM -0700, Kees Cook wrote:
> For both pidfd and seccomp, the __user pointer is not used. Update
> __fd_install_received() to make writing to ufd optional via a NULL check.
> However, for the fd_install_received_user() wrapper, ufd is NULL checked
> so an -EFAULT can be returned to avoid changing the SCM_RIGHTS interface
> behavior. Add new wrapper fd_install_received() for pidfd and seccomp
> that does not use the ufd argument. For the new helper, the new fd needs
> to be returned on success. Update the existing callers to handle it.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  fs/file.c            | 22 ++++++++++++++--------
>  include/linux/file.h |  7 +++++++
>  net/compat.c         |  2 +-
>  net/core/scm.c       |  2 +-
>  4 files changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/file.c b/fs/file.c
> index f2167d6feec6..de85a42defe2 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -942,9 +942,10 @@ int replace_fd(unsigned fd, struct file *file, unsigned flags)
>   * @o_flags: the O_* flags to apply to the new fd entry
>   *
>   * Installs a received file into the file descriptor table, with appropriate
> - * checks and count updates. Writes the fd number to userspace.
> + * checks and count updates. Optionally writes the fd number to userspace, if
> + * @ufd is non-NULL.
>   *
> - * Returns -ve on error.
> + * Returns newly install fd or -ve on error.
>   */
>  int __fd_install_received(struct file *file, int __user *ufd, unsigned int o_flags)
>  {
> @@ -960,20 +961,25 @@ int __fd_install_received(struct file *file, int __user *ufd, unsigned int o_fla
>  	if (new_fd < 0)
>  		return new_fd;
>  
> -	error = put_user(new_fd, ufd);
> -	if (error) {
> -		put_unused_fd(new_fd);
> -		return error;
> +	if (ufd) {
> +		error = put_user(new_fd, ufd);
> +		if (error) {
> +			put_unused_fd(new_fd);
> +			return error;
> +		}
>  	}
>  
> -	/* Bump the usage count and install the file. */
> +	/* Bump the usage count and install the file. The resulting value of
> +	 * "error" is ignored here since we only need to take action when
> +	 * the file is a socket and testing "sock" for NULL is sufficient.
> +	 */
>  	sock = sock_from_file(file, &error);
>  	if (sock) {
>  		sock_update_netprioidx(&sock->sk->sk_cgrp_data);
>  		sock_update_classid(&sock->sk->sk_cgrp_data);
>  	}
>  	fd_install(new_fd, get_file(file));
> -	return 0;
> +	return new_fd;
>  }
>  
>  static int ksys_dup3(unsigned int oldfd, unsigned int newfd, int flags)
> diff --git a/include/linux/file.h b/include/linux/file.h
> index fe18a1a0d555..e19974ed9322 100644
> --- a/include/linux/file.h
> +++ b/include/linux/file.h
> @@ -9,6 +9,7 @@
>  #include <linux/compiler.h>
>  #include <linux/types.h>
>  #include <linux/posix_types.h>
> +#include <linux/errno.h>
>  
>  struct file;
>  
> @@ -96,8 +97,14 @@ extern int __fd_install_received(struct file *file, int __user *ufd,
>  static inline int fd_install_received_user(struct file *file, int __user *ufd,
>  					   unsigned int o_flags)
>  {
> +	if (ufd == NULL)
> +		return -EFAULT;
Isn't this *technically* a behvaiour change? Nonetheless, I think this is a much better
approach than forcing everyone to do null checking, and avoids at least one error case
where the kernel installs FDs for SCM_RIGHTS, and they're not actualy usable.

>  	return __fd_install_received(file, ufd, o_flags);
>  }
> +static inline int fd_install_received(struct file *file, unsigned int o_flags)
> +{
> +	return __fd_install_received(file, NULL, o_flags);
> +}
>  
>  extern void flush_delayed_fput(void);
>  extern void __fput_sync(struct file *);
> diff --git a/net/compat.c b/net/compat.c
> index 94f288e8dac5..71494337cca7 100644
> --- a/net/compat.c
> +++ b/net/compat.c
> @@ -299,7 +299,7 @@ void scm_detach_fds_compat(struct msghdr *msg, struct scm_cookie *scm)
>  
>  	for (i = 0; i < fdmax; i++) {
>  		err = fd_install_received_user(scm->fp->fp[i], cmsg_data + i, o_flags);
> -		if (err)
> +		if (err < 0)
>  			break;
>  	}
>  
> diff --git a/net/core/scm.c b/net/core/scm.c
> index df190f1fdd28..b9a0442ebd26 100644
> --- a/net/core/scm.c
> +++ b/net/core/scm.c
> @@ -307,7 +307,7 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
>  
>  	for (i = 0; i < fdmax; i++) {
>  		err = fd_install_received_user(scm->fp->fp[i], cmsg_data + i, o_flags);
> -		if (err)
> +		if (err < 0)
>  			break;
>  	}
>  
> -- 
> 2.25.1
> 

Reviewed-by: Sargun Dhillon <sargun@sargun.me>
Kees Cook June 18, 2020, 8:13 p.m. UTC | #2
On Thu, Jun 18, 2020 at 05:49:19AM +0000, Sargun Dhillon wrote:
> On Wed, Jun 17, 2020 at 03:03:23PM -0700, Kees Cook wrote:
> > [...]
> >  static inline int fd_install_received_user(struct file *file, int __user *ufd,
> >  					   unsigned int o_flags)
> >  {
> > +	if (ufd == NULL)
> > +		return -EFAULT;
> Isn't this *technically* a behvaiour change? Nonetheless, I think this is a much better
> approach than forcing everyone to do null checking, and avoids at least one error case
> where the kernel installs FDs for SCM_RIGHTS, and they're not actualy usable.

So, the only behavior change I see is that the order of sanity checks is
changed.

The loop in scm_detach_fds() is:


        for (i = 0; i < fdmax; i++) {
                err = __scm_install_fd(scm->fp->fp[i], cmsg_data + i, o_flags);
                if (err < 0)
                        break;
        }

Before, __scm_install_fd() does:

        error = security_file_receive(file);
        if (error)
                return error;

        new_fd = get_unused_fd_flags(o_flags);
        if (new_fd < 0)
                return new_fd;

        error = put_user(new_fd, ufd);
        if (error) {
                put_unused_fd(new_fd);
                return error;
        }
	...

After, fd_install_received_user() and __fd_install_received() does:

        if (ufd == NULL)
                return -EFAULT;
	...
        error = security_file_receive(file);
        if (error)
                return error;
	...
                new_fd = get_unused_fd_flags(o_flags);
                if (new_fd < 0)
                        return new_fd;
	...
                error = put_user(new_fd, ufd);
                if (error) {
                        put_unused_fd(new_fd);
                        return error;
                }

i.e. if a caller attempts a receive that is rejected by LSM *and*
includes a NULL userpointer destination, they will get an EFAULT now
instead of an EPERM.

I struggle to imagine a situation where this could possible matter
(both fail, neither installs files). It is only the error code that
is different. I am comfortable making this change and seeing if anyone
screams. If they do, I can restore the v4 "ufd_required" way of doing it.

> Reviewed-by: Sargun Dhillon <sargun@sargun.me>

Thanks!
David Laight June 19, 2020, 8:20 a.m. UTC | #3
From: Kees Cook
> Sent: 18 June 2020 21:13
> On Thu, Jun 18, 2020 at 05:49:19AM +0000, Sargun Dhillon wrote:
> > On Wed, Jun 17, 2020 at 03:03:23PM -0700, Kees Cook wrote:
> > > [...]
> > >  static inline int fd_install_received_user(struct file *file, int __user *ufd,
> > >  					   unsigned int o_flags)
> > >  {
> > > +	if (ufd == NULL)
> > > +		return -EFAULT;
> > Isn't this *technically* a behvaiour change? Nonetheless, I think this is a much better
> > approach than forcing everyone to do null checking, and avoids at least one error case
> > where the kernel installs FDs for SCM_RIGHTS, and they're not actualy usable.
> 
> So, the only behavior change I see is that the order of sanity checks is
> changed.
> 
> The loop in scm_detach_fds() is:
> 
> 
>         for (i = 0; i < fdmax; i++) {
>                 err = __scm_install_fd(scm->fp->fp[i], cmsg_data + i, o_flags);
>                 if (err < 0)
>                         break;
>         }
> 
> Before, __scm_install_fd() does:
> 
>         error = security_file_receive(file);
>         if (error)
>                 return error;
> 
>         new_fd = get_unused_fd_flags(o_flags);
>         if (new_fd < 0)
>                 return new_fd;
> 
>         error = put_user(new_fd, ufd);
>         if (error) {
>                 put_unused_fd(new_fd);
>                 return error;
>         }
> 	...
> 
> After, fd_install_received_user() and __fd_install_received() does:
> 
>         if (ufd == NULL)
>                 return -EFAULT;
> 	...
>         error = security_file_receive(file);
>         if (error)
>                 return error;
> 	...
>                 new_fd = get_unused_fd_flags(o_flags);
>                 if (new_fd < 0)
>                         return new_fd;
> 	...
>                 error = put_user(new_fd, ufd);
>                 if (error) {
>                         put_unused_fd(new_fd);
>                         return error;
>                 }
> 
> i.e. if a caller attempts a receive that is rejected by LSM *and*
> includes a NULL userpointer destination, they will get an EFAULT now
> instead of an EPERM.

The 'user' pointer the fd is written to is in the middle of
the 'cmsg' buffer.
So to hit 'ufd == NULL' the program would have to pass a small
negative integer!

The error paths are strange if there are multiple fd in the message.
A quick look at the old code seems to imply that if the user doesn't
supply a big enough buffer then the extra 'file *' just get closed.
OTOH if there is an error processing one of the files the request
fails with the earlier file allocated fd numbers.

In addition most of the userspace buffer is written after the
loop - any errors there return -EFAULT (SIGSEGV) without
even trying to tidy up the allocated fd.

ISTM that the put_user(new_fd, ufd) could be done in __scm_install_fd()
after __fd_install_received() returns.

scm_detach_fds() could do the put_user(SOL_SOCKET,...) before actually
processing the first file - so that the state can be left unchanged
when a naff buffer is passed.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Patch
diff mbox series

diff --git a/fs/file.c b/fs/file.c
index f2167d6feec6..de85a42defe2 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -942,9 +942,10 @@  int replace_fd(unsigned fd, struct file *file, unsigned flags)
  * @o_flags: the O_* flags to apply to the new fd entry
  *
  * Installs a received file into the file descriptor table, with appropriate
- * checks and count updates. Writes the fd number to userspace.
+ * checks and count updates. Optionally writes the fd number to userspace, if
+ * @ufd is non-NULL.
  *
- * Returns -ve on error.
+ * Returns newly install fd or -ve on error.
  */
 int __fd_install_received(struct file *file, int __user *ufd, unsigned int o_flags)
 {
@@ -960,20 +961,25 @@  int __fd_install_received(struct file *file, int __user *ufd, unsigned int o_fla
 	if (new_fd < 0)
 		return new_fd;
 
-	error = put_user(new_fd, ufd);
-	if (error) {
-		put_unused_fd(new_fd);
-		return error;
+	if (ufd) {
+		error = put_user(new_fd, ufd);
+		if (error) {
+			put_unused_fd(new_fd);
+			return error;
+		}
 	}
 
-	/* Bump the usage count and install the file. */
+	/* Bump the usage count and install the file. The resulting value of
+	 * "error" is ignored here since we only need to take action when
+	 * the file is a socket and testing "sock" for NULL is sufficient.
+	 */
 	sock = sock_from_file(file, &error);
 	if (sock) {
 		sock_update_netprioidx(&sock->sk->sk_cgrp_data);
 		sock_update_classid(&sock->sk->sk_cgrp_data);
 	}
 	fd_install(new_fd, get_file(file));
-	return 0;
+	return new_fd;
 }
 
 static int ksys_dup3(unsigned int oldfd, unsigned int newfd, int flags)
diff --git a/include/linux/file.h b/include/linux/file.h
index fe18a1a0d555..e19974ed9322 100644
--- a/include/linux/file.h
+++ b/include/linux/file.h
@@ -9,6 +9,7 @@ 
 #include <linux/compiler.h>
 #include <linux/types.h>
 #include <linux/posix_types.h>
+#include <linux/errno.h>
 
 struct file;
 
@@ -96,8 +97,14 @@  extern int __fd_install_received(struct file *file, int __user *ufd,
 static inline int fd_install_received_user(struct file *file, int __user *ufd,
 					   unsigned int o_flags)
 {
+	if (ufd == NULL)
+		return -EFAULT;
 	return __fd_install_received(file, ufd, o_flags);
 }
+static inline int fd_install_received(struct file *file, unsigned int o_flags)
+{
+	return __fd_install_received(file, NULL, o_flags);
+}
 
 extern void flush_delayed_fput(void);
 extern void __fput_sync(struct file *);
diff --git a/net/compat.c b/net/compat.c
index 94f288e8dac5..71494337cca7 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -299,7 +299,7 @@  void scm_detach_fds_compat(struct msghdr *msg, struct scm_cookie *scm)
 
 	for (i = 0; i < fdmax; i++) {
 		err = fd_install_received_user(scm->fp->fp[i], cmsg_data + i, o_flags);
-		if (err)
+		if (err < 0)
 			break;
 	}
 
diff --git a/net/core/scm.c b/net/core/scm.c
index df190f1fdd28..b9a0442ebd26 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -307,7 +307,7 @@  void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
 
 	for (i = 0; i < fdmax; i++) {
 		err = fd_install_received_user(scm->fp->fp[i], cmsg_data + i, o_flags);
-		if (err)
+		if (err < 0)
 			break;
 	}