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 <daniel@zonque.org>

diff --git a/message.c b/message.c
index 1782c071284c84d81e3c6805b4b5220fe4a2e5f2..98e92d4121b480d448ce2231df493044361a8407 100644 (file)
--- a/message.c
+++ b/message.c
@@ -20,6 +20,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/sched.h>
+#include <linux/shmem_fs.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>
 #include <linux/uaccess.h>
@@ -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 a12b6a793c0a05e0c5dbfd63888311a345396cc0..2c8573423d4f548e8384c24c0b46e914b50e68b9 100644 (file)
--- 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 e480378f1f4469c20647d1db73157a9df3a57dde..914dc8292ec56f226c743a822603eb10966f48ee 100644 (file)
--- a/queue.c
+++ b/queue.c
@@ -24,7 +24,6 @@
 #include <linux/mutex.h>
 #include <linux/poll.h>
 #include <linux/sched.h>
-#include <linux/shmem_fs.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>
 #include <linux/syscalls.h>
@@ -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 af4cd7026f26e8b276557259ad4b3dbf8b8decd8..344598b6b23fd18bc90769c91ae45b6264e87521 100644 (file)
--- a/util.c
+++ b/util.c
@@ -12,6 +12,7 @@
  */
 
 #include <linux/ctype.h>
+#include <linux/file.h>
 #include <linux/string.h>
 #include <linux/uaccess.h>
 
@@ -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 5bbfc6ed33642fc468c9e394da49fc613e6f9926..477f31174c8d9b4021f11668eccfdc4837323644 100644 (file)
--- 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