hw/9pfs: Add validation to {un}marshal code

Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

2 files changed, 96 insertions, 71 deletions

diff --git a/fsdev/virtio-9p-marshal.c b/fsdev/virtio-9p-marshal.c
index 38fb99e72e..bf980bfa38 100644
--- a/fsdev/virtio-9p-marshal.c
+++ b/fsdev/virtio-9p-marshal.c
@@ -20,17 +20,12 @@
 #include <sys/uio.h>
 #include <string.h>
 #include <stdint.h>
+#include <errno.h>
 
 #include "compiler.h"
 #include "virtio-9p-marshal.h"
 #include "bswap.h"
 
-void v9fs_string_init(V9fsString *str)
-{
-    str->data = NULL;
-    str->size = 0;
-}
-
 void v9fs_string_free(V9fsString *str)
 {
     g_free(str->data);
@@ -62,11 +57,13 @@ void v9fs_string_copy(V9fsString *lhs, V9fsString *rhs)
 }
 
 
-static size_t v9fs_packunpack(void *addr, struct iovec *sg, int sg_count,
-                              size_t offset, size_t size, int pack)
+static ssize_t v9fs_packunpack(void *addr, struct iovec *sg, int sg_count,
+                               size_t offset, size_t size, int pack)
 {
     int i = 0;
     size_t copied = 0;
+    size_t req_size = size;
+
 
     for (i = 0; size && i < sg_count; i++) {
         size_t len;
@@ -90,27 +87,33 @@ static size_t v9fs_packunpack(void *addr, struct iovec *sg, int sg_count,
             }
         }
     }
-
+    if (copied < req_size) {
+        /*
+         * We copied less that requested size. error out
+         */
+        return -ENOBUFS;
+    }
     return copied;
 }
 
-static size_t v9fs_unpack(void *dst, struct iovec *out_sg, int out_num,
-                          size_t offset, size_t size)
+static ssize_t v9fs_unpack(void *dst, struct iovec *out_sg, int out_num,
+                           size_t offset, size_t size)
 {
     return v9fs_packunpack(dst, out_sg, out_num, offset, size, 0);
 }
 
-size_t v9fs_pack(struct iovec *in_sg, int in_num, size_t offset,
-                const void *src, size_t size)
+ssize_t v9fs_pack(struct iovec *in_sg, int in_num, size_t offset,
+                  const void *src, size_t size)
 {
     return v9fs_packunpack((void *)src, in_sg, in_num, offset, size, 1);
 }
 
