[Xen-changelog] [xen stable-4.3] libvchan: Fix handling of invalid ring buffer indices

[patchbot@xxxxxxx]

commit d7c6be61836b0a4d996f82d3e7c7e50150996701
Author:     Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
AuthorDate: Thu Feb 6 17:27:10 2014 +0100
Commit:     Jan Beulich <jbeulich@xxxxxxxx>
CommitDate: Thu Feb 6 17:27:10 2014 +0100

    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@xxxxxxxxxxxxxxxxxxxxxx>
    Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
    master commit: 2efcb0193bf3916c8ce34882e845f5ceb1e511f7
    master date: 2014-02-06 16:44:41 +0100
---
 tools/libvchan/io.c |   47 +++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/tools/libvchan/io.c b/tools/libvchan/io.c
index 3c8d236..40d072d 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);
--
generated by git-patchbot for /home/xen/git/xen.git#stable-4.3

_______________________________________________
Xen-changelog mailing list
Xen-changelog@xxxxxxxxxxxxx
http://lists.xensource.com/xen-changelog