You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
172 lines
6.0 KiB
172 lines
6.0 KiB
From: Xen.org security team <security () xen org>
|
|
Date: Thu, 06 Feb 2014 12:39:17 +0000
|
|
|
|
From b4c452646efd37b4cd0996256dd0ab7bf6ccb7f6 Mon Sep 17 00:00:00 2001
|
|
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
|
|
<marmarek@invisiblethingslab.com>
|
|
Date: Mon, 20 Jan 2014 15:51:56 +0000
|
|
Subject: [PATCH] libvchan: Fix handling of invalid ring buffer indices
|
|
MIME-Version: 1.0
|
|
Content-Type: text/plain; charset=UTF-8
|
|
Content-Transfer-Encoding: 8bit
|
|
|
|
The remote (hostile) process can set ring buffer indices to any value
|
|
at any time. If that happens, it is possible to get "buffer space"
|
|
(either for writing data, or ready for reading) negative or greater
|
|
than buffer size. This will end up with buffer overflow in the second
|
|
memcpy inside of do_send/do_recv.
|
|
|
|
Fix this by introducing new available bytes accessor functions
|
|
raw_get_data_ready and raw_get_buffer_space which are robust against
|
|
mad ring states, and only return sanitised values.
|
|
|
|
Proof sketch of correctness:
|
|
|
|
Now {rd,wr}_{cons,prod} are only ever used in the raw available bytes
|
|
functions, and in do_send and do_recv.
|
|
|
|
The raw available bytes functions do unsigned arithmetic on the
|
|
returned values. If the result is "negative" or too big it will be
|
|
>ring_size (since we used unsigned arithmetic). Otherwise the result
|
|
is a positive in-range value representing a reasonable ring state, in
|
|
which case we can safely convert it to int (as the rest of the code
|
|
expects).
|
|
|
|
do_send and do_recv immediately mask the ring index value with the
|
|
ring size. The result is always going to be plausible. If the ring
|
|
state has become mad, the worst case is that our behaviour is
|
|
inconsistent with the peer's ring pointer. I.e. we read or write to
|
|
arguably-incorrect parts of the ring - but always parts of the ring.
|
|
And of course if a peer misoperates the ring they can achieve this
|
|
effect anyway.
|
|
|
|
So the security problem is fixed.
|
|
|
|
This is XSA-86.
|
|
|
|
(The patch is essentially Ian Jackson's work, although parts of the
|
|
commit message are by Marek.)
|
|
|
|
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
|
|
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
|
|
Cc: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
|
|
Cc: Joanna Rutkowska <joanna@invisiblethingslab.com>
|
|
---
|
|
tools/libvchan/io.c | 47 +++++++++++++++++++++++++++++++++++++++++------
|
|
1 file changed, 41 insertions(+), 6 deletions(-)
|
|
|
|
diff --git a/tools/libvchan/io.c b/tools/libvchan/io.c
|
|
index 2383364..804c63c 100644
|
|
--- a/tools/libvchan/io.c
|
|
+++ b/tools/libvchan/io.c
|
|
@@ -111,12 +111,26 @@ static inline int send_notify(struct libxenvchan *ctrl, uint8_t bit)
|
|
return 0;
|
|
}
|
|
|
|
+/*
|
|
+ * Get the amount of buffer space available, and do nothing about
|
|
+ * notifications.
|
|
+ */
|
|
+static inline int raw_get_data_ready(struct libxenvchan *ctrl)
|
|
+{
|
|
+ uint32_t ready = rd_prod(ctrl) - rd_cons(ctrl);
|
|
+ if (ready >= rd_ring_size(ctrl))
|
|
+ /* We have no way to return errors. Locking up the ring is
|
|
+ * better than the alternatives. */
|
|
+ return 0;
|
|
+ return ready;
|
|
+}
|
|
+
|
|
/**
|
|
* Get the amount of buffer space available and enable notifications if needed.
|
|
*/
|
|
static inline int fast_get_data_ready(struct libxenvchan *ctrl, size_t request)
|
|
{
|
|
- int ready = rd_prod(ctrl) - rd_cons(ctrl);
|
|
+ int ready = raw_get_data_ready(ctrl);
|
|
if (ready >= request)
|
|
return ready;
|
|
/* We plan to consume all data; please tell us if you send more */
|
|
@@ -126,7 +140,7 @@ static inline int fast_get_data_ready(struct libxenvchan *ctrl, size_t request)
|
|
* will not get notified even though the actual amount of data ready is
|
|
* above request. Reread rd_prod to cover this case.
|
|
*/
|
|
- return rd_prod(ctrl) - rd_cons(ctrl);
|
|
+ return raw_get_data_ready(ctrl);
|
|
}
|
|
|
|
int libxenvchan_data_ready(struct libxenvchan *ctrl)
|
|
@@ -135,7 +149,21 @@ int libxenvchan_data_ready(struct libxenvchan *ctrl)
|
|
* when it changes
|
|
*/
|
|
request_notify(ctrl, VCHAN_NOTIFY_WRITE);
|
|
- return rd_prod(ctrl) - rd_cons(ctrl);
|
|
+ return raw_get_data_ready(ctrl);
|
|
+}
|
|
+
|
|
+/**
|
|
+ * Get the amount of buffer space available, and do nothing
|
|
+ * about notifications
|
|
+ */
|
|
+static inline int raw_get_buffer_space(struct libxenvchan *ctrl)
|
|
+{
|
|
+ uint32_t ready = wr_ring_size(ctrl) - (wr_prod(ctrl) - wr_cons(ctrl));
|
|
+ if (ready > wr_ring_size(ctrl))
|
|
+ /* We have no way to return errors. Locking up the ring is
|
|
+ * better than the alternatives. */
|
|
+ return 0;
|
|
+ return ready;
|
|
}
|
|
|
|
/**
|
|
@@ -143,7 +171,7 @@ int libxenvchan_data_ready(struct libxenvchan *ctrl)
|
|
*/
|
|
static inline int fast_get_buffer_space(struct libxenvchan *ctrl, size_t request)
|
|
{
|
|
- int ready = wr_ring_size(ctrl) - (wr_prod(ctrl) - wr_cons(ctrl));
|
|
+ int ready = raw_get_buffer_space(ctrl);
|
|
if (ready >= request)
|
|
return ready;
|
|
/* We plan to fill the buffer; please tell us when you've read it */
|
|
@@ -153,7 +181,7 @@ static inline int fast_get_buffer_space(struct libxenvchan *ctrl, size_t request
|
|
* will not get notified even though the actual amount of buffer space
|
|
* is above request. Reread wr_cons to cover this case.
|
|
*/
|
|
- return wr_ring_size(ctrl) - (wr_prod(ctrl) - wr_cons(ctrl));
|
|
+ return raw_get_buffer_space(ctrl);
|
|
}
|
|
|
|
int libxenvchan_buffer_space(struct libxenvchan *ctrl)
|
|
@@ -162,7 +190,7 @@ int libxenvchan_buffer_space(struct libxenvchan *ctrl)
|
|
* when it changes
|
|
*/
|
|
request_notify(ctrl, VCHAN_NOTIFY_READ);
|
|
- return wr_ring_size(ctrl) - (wr_prod(ctrl) - wr_cons(ctrl));
|
|
+ return raw_get_buffer_space(ctrl);
|
|
}
|
|
|
|
int libxenvchan_wait(struct libxenvchan *ctrl)
|
|
@@ -176,6 +204,8 @@ int libxenvchan_wait(struct libxenvchan *ctrl)
|
|
|
|
/**
|
|
* returns -1 on error, or size on success
|
|
+ *
|
|
+ * caller must have checked that enough space is available
|
|
*/
|
|
static int do_send(struct libxenvchan *ctrl, const void *data, size_t size)
|
|
{
|
|
@@ -248,6 +278,11 @@ int libxenvchan_write(struct libxenvchan *ctrl, const void *data, size_t size)
|
|
}
|
|
}
|
|
|
|
+/**
|
|
+ * returns -1 on error, or size on success
|
|
+ *
|
|
+ * caller must have checked that enough data is available
|
|
+ */
|
|
static int do_recv(struct libxenvchan *ctrl, void *data, size_t size)
|
|
{
|
|
int real_idx = rd_cons(ctrl) & (rd_ring_size(ctrl) - 1);
|
|
--
|
|
1.7.10.4
|