[PATCH 1/2] x86/hvmloader: Avoid data corruption with xenstore reads/writes

[PATCH 1/2] x86/hvmloader: Avoid data corruption with xenstore reads/writes

[Andrew Cooper]

The functions ring_read and ring_write() have logic to try and deal with
partial reads and writes.

However, in all cases where the "while (len)" loop executed twice, data
corruption would occur as the second memcpy() starts from the beginning of
"data" again, rather than from where it got to.

This bug manifested itself as protocol corruption when a reply header crossed
the first wrap of the response ring.  However, similar corruption would also
occur if hvmloader observed xenstored performing partial writes of the block
in question, or if hvmloader had to wait for xenstored to make space in either
ring.

Reported-by: Adam Kucia <djexit@o2.pl>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
---
 tools/firmware/hvmloader/xenbus.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/tools/firmware/hvmloader/xenbus.c b/tools/firmware/hvmloader/xenbus.c
index f900a1e..00513f7 100644
--- a/tools/firmware/hvmloader/xenbus.c
+++ b/tools/firmware/hvmloader/xenbus.c
@@ -105,7 +105,7 @@ void xenbus_shutdown(void)
 /* Helper functions: copy data in and out of the ring */
 static void ring_write(const char *data, uint32_t len)
 {
-    uint32_t part;
+    uint32_t part, done = 0;
 
     ASSERT(len <= XENSTORE_PAYLOAD_MAX);
 
@@ -122,16 +122,18 @@ static void ring_write(const char *data, uint32_t len)
         if ( part > len ) 
             part = len;
 
-        memcpy(rings->req + MASK_XENSTORE_IDX(rings->req_prod), data, part);
+        memcpy(rings->req + MASK_XENSTORE_IDX(rings->req_prod),
+               data + done, part);
         barrier(); /* = wmb before prod write, rmb before next cons read */
         rings->req_prod += part;
         len -= part;
+        done += part;
     }
 }
 
 static void ring_read(char *data, uint32_t len)
 {
-    uint32_t part;
+    uint32_t part, done = 0;
 
     ASSERT(len <= XENSTORE_PAYLOAD_MAX);
 
@@ -148,10 +150,12 @@ static void ring_read(char *data, uint32_t len)
         if ( part > len )
             part = len;
 
-        memcpy(data, rings->rsp + MASK_XENSTORE_IDX(rings->rsp_cons), part);
+        memcpy(data + done,
+               rings->rsp + MASK_XENSTORE_IDX(rings->rsp_cons), part);
         barrier(); /* = wmb before cons write, rmb before next prod read */
         rings->rsp_cons += part;
         len -= part;
+        done += part;
     }
 }
 
-- 
1.7.10.4

[PATCH 2/2] x86/hvmloader: Improve error handling for xenbus interactions

[Andrew Cooper]

Consume and ignore all XS_DEBUG packets, and pass the response type back to
the caller of xenbus_recv() so the caller can take appropriate action if an
unexpected reply was received.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
---
 tools/firmware/hvmloader/xenbus.c |   36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/tools/firmware/hvmloader/xenbus.c b/tools/firmware/hvmloader/xenbus.c
index 00513f7..d0ed993 100644
--- a/tools/firmware/hvmloader/xenbus.c
+++ b/tools/firmware/hvmloader/xenbus.c
@@ -208,15 +208,23 @@ static void xenbus_send(uint32_t type, ...)
  * Returns 0 for success, or an errno for error.
  * The answer is returned in a static buffer which is only
  * valid until the next call of xenbus_send(). */
-static int xenbus_recv(uint32_t *reply_len, const char **reply_data)
+static int xenbus_recv(uint32_t *reply_len, const char **reply_data,
+                       uint32_t *reply_type)
 {
     struct xsd_sockmsg hdr;
 
-    /* Pull the reply off the ring */
-    ring_read((char *) &hdr, sizeof(hdr));
-    ring_read(payload, hdr.len);
-    /* For sanity's sake, nul-terminate the answer */
-    payload[hdr.len] = '\0';
+    do
+    {
+        /* Pull the reply off the ring */
+        ring_read((char *) &hdr, sizeof(hdr));
+        ring_read(payload, hdr.len);
+        /* For sanity's sake, nul-terminate the answer */
+        payload[hdr.len] = '\0';
+
+    } while ( hdr.type == XS_DEBUG );
+
+    if ( reply_type )
+        *reply_type = hdr.type;
 
     /* Handle errors */
     if ( hdr.type == XS_ERROR )
@@ -247,7 +255,7 @@ static int xenbus_recv(uint32_t *reply_len, const char **reply_data)
  */
 const char *xenstore_read(const char *path, const char *default_resp)
 {
-    uint32_t len = 0;
+    uint32_t len = 0, type = 0;
     const char *answer = NULL;
 
     xenbus_send(XS_READ,
@@ -255,7 +263,7 @@ const char *xenstore_read(const char *path, const char *default_resp)
                 "", 1, /* nul separator */
                 NULL, 0);
 
-    if ( xenbus_recv(&len, &answer) )
+    if ( xenbus_recv(&len, &answer, &type) || (type != XS_READ) )
         answer = NULL;
 
     if ( (default_resp != NULL) && ((answer == NULL) || (*answer == '\0')) )
@@ -270,13 +278,23 @@ const char *xenstore_read(const char *path, const char *default_resp)
  */
 int xenstore_write(const char *path, const char *value)
 {
+    uint32_t len = 0, type = 0;
+    const char *answer = NULL;
+    int ret;
+
     xenbus_send(XS_WRITE,
                 path, strlen(path),
                 "", 1, /* nul separator */
                 value, strlen(value),
                 NULL, 0);
 
-    return ( xenbus_recv(NULL, NULL) );
+    ret = xenbus_recv(&len, &answer, &type);
+
+    if ( ret == 0 && ((type != XS_WRITE) || (len != 3) ||
+                      !answer || strcmp(answer, "OK")) )
+        ret = EIO;
+
+    return ret;
 }
 
 /*
-- 
1.7.10.4