On Tue, Jun 02, 2020 at 06:10:41PM -0700, Sargun Dhillon wrote:
Previously there were two chunks of code where the logic to receive file descriptors was duplicated in net. The compat version of copying file descriptors via SCM_RIGHTS did not have logic to update cgroups. Logic to change the cgroup data was added in: commit 48a87cc26c13 ("net: netprio: fd passed in SCM_RIGHTS datagram not set correctly") commit d84295067fc7 ("net: net_cls: fd passed in SCM_RIGHTS datagram not set correctly")
This was not copied to the compat path. This commit fixes that, and thus should be cherry-picked into stable.
This introduces a helper (file_receive) which encapsulates the logic for handling calling security hooks as well as manipulating cgroup information. This helper can then be used other places in the kernel where file descriptors are copied between processes
I tested cgroup classid setting on both the compat (x32) path, and the native path to ensure that when moving the file descriptor the classid is set.
Signed-off-by: Sargun Dhillon sargun@sargun.me Suggested-by: Kees Cook keescook@chromium.org Cc: Al Viro viro@zeniv.linux.org.uk Cc: Christian Brauner christian.brauner@ubuntu.com Cc: Daniel Wagner daniel.wagner@bmw-carit.de Cc: David S. Miller davem@davemloft.net Cc: Jann Horn jannh@google.com, Cc: John Fastabend john.r.fastabend@intel.com Cc: Tejun Heo tj@kernel.org Cc: Tycho Andersen tycho@tycho.ws Cc: stable@vger.kernel.org Cc: cgroups@vger.kernel.org Cc: linux-fsdevel@vger.kernel.org Cc: linux-kernel@vger.kernel.org
fs/file.c | 35 +++++++++++++++++++++++++++++++++++ include/linux/file.h | 1 + net/compat.c | 10 +++++----- net/core/scm.c | 14 ++++---------- 4 files changed, 45 insertions(+), 15 deletions(-)
diff --git a/fs/file.c b/fs/file.c index abb8b7081d7a..5afd76fca8c2 100644 --- a/fs/file.c +++ b/fs/file.c @@ -18,6 +18,9 @@ #include <linux/bitops.h> #include <linux/spinlock.h> #include <linux/rcupdate.h> +#include <net/sock.h> +#include <net/netprio_cgroup.h> +#include <net/cls_cgroup.h> unsigned int sysctl_nr_open __read_mostly = 1024*1024; unsigned int sysctl_nr_open_min = BITS_PER_LONG; @@ -931,6 +934,38 @@ int replace_fd(unsigned fd, struct file *file, unsigned flags) return err; } +/*
- File Receive - Receive a file from another process
- This function is designed to receive files from other tasks. It encapsulates
- logic around security and cgroups. The file descriptor provided must be a
- freshly allocated (unused) file descriptor.
- This helper does not consume a reference to the file, so the caller must put
- their reference.
- Returns 0 upon success.
- */
+int file_receive(int fd, struct file *file)
This is all just a remote version of fd_install(), yet it deviates from fd_install()'s semantics and naming. That's not great imho. What about naming this something like:
fd_install_received()
and move the get_file() out of there so it has the same semantics as fd_install(). It seems rather dangerous to have a function like fd_install() that consumes a reference once it returned and another version of this that is basically the same thing but doesn't consume a reference because it takes its own. Seems an invitation for confusion. Does that make sense?
+{
- struct socket *sock;
- int err;
- err = security_file_receive(file);
- if (err)
return err;- fd_install(fd, get_file(file));
- sock = sock_from_file(file, &err);
- if (sock) {
sock_update_netprioidx(&sock->sk->sk_cgrp_data);sock_update_classid(&sock->sk->sk_cgrp_data);- }
- return 0;
+}
static int ksys_dup3(unsigned int oldfd, unsigned int newfd, int flags) { int err = -EBADF; diff --git a/include/linux/file.h b/include/linux/file.h index 142d102f285e..7b56dc23e560 100644 --- a/include/linux/file.h +++ b/include/linux/file.h @@ -94,4 +94,5 @@ extern void fd_install(unsigned int fd, struct file *file); extern void flush_delayed_fput(void); extern void __fput_sync(struct file *); +extern int file_receive(int fd, struct file *file); #endif /* __LINUX_FILE_H */ diff --git a/net/compat.c b/net/compat.c index 4bed96e84d9a..8ac0e7e09208 100644 --- a/net/compat.c +++ b/net/compat.c @@ -293,9 +293,6 @@ void scm_detach_fds_compat(struct msghdr *kmsg, struct scm_cookie *scm) for (i = 0, cmfptr = (int __user *) CMSG_COMPAT_DATA(cm); i < fdmax; i++, cmfptr++) { int new_fd;
err = security_file_receive(fp[i]);if (err)break;@@ -306,8 +303,11 @@ void scm_detach_fds_compat(struct msghdr *kmsg, struct scm_cookie *scm) put_unused_fd(new_fd); break; }
/* Bump the usage count and install the file. */fd_install(new_fd, get_file(fp[i]));
err = file_receive(new_fd, fp[i]);if (err) {put_unused_fd(new_fd);break;}if (i > 0) { diff --git a/net/core/scm.c b/net/core/scm.c index dc6fed1f221c..ba93abf2881b 100644 --- a/net/core/scm.c +++ b/net/core/scm.c @@ -303,11 +303,7 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm) for (i=0, cmfptr=(__force int __user *)CMSG_DATA(cm); i<fdmax; i++, cmfptr++) {
struct socket *sock;err = security_file_receive(fp[i]);if (err)break;@@ -318,13 +314,11 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm) put_unused_fd(new_fd); break; }
/* Bump the usage count and install the file. */sock = sock_from_file(fp[i], &err);if (sock) {sock_update_netprioidx(&sock->sk->sk_cgrp_data);sock_update_classid(&sock->sk->sk_cgrp_data);
err = file_receive(new_fd, fp[i]);if (err) {put_unused_fd(new_fd);break;
fd_install(new_fd, get_file(fp[i]));if (i > 0) -- 2.25.1