From 2ef4994c1ba970dd95c7b3d293e9053d4bda112a Mon Sep 17 00:00:00 2001 From: Daniel Mack Date: Mon, 20 Oct 2014 13:58:55 +0200 Subject: [PATCH] message, queue: pin files over their entire lifetime Make sure the passed fds and memfds are pinned throughout their usage in kdbus, that is, until they are installed. That closes a race gap in which a user could possibly replace an fd after submitting a message to the kernel and the message's delivery and the fd's installation. While at it, also move the seal check for memfds from queue.c to message.c and introduce a method to free an array of struct file*. Now, the incoming QA check in message.c will make sure the files are of the correct type, memfds are sealed etc. After that, when queue entry items are created, we call get_file() on each of the passed files to add increase the reference count once more, and decrement them when the entry is installed in the receiver's task. Also, the reference taken my the kmsg are dropped from kdbus_kmsg_free(). Signed-off-by: Daniel Mack --- message.c | 80 +++++++++++++++++++++++++---- message.h | 3 +- queue.c | 148 ++++++++---------------------------------------------- util.c | 23 +++++++++ util.h | 1 + 5 files changed, 117 insertions(+), 138 deletions(-) diff --git a/message.c b/message.c index 1782c07..98e92d4 100644 --- a/message.c +++ b/message.c @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -44,7 +45,11 @@ */ void kdbus_kmsg_free(struct kdbus_kmsg *kmsg) { + kdbus_fput_files(kmsg->memfds, kmsg->memfds_count); + kdbus_fput_files(kmsg->fds, kmsg->fds_count); kdbus_meta_free(kmsg->meta); + kfree(kmsg->memfds); + kfree(kmsg->fds); kfree(kmsg); } @@ -118,6 +123,27 @@ static int kdbus_msg_scan_items(struct kdbus_conn *conn, bool has_bloom = false; bool has_name = false; bool has_fds = false; + struct file *f; + + KDBUS_ITEMS_FOREACH(item, msg->items, KDBUS_ITEMS_SIZE(msg, items)) { + if (item->type == KDBUS_ITEM_PAYLOAD_MEMFD) { + /* do not allow to broadcast file descriptors */ + if (msg->dst_id == KDBUS_DST_ID_BROADCAST) + return -ENOTUNIQ; + + kmsg->memfds_count++; + } + } + + if (kmsg->memfds_count > 0) { + kmsg->memfds = kcalloc(kmsg->memfds_count, + sizeof(struct file *), GFP_KERNEL); + if (!kmsg->memfds) + return -ENOMEM; + + /* reset counter so we can reuse it */ + kmsg->memfds_count = 0; + } KDBUS_ITEMS_FOREACH(item, msg->items, KDBUS_ITEMS_SIZE(msg, items)) { size_t payload_size; @@ -144,16 +170,42 @@ static int kdbus_msg_scan_items(struct kdbus_conn *conn, kmsg->vecs_count++; break; - case KDBUS_ITEM_PAYLOAD_MEMFD: - /* do not allow to broadcast file descriptors */ - if (msg->dst_id == KDBUS_DST_ID_BROADCAST) - return -ENOTUNIQ; + case KDBUS_ITEM_PAYLOAD_MEMFD: { + int seals, mask; + f = fget(item->memfd.fd); + if (!f) + return -EBADF; + + kmsg->memfds[kmsg->memfds_count] = f; kmsg->memfds_count++; + + /* + * We only accept a sealed memfd file whose content + * cannot be altered by the sender or anybody else + * while it is shared or in-flight. Other files need + * to be passed with KDBUS_MSG_FDS. + */ + seals = shmem_get_seals(f); + if (seals < 0) + return -EMEDIUMTYPE; + + mask = F_SEAL_SHRINK | F_SEAL_GROW | F_SEAL_WRITE; + if ((seals & mask) != mask) + return -ETXTBSY; + + /* + * The specified size in the item cannot be larger + * than the backing file. + */ + if (item->memfd.size > i_size_read(file_inode(f))) + return -EBADF; + break; + } case KDBUS_ITEM_FDS: { - unsigned int i, n; + unsigned int n, i; /* do not allow multiple fd arrays */ if (has_fds) @@ -164,24 +216,30 @@ static int kdbus_msg_scan_items(struct kdbus_conn *conn, if (msg->dst_id == KDBUS_DST_ID_BROADCAST) return -ENOTUNIQ; - n = payload_size / sizeof(int); + n = KDBUS_ITEM_PAYLOAD_SIZE(item) / sizeof(int); if (n > KDBUS_MSG_MAX_FDS) return -EMFILE; + kmsg->fds = kcalloc(n, sizeof(struct file *), + GFP_KERNEL); + if (!kmsg->fds) + return -ENOMEM; + for (i = 0; i < n; i++) { - struct file *f; int ret; f = fget(item->fds[i]); - ret = kdbus_handle_check_file(f); - fput(f); + if (!f) + return -EBADF; + + kmsg->fds[i] = f; + kmsg->fds_count++; + ret = kdbus_handle_check_file(f); if (ret < 0) return ret; } - kmsg->fds = item->fds; - kmsg->fds_count = n; break; } diff --git a/message.h b/message.h index a12b6a7..2c85734 100644 --- a/message.h +++ b/message.h @@ -48,11 +48,12 @@ struct kdbus_kmsg { u64 dst_name_id; const struct kdbus_bloom_filter *bloom_filter; u64 bloom_generation; - const int *fds; + struct file **fds; unsigned int fds_count; struct kdbus_meta *meta; size_t vecs_size; unsigned int vecs_count; + struct file **memfds; unsigned int memfds_count; struct list_head queue_entry; diff --git a/queue.c b/queue.c index e480378..914dc82 100644 --- a/queue.c +++ b/queue.c @@ -24,7 +24,6 @@ #include #include #include -#include #include #include #include @@ -36,70 +35,6 @@ #include "util.h" #include "queue.h" -static void kdbus_queue_entry_fds_unref(struct kdbus_queue_entry *entry) -{ - unsigned int i; - - if (!entry->fds_fp) - return; - - for (i = 0; i < entry->fds_count; i++) { - if (!entry->fds_fp[i]) - break; - - fput(entry->fds_fp[i]); - } - - kfree(entry->fds_fp); - entry->fds_fp = NULL; - - entry->fds_count = 0; -} - -/* grab references of passed-in FDS for the queued message */ -static int kdbus_queue_entry_fds_ref(struct kdbus_queue_entry *entry, - const int *fds, unsigned int fds_count) -{ - unsigned int i; - - entry->fds_fp = kcalloc(fds_count, sizeof(struct file *), GFP_KERNEL); - if (!entry->fds_fp) - return -ENOMEM; - - for (i = 0; i < fds_count; i++) { - entry->fds_fp[i] = fget(fds[i]); - if (!entry->fds_fp[i]) { - kdbus_queue_entry_fds_unref(entry); - return -EBADF; - } - } - - return 0; -} - -static void kdbus_queue_entry_memfds_unref(struct kdbus_queue_entry *entry) -{ - unsigned int i; - - if (!entry->memfds_fp) - return; - - for (i = 0; i < entry->memfds_count; i++) { - if (!entry->memfds_fp[i]) - break; - - fput(entry->memfds_fp[i]); - } - - kfree(entry->memfds_fp); - entry->memfds_fp = NULL; - - kfree(entry->memfds); - entry->memfds = NULL; - - entry->memfds_count = 0; -} - static int kdbus_queue_entry_fds_install(struct kdbus_queue_entry *entry, int **ret_fds) { @@ -249,50 +184,6 @@ exit_rewind_memfds: return ret; } - - -/* Validate the state of the incoming PAYLOAD_MEMFD, and grab a reference - * to put it into the receiver's queue. */ -static int kdbus_conn_memfd_ref(const struct kdbus_item *item, - struct file **file) -{ - struct file *fp; - int seals, mask; - int ret; - - fp = fget(item->memfd.fd); - if (!fp) - return -EBADF; - - /* - * We only accept a sealed memfd file whose content cannot be altered - * by the sender or anybody else while it is shared or in-flight. - * Other files need to be passed with KDBUS_MSG_FDS. - */ - seals = shmem_get_seals(fp); - if (seals < 0) - return -EMEDIUMTYPE; - - mask = F_SEAL_SHRINK | F_SEAL_GROW | F_SEAL_WRITE; - if ((seals & mask) != mask) { - ret = -ETXTBSY; - goto exit_unref; - } - - /* The specified size in the item cannot be larger than the file. */ - if (item->memfd.size > i_size_read(file_inode(fp))) { - ret = -EBADF; - goto exit_unref; - } - - *file = fp; - return 0; - -exit_unref: - fput(fp); - return ret; -} - static int kdbus_queue_entry_payload_add(struct kdbus_queue_entry *entry, const struct kdbus_kmsg *kmsg, size_t items, size_t vec_data) @@ -302,7 +193,7 @@ static int kdbus_queue_entry_payload_add(struct kdbus_queue_entry *entry, if (kmsg->memfds_count > 0) { entry->memfds = kcalloc(kmsg->memfds_count, - sizeof(size_t), GFP_KERNEL); + sizeof(off_t), GFP_KERNEL); if (!entry->memfds) return -ENOMEM; @@ -373,8 +264,6 @@ static int kdbus_queue_entry_payload_add(struct kdbus_queue_entry *entry, char tmp[KDBUS_ITEM_HEADER_SIZE + sizeof(struct kdbus_memfd)]; struct kdbus_item *it = (struct kdbus_item *)tmp; - struct file *fp; - size_t memfd; /* add item */ it->type = KDBUS_ITEM_PAYLOAD_MEMFD; @@ -386,18 +275,14 @@ static int kdbus_queue_entry_payload_add(struct kdbus_queue_entry *entry, if (ret < 0) return ret; - /* grab reference of incoming file */ - ret = kdbus_conn_memfd_ref(item, &fp); - if (ret < 0) - return ret; - /* * Remember the file and the location of the fd number * which will be updated at RECV time. */ - memfd = items + offsetof(struct kdbus_item, memfd.fd); - entry->memfds[entry->memfds_count] = memfd; - entry->memfds_fp[entry->memfds_count] = fp; + entry->memfds[entry->memfds_count] = + items + offsetof(struct kdbus_item, memfd.fd); + entry->memfds_fp[entry->memfds_count] = + get_file(kmsg->memfds[entry->memfds_count]); entry->memfds_count++; items += KDBUS_ALIGN8(it->size); @@ -625,6 +510,11 @@ int kdbus_queue_entry_alloc(struct kdbus_conn *conn, /* space for FDS item */ if (kmsg->fds_count > 0) { + entry->fds_fp = kcalloc(kmsg->fds_count, sizeof(struct file *), + GFP_KERNEL); + if (!entry->fds_fp) + return -ENOMEM; + fds = msg_size; msg_size += KDBUS_ITEM_SIZE(kmsg->fds_count * sizeof(int)); } @@ -687,6 +577,7 @@ int kdbus_queue_entry_alloc(struct kdbus_conn *conn, /* add a FDS item; the array content will be updated at RECV time */ if (kmsg->fds_count > 0) { char tmp[KDBUS_ITEM_HEADER_SIZE]; + unsigned int i; it = (struct kdbus_item *)tmp; it->type = KDBUS_ITEM_FDS; @@ -697,10 +588,13 @@ int kdbus_queue_entry_alloc(struct kdbus_conn *conn, if (ret < 0) goto exit_pool_free; - ret = kdbus_queue_entry_fds_ref(entry, kmsg->fds, - kmsg->fds_count); - if (ret < 0) - goto exit_pool_free; + for (i = 0; i < kmsg->fds_count; i++) { + entry->fds_fp[i] = get_file(kmsg->fds[i]); + if (!entry->fds_fp[i]) { + ret = -EBADF; + goto exit_pool_free; + } + } /* remember the array to update at RECV */ entry->fds = fds + offsetof(struct kdbus_item, fds); @@ -736,8 +630,10 @@ exit: */ void kdbus_queue_entry_free(struct kdbus_queue_entry *entry) { - kdbus_queue_entry_memfds_unref(entry); - kdbus_queue_entry_fds_unref(entry); + kdbus_fput_files(entry->memfds_fp, entry->memfds_count); + kdbus_fput_files(entry->fds_fp, entry->fds_count); + kfree(entry->memfds_fp); + kfree(entry->fds_fp); kfree(entry); } diff --git a/util.c b/util.c index af4cd70..344598b 100644 --- a/util.c +++ b/util.c @@ -12,6 +12,7 @@ */ #include +#include #include #include @@ -82,3 +83,25 @@ int kdbus_negotiate_flags(u64 flags, void __user *buf, off_t offset, u64 valid) return 0; } + +/** + * kdbus_fput_files() - fput() an array of struct files + * @files: The array of files to put, may be NULL + * @count: The number of elements in @files + * + * Call fput() on all non-NULL elements in @files, and set the entries to + * NULL afterwards. + */ +void kdbus_fput_files(struct file **files, unsigned int count) +{ + unsigned int i; + + if (!files) + return; + + for (i = 0; i < count; i++) + if (files[i]) { + fput(files[i]); + files[i] = NULL; + } +} diff --git a/util.h b/util.h index 5bbfc6e..477f311 100644 --- a/util.h +++ b/util.h @@ -84,5 +84,6 @@ static inline bool kdbus_str_valid(const char *str, size_t size) int kdbus_sysname_is_valid(const char *name); int kdbus_negotiate_flags(u64 flags, void __user *buf, off_t offset, u64 valid); +void kdbus_fput_files(struct file **files, unsigned int count); #endif -- 2.34.1