[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Xen-devel] [PATCH] Prevent unnatural use of ioctl in /proc/xen/xenbus_dev



Signed-off-by: Rusty Russell <rusty@xxxxxxxxxxxxxxx>

# HG changeset patch
# User Rusty Russell <rusty@xxxxxxxxxxxxxxx>
# Node ID df3759bbb309f6d01d6087af06ba4297a5169538
# Parent  f0d728001aaad4eb6c716cbdbb5d1f8a8a5f1620
xenbus_dev's use of ioctl for read/write is a crime against nature.
Make it a read-write interface, but check boundaries so we can recover if 
userspace dies.  This also simplifies libxenstore.

diff -r f0d728001aaa -r df3759bbb309 
linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_dev.c
--- a/linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_dev.c      Wed Sep  7 
23:11:44 2005
+++ b/linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_dev.c      Fri Sep  9 
11:07:29 2005
@@ -5,6 +5,7 @@
  * to xenstore.
  * 
  * Copyright (c) 2005, Christian Limpach
+ * Copyright (c) 2005, Rusty Russell, IBM Corporation
  * 
  * This file may be distributed separately from the Linux kernel, or
  * incorporated into other software packages, subject to the following license:
@@ -45,100 +46,93 @@
 #include <asm-xen/xen_proc.h>
 
 struct xenbus_dev_data {
-       int in_transaction;
+       /* Are there bytes left to be read in this message? */
+       int bytes_left;
+       /* Are we still waiting for the reply to a message we wrote? */
+       int awaiting_reply;
+       /* Buffer for outgoing messages. */
+       unsigned int len;
+       union {
+               struct xsd_sockmsg msg;
+               char buffer[PAGE_SIZE];
+       } u;
 };
 
 static struct proc_dir_entry *xenbus_dev_intf;
 
