libvchan: Fix handling of invalid ring buffer indices

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>

diff --git a/tools/libvchan/io.c b/tools/libvchan/io.c
index 2383364cc5c1b04b9384ab6009bf4e60ecf72017..804c63cca7aabad951a1a0128a42a2028e5ba598 100644 (file)
--- 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);