| Message ID | 20200617220327.3731559-4-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add seccomp notifier ioctl that enables adding fds | expand |
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>
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!
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)
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; }
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(-)