-size_t v9fs_unmarshal(struct iovec *out_sg, int out_num, size_t offset,
-                      int bswap, const char *fmt, ...)
+ssize_t v9fs_unmarshal(struct iovec *out_sg, int out_num, size_t offset,
+                       int bswap, const char *fmt, ...)
 {
     int i;
     va_list ap;
+    ssize_t copied = 0;
     size_t old_offset = offset;
 
     va_start(ap, fmt);
@@ -118,13 +121,13 @@ size_t v9fs_unmarshal(struct iovec *out_sg, int out_num, size_t offset,
         switch (fmt[i]) {
         case 'b': {
             uint8_t *valp = va_arg(ap, uint8_t *);
-            offset += v9fs_unpack(valp, out_sg, out_num, offset, sizeof(*valp));
+            copied = v9fs_unpack(valp, out_sg, out_num, offset, sizeof(*valp));
             break;
         }
         case 'w': {
             uint16_t val, *valp;
             valp = va_arg(ap, uint16_t *);
-            offset += v9fs_unpack(&val, out_sg, out_num, offset, sizeof(val));
+            copied = v9fs_unpack(&val, out_sg, out_num, offset, sizeof(val));
             if (bswap) {
                 *valp = le16_to_cpu(val);
             } else {
@@ -135,7 +138,7 @@ size_t v9fs_unmarshal(struct iovec *out_sg, int out_num, size_t offset,
         case 'd': {
             uint32_t val, *valp;
             valp = va_arg(ap, uint32_t *);
-            offset += v9fs_unpack(&val, out_sg, out_num, offset, sizeof(val));
+            copied = v9fs_unpack(&val, out_sg, out_num, offset, sizeof(val));
             if (bswap) {
                 *valp = le32_to_cpu(val);
             } else {
@@ -146,7 +149,7 @@ size_t v9fs_unmarshal(struct iovec *out_sg, int out_num, size_t offset,
         case 'q': {
             uint64_t val, *valp;
             valp = va_arg(ap, uint64_t *);
-            offset += v9fs_unpack(&val, out_sg, out_num, offset, sizeof(val));
+            copied = v9fs_unpack(&val, out_sg, out_num, offset, sizeof(val));
             if (bswap) {
                 *valp = le64_to_cpu(val);
             } else {
@@ -156,59 +159,70 @@ size_t v9fs_unmarshal(struct iovec *out_sg, int out_num, size_t offset,
         }
         case 's': {
             V9fsString *str = va_arg(ap, V9fsString *);
-            offset += v9fs_unmarshal(out_sg, out_num, offset, bswap,
-                            "w", &str->size);
-            /* FIXME: sanity check str->size */
-            str->data = g_malloc(str->size + 1);
-            offset += v9fs_unpack(str->data, out_sg, out_num, offset,
-                            str->size);
-            str->data[str->size] = 0;
+            copied = v9fs_unmarshal(out_sg, out_num, offset, bswap,
+                                    "w", &str->size);
+            if (copied > 0) {
+                offset += copied;
+                str->data = g_malloc(str->size + 1);
+                copied = v9fs_unpack(str->data, out_sg, out_num, offset,
+                                     str->size);
+                if (copied > 0) {
+                    str->data[str->size] = 0;
+                } else {
+                    v9fs_string_free(str);
+                }
+            }
             break;
         }
         case 'Q': {
             V9fsQID *qidp = va_arg(ap, V9fsQID *);
-            offset += v9fs_unmarshal(out_sg, out_num, offset, bswap, "bdq",
-                                     &qidp->type, &qidp->version, &qidp->path);
+            copied = v9fs_unmarshal(out_sg, out_num, offset, bswap, "bdq",
+                                    &qidp->type, &qidp->version, &qidp->path);
             break;
         }
         case 'S': {
             V9fsStat *statp = va_arg(ap, V9fsStat *);
-            offset += v9fs_unmarshal(out_sg, out_num, offset, bswap,
+            copied = v9fs_unmarshal(out_sg, out_num, offset, bswap,
                                     "wwdQdddqsssssddd",
-                                     &statp->size, &statp->type, &statp->dev,
-                                     &statp->qid, &statp->mode, &statp->atime,
-                                     &statp->mtime, &statp->length,
-                                     &statp->name, &statp->uid, &statp->gid,
-                                     &statp->muid, &statp->extension,
-                                     &statp->n_uid, &statp->n_gid,
-                                     &statp->n_muid);
+                                    &statp->size, &statp->type, &statp->dev,
+                                    &statp->qid, &statp->mode, &statp->atime,
+                                    &statp->mtime, &statp->length,
+                                    &statp->name, &statp->uid, &statp->gid,
+                                    &statp->muid, &statp->extension,
+                                    &statp->n_uid, &statp->n_gid,
+                                    &statp->n_muid);
             break;
         }
         case 'I': {
             V9fsIattr *iattr = va_arg(ap, V9fsIattr *);
-            offset += v9fs_unmarshal(out_sg, out_num, offset, bswap,
-                                     "ddddqqqqq",
-                                     &iattr->valid, &iattr->mode,
-                                     &iattr->uid, &iattr->gid, &iattr->size,
-                                     &iattr->atime_sec, &iattr->atime_nsec,
-                                     &iattr->mtime_sec, &iattr->mtime_nsec);
+            copied = v9fs_unmarshal(out_sg, out_num, offset, bswap,
+                                    "ddddqqqqq",
+                                    &iattr->valid, &iattr->mode,
+                                    &iattr->uid, &iattr->gid, &iattr->size,
+                                    &iattr->atime_sec, &iattr->atime_nsec,
+                                    &iattr->mtime_sec, &iattr->mtime_nsec);
             break;
         }
         default:
             break;
         }
+        if (copied < 0) {
+            va_end(ap);
+            return copied;
+        }
+        offset += copied;
     }
-
     va_end(ap);
 
     return offset - old_offset;
 }
 
-size_t v9fs_marshal(struct iovec *in_sg, int in_num, size_t offset,
-                    int bswap, const char *fmt, ...)
+ssize_t v9fs_marshal(struct iovec *in_sg, int in_num, size_t offset,
+                     int bswap, const char *fmt, ...)
 {
     int i;
     va_list ap;
+    ssize_t copied = 0;
     size_t old_offset = offset;
 
     va_start(ap, fmt);
@@ -216,7 +230,7 @@ size_t v9fs_marshal(struct iovec *in_sg, int in_num, size_t offset,
         switch (fmt[i]) {
         case 'b': {
             uint8_t val = va_arg(ap, int);
-            offset += v9fs_pack(in_sg, in_num, offset, &val, sizeof(val));
+            copied = v9fs_pack(in_sg, in_num, offset, &val, sizeof(val));
             break;
         }
         case 'w': {
@@ -226,7 +240,7 @@ size_t v9fs_marshal(struct iovec *in_sg, int in_num, size_t offset,
             } else {
                 val =  va_arg(ap, int);
             }
-            offset += v9fs_pack(in_sg, in_num, offset, &val, sizeof(val));
+            copied = v9fs_pack(in_sg, in_num, offset, &val, sizeof(val));
             break;
         }
         case 'd': {
@@ -236,7 +250,7 @@ size_t v9fs_marshal(struct iovec *in_sg, int in_num, size_t offset,
             } else {
                 val =  va_arg(ap, uint32_t);
             }
-            offset += v9fs_pack(in_sg, in_num, offset, &val, sizeof(val));
+            copied = v9fs_pack(in_sg, in_num, offset, &val, sizeof(val));
             break;
         }
         case 'q': {
@@ -246,37 +260,40 @@ size_t v9fs_marshal(struct iovec *in_sg, int in_num, size_t offset,
             } else {
                 val =  va_arg(ap, uint64_t);
             }
-            offset += v9fs_pack(in_sg, in_num, offset, &val, sizeof(val));
+            copied = v9fs_pack(in_sg, in_num, offset, &val, sizeof(val));
             break;
         }
         case 's': {
             V9fsString *str = va_arg(ap, V9fsString *);
-            offset += v9fs_marshal(in_sg, in_num, offset, bswap,
-                            "w", str->size);
-            offset += v9fs_pack(in_sg, in_num, offset, str->data, str->size);
+            copied = v9fs_marshal(in_sg, in_num, offset, bswap,
+                                  "w", str->size);
+            if (copied > 0) {
+                offset += copied;
+                copied = v9fs_pack(in_sg, in_num, offset, str->data, str->size);
+            }
             break;
         }
         case 'Q': {
             V9fsQID *qidp = va_arg(ap, V9fsQID *);
-            offset += v9fs_marshal(in_sg, in_num, offset, bswap, "bdq",
-                                   qidp->type, qidp->version, qidp->path);
+            copied = v9fs_marshal(in_sg, in_num, offset, bswap, "bdq",
+                                  qidp->type, qidp->version, qidp->path);
             break;
         }
         case 'S': {
             V9fsStat *statp = va_arg(ap, V9fsStat *);
-            offset += v9fs_marshal(in_sg, in_num, offset, bswap,
-                                   "wwdQdddqsssssddd",
-                                   statp->size, statp->type, statp->dev,
-                                   &statp->qid, statp->mode, statp->atime,
-                                   statp->mtime, statp->length, &statp->name,
-                                   &statp->uid, &statp->gid, &statp->muid,
-                                   &statp->extension, statp->n_uid,
-                                   statp->n_gid, statp->n_muid);
+            copied = v9fs_marshal(in_sg, in_num, offset, bswap,
+                                  "wwdQdddqsssssddd",
+                                  statp->size, statp->type, statp->dev,
+                                  &statp->qid, statp->mode, statp->atime,
+                                  statp->mtime, statp->length, &statp->name,
+                                  &statp->uid, &statp->gid, &statp->muid,
+                                  &statp->extension, statp->n_uid,
+                                  statp->n_gid, statp->n_muid);
             break;
         }
         case 'A': {
             V9fsStatDotl *statp = va_arg(ap, V9fsStatDotl *);
-            offset += v9fs_marshal(in_sg, in_num, offset, bswap,
+            copied = v9fs_marshal(in_sg, in_num, offset, bswap,
                                    "qQdddqqqqqqqqqqqqqqq",
                                    statp->st_result_mask,
                                    &statp->qid, statp->st_mode,
@@ -294,6 +311,11 @@ size_t v9fs_marshal(struct iovec *in_sg, int in_num, size_t offset,
         default:
             break;
         }
+        if (copied < 0) {
+            va_end(ap);
+            return copied;
+        }
+        offset += copied;
     }
     va_end(ap);
 
diff --git a/fsdev/virtio-9p-marshal.h b/fsdev/virtio-9p-marshal.h
index f4414006dc..5df65a8357 100644
--- a/fsdev/virtio-9p-marshal.h
+++ b/fsdev/virtio-9p-marshal.h
@@ -71,17 +71,20 @@ typedef struct V9fsStatDotl {
     uint64_t st_data_version;
 } V9fsStatDotl;
 
-extern void v9fs_string_init(V9fsString *str);
+static inline void v9fs_string_init(V9fsString *str)
+{
+    str->data = NULL;
+    str->size = 0;
+}
 extern void v9fs_string_free(V9fsString *str);
 extern void v9fs_string_null(V9fsString *str);
 extern void v9fs_string_sprintf(V9fsString *str, const char *fmt, ...);
 extern void v9fs_string_copy(V9fsString *lhs, V9fsString *rhs);
 
-size_t v9fs_pack(struct iovec *in_sg, int in_num, size_t offset,
-                 const void *src, size_t size);
-size_t v9fs_unmarshal(struct iovec *out_sg, int out_num, size_t offset,
-                      int bswap, const char *fmt, ...);
-size_t v9fs_marshal(struct iovec *in_sg, int in_num, size_t offset,
-                    int bswap, const char *fmt, ...);
-
+ssize_t v9fs_pack(struct iovec *in_sg, int in_num, size_t offset,
+                  const void *src, size_t size);
+ssize_t v9fs_unmarshal(struct iovec *out_sg, int out_num, size_t offset,
+                       int bswap, const char *fmt, ...);
+ssize_t v9fs_marshal(struct iovec *in_sg, int in_num, size_t offset,
+                     int bswap, const char *fmt, ...);
 #endif