Rusty Russell
2005-Sep-09 11:29 UTC
[Xen-devel] [PATCH] Prevent unnatural use of ioctl in /proc/xen/xenbus_dev
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
# HG changeset patch
# User Rusty Russell <rusty@rustcorp.com.au>
# 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@lists.xensource.com
http://lists.xensource.com/xen-devel
Keir Fraser
2005-Sep-09 12:48 UTC
Re: [Xen-devel] [PATCH] Prevent unnatural use of ioctl in /proc/xen/xenbus_dev
On 9 Sep 2005, at 12:29, Rusty Russell wrote:> 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.How can you cleanly express the request/reponse nature of interacting with xenstore, and grouping of accesses into transactions, via read/write? I would imagine you lose valuable framing information that you end up having to reconstruct. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Rusty Russell
2005-Sep-10 12:19 UTC
Re: [Xen-devel] [PATCH] Prevent unnatural use of ioctl in /proc/xen/xenbus_dev
On Fri, 2005-09-09 at 13:48 +0100, Keir Fraser wrote:> On 9 Sep 2005, at 12:29, Rusty Russell wrote: > > > 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. > > How can you cleanly express the request/reponse nature of interacting > with xenstore, and grouping of accesses into transactions, via > read/write?Err... the same way we do when tools interact with the store over a pipe? Which is the same protocol we use in shared memory.> I would imagine you lose valuable framing information that > you end up having to reconstruct.The only painful part is that userspace is now hijacking the kernel''s xenstore comms channel. While we clearly trust userspace somewhat, since they''re using our ID for operations, we want to ensure that we have a usable channel if they die unexpectedly. Otherwise it would be entirely trivial... Rusty. -- A bad analogy is like a leaky screwdriver -- Richard Braakman _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel