| Message ID | 20200616032524.460144-4-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add seccomp notifier ioctl that enables adding fds | expand |
From: Kees Cook > Sent: 16 June 2020 04:25 > > For both pidfd and seccomp, the __user pointer is not used. Update > __fd_install_received() to make writing to ufd optional. (ufd > itself cannot checked for NULL because this changes the SCM_RIGHTS > interface behavior.) In these cases, the new fd needs to be returned > on success. Update the existing callers to handle it. Add new wrapper > fd_install_received() for pidfd and seccomp that does not use the ufd > argument. ...> > static inline int fd_install_received_user(struct file *file, int __user *ufd, > unsigned int o_flags) > { > - return __fd_install_received(file, ufd, o_flags); > + return __fd_install_received(file, true, ufd, o_flags); > +} Can you get rid of the 'return user' parameter by adding if (!ufd) return -EFAULT; to the above wrapper, then checking for NULL in the function? Or does that do the wrong horrid things in the fail path? David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Wed, Jun 17, 2020 at 03:35:20PM +0000, David Laight wrote: > From: Kees Cook > > Sent: 16 June 2020 04:25 > > > > For both pidfd and seccomp, the __user pointer is not used. Update > > __fd_install_received() to make writing to ufd optional. (ufd > > itself cannot checked for NULL because this changes the SCM_RIGHTS > > interface behavior.) In these cases, the new fd needs to be returned > > on success. Update the existing callers to handle it. Add new wrapper > > fd_install_received() for pidfd and seccomp that does not use the ufd > > argument. > ...> > > static inline int fd_install_received_user(struct file *file, int __user *ufd, > > unsigned int o_flags) > > { > > - return __fd_install_received(file, ufd, o_flags); > > + return __fd_install_received(file, true, ufd, o_flags); > > +} > > Can you get rid of the 'return user' parameter by adding > if (!ufd) return -EFAULT; > to the above wrapper, then checking for NULL in the function? > > Or does that do the wrong horrid things in the fail path? Oh, hm. No, that shouldn't break the failure path, since everything gets unwound in __fd_install_received if the ufd write fails. Effectively this (I'll chop it up into the correct patches): diff --git a/fs/file.c b/fs/file.c index b583e7c60571..3b80324a31cc 100644 --- a/fs/file.c +++ b/fs/file.c @@ -939,18 +939,16 @@ int replace_fd(unsigned fd, struct file *file, unsigned flags) * * @fd: fd to install into (if negative, a new fd will be allocated) * @file: struct file that was received from another process - * @ufd_required: true to use @ufd for writing fd number to userspace * @ufd: __user pointer to write new fd number to * @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. Optionally writes the fd number to userspace, if - * @ufd_required is true (@ufd cannot just be tested for NULL because NULL may - * actually get passed into SCM_RIGHTS). + * @ufd is non-NULL. * * Returns newly install fd or -ve on error. */ -int __fd_install_received(int fd, struct file *file, bool ufd_required, +int __fd_install_received(int fd, struct file *file, int __user *ufd, unsigned int o_flags) { struct socket *sock; @@ -967,7 +965,7 @@ int __fd_install_received(int fd, struct file *file, bool ufd_required, return new_fd; } - if (ufd_required) { + if (ufd) { error = put_user(new_fd, ufd); if (error) { put_unused_fd(new_fd); diff --git a/include/linux/file.h b/include/linux/file.h index f1d16e24a12e..2ade0d90bc5e 100644 --- a/include/linux/file.h +++ b/include/linux/file.h @@ -91,20 +91,22 @@ extern void put_unused_fd(unsigned int fd); extern void fd_install(unsigned int fd, struct file *file); -extern int __fd_install_received(int fd, struct file *file, bool ufd_required, +extern int __fd_install_received(int fd, struct file *file, int __user *ufd, unsigned int o_flags); static inline int fd_install_received_user(struct file *file, int __user *ufd, unsigned int o_flags) { - return __fd_install_received(-1, file, true, ufd, o_flags); + if (ufd == NULL) + return -EFAULT; + return __fd_install_received(-1, file, ufd, o_flags); } static inline int fd_install_received(struct file *file, unsigned int o_flags) { - return __fd_install_received(-1, file, false, NULL, o_flags); + return __fd_install_received(-1, file, NULL, o_flags); } static inline int fd_replace_received(int fd, struct file *file, unsigned int o_flags) { - return __fd_install_received(fd, file, false, NULL, o_flags); + return __fd_install_received(fd, file, NULL, o_flags); } extern void flush_delayed_fput(void);
From: Kees Cook > Sent: 17 June 2020 20:58 > On Wed, Jun 17, 2020 at 03:35:20PM +0000, David Laight wrote: > > From: Kees Cook > > > Sent: 16 June 2020 04:25 > > > > > > For both pidfd and seccomp, the __user pointer is not used. Update > > > __fd_install_received() to make writing to ufd optional. (ufd > > > itself cannot checked for NULL because this changes the SCM_RIGHTS > > > interface behavior.) In these cases, the new fd needs to be returned > > > on success. Update the existing callers to handle it. Add new wrapper > > > fd_install_received() for pidfd and seccomp that does not use the ufd > > > argument. > > ...> > > > static inline int fd_install_received_user(struct file *file, int __user *ufd, > > > unsigned int o_flags) > > > { > > > - return __fd_install_received(file, ufd, o_flags); > > > + return __fd_install_received(file, true, ufd, o_flags); > > > +} > > > > Can you get rid of the 'return user' parameter by adding > > if (!ufd) return -EFAULT; > > to the above wrapper, then checking for NULL in the function? > > > > Or does that do the wrong horrid things in the fail path? > > Oh, hm. No, that shouldn't break the failure path, since everything gets > unwound in __fd_install_received if the ufd write fails. > > Effectively this (I'll chop it up into the correct patches): Yep, that's what i was thinking... Personally I'm not sure that it matters whether the file is left attached to a process fd when the copy_to_user() fails. The kernel data structures are consistent either way. So sane code relies on catching SIGSEGV, fixing thigs up, and carrying on. (IIRC the original /bin/sh code called sbrk() in its SIGSEGV handler instead of doing the limit check in malloc()!) The important error path is 'failing to get an fd number'. In that case the caller needs to keep the 'file *' or close it. I've not looked at the code, but I wonder if you need to pass the 'file *' by reference so that you can consume it (write NULL) and return an error. 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 fcfddae0d252..14a8ef74efb2 100644 --- a/fs/file.c +++ b/fs/file.c @@ -944,11 +944,14 @@ 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. Optionally writes the fd number to userspace. + * checks and count updates. Optionally writes the fd number to userspace, if + * @ufd_required is true (@ufd cannot just be tested for NULL because NULL may + * actually get passed into SCM_RIGHTS). * - * 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) +int __fd_install_received(struct file *file, bool ufd_required, int __user *ufd, + unsigned int o_flags) { struct socket *sock; int new_fd; @@ -962,20 +965,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_required) { + 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..999a2c56db07 100644 --- a/include/linux/file.h +++ b/include/linux/file.h @@ -91,12 +91,16 @@ extern void put_unused_fd(unsigned int fd); extern void fd_install(unsigned int fd, struct file *file); -extern int __fd_install_received(struct file *file, int __user *ufd, - unsigned int o_flags); +extern int __fd_install_received(struct file *file, bool ufd_required, + int __user *ufd, unsigned int o_flags); static inline int fd_install_received_user(struct file *file, int __user *ufd, unsigned int o_flags) { - return __fd_install_received(file, ufd, o_flags); + return __fd_install_received(file, true, ufd, o_flags); +} +static inline int fd_install_received(struct file *file, unsigned int o_flags) +{ + return __fd_install_received(file, false, NULL, o_flags); } extern void flush_delayed_fput(void); 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. (ufd itself cannot checked for NULL because this changes the SCM_RIGHTS interface behavior.) In these cases, the new fd needs to be returned on success. Update the existing callers to handle it. Add new wrapper fd_install_received() for pidfd and seccomp that does not use the ufd argument. Signed-off-by: Kees Cook <keescook@chromium.org> --- fs/file.c | 26 +++++++++++++++++--------- include/linux/file.h | 10 +++++++--- net/compat.c | 2 +- net/core/scm.c | 2 +- 4 files changed, 26 insertions(+), 14 deletions(-)