| Message ID | 20200706201720.3482959-4-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add seccomp notifier ioctl that enables adding fds | expand |
On Mon, Jul 06, 2020 at 01:17:16PM -0700, Kees Cook wrote: > For both pidfd and seccomp, the __user pointer is not used. Update > __receive_fd() to make writing to ufd optional via a NULL check. However, > for the receive_fd_user() wrapper, ufd is NULL checked so an -EFAULT > can be returned to avoid changing the SCM_RIGHTS interface behavior. Add > new wrapper receive_fd() for pidfd and seccomp that does not use the ufd > argument. For the new helper, the allocated fd needs to be returned on > success. Update the existing callers to handle it. Oh here we go, I'll take the last comment back.
On Mon, Jul 06, 2020 at 01:17:16PM -0700, Kees Cook wrote: > For both pidfd and seccomp, the __user pointer is not used. Update > __receive_fd() to make writing to ufd optional via a NULL check. However, > for the receive_fd_user() wrapper, ufd is NULL checked so an -EFAULT > can be returned to avoid changing the SCM_RIGHTS interface behavior. Add > new wrapper receive_fd() for pidfd and seccomp that does not use the ufd > argument. For the new helper, the allocated fd needs to be returned on > success. Update the existing callers to handle it. > > Reviewed-by: Sargun Dhillon <sargun@sargun.me> > Signed-off-by: Kees Cook <keescook@chromium.org> > --- Hm, I'm not sure why 2/7 and 3/7 aren't just one patch but ok. :) Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
On Tue, Jul 07, 2020 at 01:49:23PM +0200, Christian Brauner wrote: > On Mon, Jul 06, 2020 at 01:17:16PM -0700, Kees Cook wrote: > > For both pidfd and seccomp, the __user pointer is not used. Update > > __receive_fd() to make writing to ufd optional via a NULL check. However, > > for the receive_fd_user() wrapper, ufd is NULL checked so an -EFAULT > > can be returned to avoid changing the SCM_RIGHTS interface behavior. Add > > new wrapper receive_fd() for pidfd and seccomp that does not use the ufd > > argument. For the new helper, the allocated fd needs to be returned on > > success. Update the existing callers to handle it. > > > > Reviewed-by: Sargun Dhillon <sargun@sargun.me> > > Signed-off-by: Kees Cook <keescook@chromium.org> > > --- > > Hm, I'm not sure why 2/7 and 3/7 aren't just one patch but ok. :) I wanted to do a "clean" move from one source to another without any behavioral changes first. > Acked-by: Christian Brauner <christian.brauner@ubuntu.com> Thanks!
diff --git a/fs/file.c b/fs/file.c index 3bd67233088b..0efdcf413210 100644 --- a/fs/file.c +++ b/fs/file.c @@ -942,12 +942,13 @@ 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. * * This helper handles its own reference counting of the incoming * struct file. * - * Returns -ve on error. + * Returns newly install fd or -ve on error. */ int __receive_fd(struct file *file, int __user *ufd, unsigned int o_flags) { @@ -963,20 +964,26 @@ int __receive_fd(struct file *file, int __user *ufd, unsigned int o_flags) 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 b14ff2ffd0bd..d9fee9f5c8da 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 __receive_fd(struct file *file, int __user *ufd, static inline int receive_fd_user(struct file *file, int __user *ufd, unsigned int o_flags) { + if (ufd == NULL) + return -EFAULT; return __receive_fd(file, ufd, o_flags); } +static inline int receive_fd(struct file *file, unsigned int o_flags) +{ + return __receive_fd(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 e74cd3dae8b0..dc7ddbc2b15e 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 = receive_fd_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 67c166a7820d..8156d4fb8a39 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 = receive_fd_user(scm->fp->fp[i], cmsg_data + i, o_flags); - if (err) + if (err < 0) break; }