-void *xs_talkv(enum xsd_sockmsg_type type, const struct kvec *iovec,
-              unsigned int num_vecs, unsigned int *len);
+/* Reply can be long (dir, getperm): don't buffer, just examine
+ * headers so we can discard rest if they die. */
+static ssize_t xenbus_dev_read(struct file *filp,
+                              char __user *ubuf,
+                              size_t len, loff_t *ppos)
+{
+       struct xenbus_dev_data *data = filp->private_data;
+       struct xsd_sockmsg msg;
+       int err;
 
-static int xenbus_dev_talkv(struct xenbus_dev_data *u, unsigned long data)
-{
-       struct xenbus_dev_talkv xt;
-       unsigned int len;
-       void *resp, *base;
-       struct kvec *iovec;
-       int ret = -EFAULT, v = 0;
+       /* Refill empty buffer? */
+       if (data->bytes_left == 0) {
+               if (len < sizeof(msg))
+                       return -EINVAL;
 
-       if (copy_from_user(&xt, (void *)data, sizeof(xt)))
-               return -EFAULT;
-
-       iovec = kmalloc(xt.num_vecs * sizeof(struct kvec), GFP_KERNEL);
-       if (iovec == NULL)
-               return -ENOMEM;
-
-       if (copy_from_user(iovec, xt.iovec,
-                          xt.num_vecs * sizeof(struct kvec)))
-               goto out;
-
-       for (v = 0; v < xt.num_vecs; v++) {
-               base = iovec[v].iov_base;
-               iovec[v].iov_base = kmalloc(iovec[v].iov_len, GFP_KERNEL);
-               if (iovec[v].iov_base == NULL ||
-                   copy_from_user(iovec[v].iov_base, base, iovec[v].iov_len))
-               {
-                       if (iovec[v].iov_base)
-                               kfree(iovec[v].iov_base);
-                       else
-                               ret = -ENOMEM;
-                       v--;
-                       goto out;
-               }
+               err = xb_read(&msg, sizeof(msg));
+               if (err)
+                       return err;
+               data->bytes_left = msg.len;
+               if (ubuf && copy_to_user(ubuf, &msg, sizeof(msg)) != 0)
+                       return -EFAULT;
+               /* We can receive spurious XS_WATCH_EVENT messages. */
+               if (msg.type != XS_WATCH_EVENT)
+                       data->awaiting_reply = 0;
+               return sizeof(msg);
        }
 
-       resp = xs_talkv(xt.type, iovec, xt.num_vecs, &len);
-       if (IS_ERR(resp)) {
-               ret = PTR_ERR(resp);
-               goto out;
-       }
+       /* Don't read over next header, or over temporary buffer. */
+       if (len > sizeof(data->u.buffer))
+               len = sizeof(data->u.buffer);
+       if (len > data->bytes_left)
+               len = data->bytes_left;
 
-       switch (xt.type) {
-       case XS_TRANSACTION_START:
-               u->in_transaction = 1;
-               break;
-       case XS_TRANSACTION_END:
-               u->in_transaction = 0;
-               break;
-       default:
-               break;
-       }
+       err = xb_read(data->u.buffer, len);
+       if (err)
+               return err;
 
-       ret = len;
-       if (len > xt.len)
-               len = xt.len;
-
-       if (copy_to_user(xt.buf, resp, len))
-               ret = -EFAULT;
-
-       kfree(resp);
- out:
-       while (v-- > 0)
-               kfree(iovec[v].iov_base);
-       kfree(iovec);
-       return ret;
+       data->bytes_left -= len;
+       if (ubuf && copy_to_user(ubuf, data->u.buffer, len) != 0)
+               return -EFAULT;
+       return len;
 }
 
-static int xenbus_dev_ioctl(struct inode *inode, struct file *filp,
-                           unsigned int cmd, unsigned long data)
+/* We do v. basic sanity checking so they don't screw up kernel later. */
+static ssize_t xenbus_dev_write(struct file *filp,
+                               const char __user *ubuf,
+                               size_t len, loff_t *ppos)
 {
-       struct xenbus_dev_data *u = filp->private_data;
-       int ret = -ENOSYS;
+       struct xenbus_dev_data *data = filp->private_data;
+       int err;
 
-       switch (cmd) {
-       case IOCTL_XENBUS_DEV_TALKV:
-               ret = xenbus_dev_talkv(u, data);
-               break;
-       default:
-               ret = -EINVAL;
-               break;
+       /* We gather data in buffer until we're ready to send it. */
+       if (len > data->len + sizeof(data->u))
+               return -EINVAL;
+       if (copy_from_user(data->u.buffer + data->len, ubuf, len) != 0)
+               return -EFAULT;
+       data->len += len;
+       if (data->len >= sizeof(data->u.msg) + data->u.msg.len) {
+               err = xb_write(data->u.buffer, data->len);
+               if (err)
+                       return err;
+               data->len = 0;
+               data->awaiting_reply = 1;
        }
-       return ret;
+       return len;
 }
 
 static int xenbus_dev_open(struct inode *inode, struct file *filp)
 {
        struct xenbus_dev_data *u;
+
+       /* Don't try seeking. */
+       nonseekable_open(inode, filp);
 
        u = kmalloc(sizeof(*u), GFP_KERNEL);
        if (u == NULL)
@@ -155,20 +149,25 @@
 
 static int xenbus_dev_release(struct inode *inode, struct file *filp)
 {
-       struct xenbus_dev_data *u = filp->private_data;
+       struct xenbus_dev_data *data = filp->private_data;
 
-       if (u->in_transaction)
-               xenbus_transaction_end(1);
+       /* Discard any unread replies. */
+       while (data->bytes_left || data->awaiting_reply)
+               xenbus_dev_read(filp, NULL, sizeof(data->u.buffer), NULL);
+
+       /* Harmless if no transaction in progress. */
+       xenbus_transaction_end(1);
 
        up(&xenbus_lock);
 
-       kfree(u);
+       kfree(data);
 
        return 0;
 }
 
 static struct file_operations xenbus_dev_file_ops = {
-       ioctl: xenbus_dev_ioctl,
+       read: xenbus_dev_read,
+       write: xenbus_dev_write,
        open: xenbus_dev_open,
        release: xenbus_dev_release
 };
diff -r f0d728001aaa -r df3759bbb309 
linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_xs.c
--- a/linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_xs.c       Wed Sep  7 
23:11:44 2005
+++ b/linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_xs.c       Fri Sep  9 
11:07:29 2005
@@ -106,10 +106,10 @@
 }
 
 /* Send message to xs, get kmalloc'ed reply.  ERR_PTR() on error. */
-void *xs_talkv(enum xsd_sockmsg_type type,
-              const struct kvec *iovec,
-              unsigned int num_vecs,
-              unsigned int *len)
+static void *xs_talkv(enum xsd_sockmsg_type type,
+                     const struct kvec *iovec,
+                     unsigned int num_vecs,
+                     unsigned int *len)
 {
        struct xsd_sockmsg msg;
        void *ret = NULL;
diff -r f0d728001aaa -r df3759bbb309 tools/xenstore/xs.c
--- a/tools/xenstore/xs.c       Wed Sep  7 23:11:44 2005
+++ b/tools/xenstore/xs.c       Fri Sep  9 11:07:29 2005
@@ -41,7 +41,6 @@
 struct xs_handle
 {
        int fd;
-       enum { SOCK, DEV } type;
 };
 
 /* Get the socket from the store daemon handle.
@@ -68,7 +67,6 @@
                h = malloc(sizeof(*h));
                if (h) {
                        h->fd = sock;
-                       h->type = SOCK;
                        return h;
                }
        }
@@ -82,16 +80,15 @@
 static struct xs_handle *get_dev(const char *connect_to)
 {
        int fd, saved_errno;
-       struct xs_handle *h = NULL;
-
-       fd = open(connect_to, O_RDONLY);
+       struct xs_handle *h;
+
+       fd = open(connect_to, O_RDWR);
        if (fd < 0)
                return NULL;
 
        h = malloc(sizeof(*h));
        if (h) {
                h->fd = fd;
-               h->type = DEV;
                return h;
        }
 
@@ -190,9 +187,9 @@
 }
 
 /* Send message to xs, get malloc'ed reply.  NULL and set errno on error. */
-static void *xs_talkv_sock(struct xs_handle *h, enum xsd_sockmsg_type type,
-                          const struct iovec *iovec, unsigned int num_vecs,
-                          unsigned int *len)
+static void *xs_talkv(struct xs_handle *h, enum xsd_sockmsg_type type,
+                     const struct iovec *iovec, unsigned int num_vecs,
+                     unsigned int *len)
 {
        struct xsd_sockmsg msg;
        void *ret = NULL;
@@ -250,54 +247,6 @@
        close(h->fd);
        h->fd = -1;
        errno = saved_errno;
-       return NULL;
-}
-
-/* Send message to xs, get malloc'ed reply.  NULL and set errno on error. */
-static void *xs_talkv_dev(struct xs_handle *h, enum xsd_sockmsg_type type,
-                         const struct iovec *iovec, unsigned int num_vecs,
-                         unsigned int *len)
-{
-       struct xenbus_dev_talkv dt;
-       char *buf;
-       int err, buflen = 1024;
-
- again:
-       buf = malloc(buflen);
-       if (buf == NULL) {
-               errno = ENOMEM;
-               return NULL;
-       }
-       dt.type = type;
-       dt.iovec = (struct kvec *)iovec;
-       dt.num_vecs = num_vecs;
-       dt.buf = buf;
-       dt.len = buflen;
-       err = ioctl(h->fd, IOCTL_XENBUS_DEV_TALKV, &dt);
-       if (err < 0) {
-               free(buf);
-               errno = err;
-               return NULL;
-       }
-       if (err > buflen) {
-               free(buf);
-               buflen = err;
-               goto again;
-       }
-       if (len)
-               *len = err;
-       return buf;
-}
-
-/* Send message to xs, get malloc'ed reply.  NULL and set errno on error. */
-static void *xs_talkv(struct xs_handle *h, enum xsd_sockmsg_type type,
-                     const struct iovec *iovec, unsigned int num_vecs,
-                     unsigned int *len)
-{
-       if (h->type == SOCK)
-               return xs_talkv_sock(h, type, iovec, num_vecs, len);
-       if (h->type == DEV)
-               return xs_talkv_dev(h, type, iovec, num_vecs, len);
        return NULL;
 }
 

-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.