Skip to contentSkip to conversation

9pfs: fix integer overflow issue in xattr read/write

PATCH In review Li Qiang 2 participants 2 messages Oct 13, 2016

lore
Share & tools
lore

Li Qiang

author
#0 lore raw

PATCH v2 1/1 9pfs: fix integer overflow issue in xattr read/write

From: Li Qiang <[email protected]>

In 9pfs xattr read/write function, it mix to use unsigned/signed ,32/64 bits integers. This will causes oob read/write issues. This patch fix this.

---
 hw/9pfs/9p.c | 34 +++++++++++++---------------------
 hw/9pfs/9p.h |  4 ++--
 2 files changed, 15 insertions(+), 23 deletions(-)
hw/9pfs/9p.c +13 -21
index e4040dc..8c7488f 100644
@@ -1642,20 +1642,17 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
 {
     ssize_t err;
     size_t offset = 7;
-    int read_count;
-    int64_t xattr_len;
+    uint64_t read_count;
     V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
     VirtQueueElement *elem = v->elems[pdu->idx];
 
-    xattr_len = fidp->fs.xattr.len;
-    read_count = xattr_len - off;
+    if (fidp->fs.xattr.len < off) {
+        read_count = 0;
+    } else {
+       read_count = fidp->fs.xattr.len - off;
+    }
     if (read_count > max_count) {
         read_count = max_count;
-    } else if (read_count < 0) {
-        /*
-         * read beyond XATTR value
-         */
-        read_count = 0;
     }
     err = pdu_marshal(pdu, offset, "d", read_count);
     if (err < 0) {
@@ -1982,23 +1979,18 @@ static int v9fs_xattr_write(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
 {
     int i, to_copy;
     ssize_t err = 0;
-    int write_count;
-    int64_t xattr_len;
+    uint64_t write_count;
     size_t offset = 7;
 
 
-    xattr_len = fidp->fs.xattr.len;
-    write_count = xattr_len - off;
-    if (write_count > count) {
-        write_count = count;
-    } else if (write_count < 0) {
-        /*
-         * write beyond XATTR value len specified in
-         * xattrcreate
-         */
+    if (fidp->fs.xattr.len < off) {
         err = -ENOSPC;
         goto out;
     }
+    write_count = fidp->fs.xattr.len - off;
+    if (write_count > count) {
+        write_count = count;
+    }
     err = pdu_marshal(pdu, offset, "d", write_count);
     if (err < 0) {
         return err;
@@ -3254,7 +3246,7 @@ static void v9fs_xattrcreate(void *opaque)
 {
     int flags;
     int32_t fid;
-    int64_t size;
+    uint64_t size;
     ssize_t err = 0;
     V9fsString name;
     size_t offset = 7;
hw/9pfs/9p.h +2 -2
index d539d2e..22198f6 100644
@@ -159,8 +159,8 @@ typedef struct V9fsConf
 
 typedef struct V9fsXattr
 {
-    int64_t copied_len;
-    int64_t len;
+    uint64_t copied_len;
+    uint64_t len;
     void *value;
     V9fsString name;
     int flags;
Signature · 1 line
1.8.3.1
#1 lore raw
Quoted 6 lines · On Wed, 12 Oct 2016 19:53:39 -0700 Li Qiang <[email protected]> wrote:

From: Li Qiang <[email protected]>

In 9pfs xattr read/write function, it mix to use unsigned/signed ,32/64 bits integers. This will causes oob read/write issues. This patch fix this.

I suggested a more detailed changelog.

Quoted 81 lines
---
 hw/9pfs/9p.c | 34 +++++++++++++---------------------
 hw/9pfs/9p.h |  4 ++--
 2 files changed, 15 insertions(+), 23 deletions(-)
hw/9pfs/9p.c +13 -21
index e4040dc..8c7488f 100644
@@ -1642,20 +1642,17 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
 {
     ssize_t err;
     size_t offset = 7;
-    int read_count;
-    int64_t xattr_len;
+    uint64_t read_count;
     V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
     VirtQueueElement *elem = v->elems[pdu->idx];
 
-    xattr_len = fidp->fs.xattr.len;
-    read_count = xattr_len - off;
+    if (fidp->fs.xattr.len < off) {
+        read_count = 0;
+    } else {
+       read_count = fidp->fs.xattr.len - off;
+    }
     if (read_count > max_count) {
         read_count = max_count;
-    } else if (read_count < 0) {
-        /*
-         * read beyond XATTR value
-         */
-        read_count = 0;
     }
     err = pdu_marshal(pdu, offset, "d", read_count);
     if (err < 0) {
@@ -1982,23 +1979,18 @@ static int v9fs_xattr_write(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
 {
     int i, to_copy;
     ssize_t err = 0;
-    int write_count;
-    int64_t xattr_len;
+    uint64_t write_count;
     size_t offset = 7;
 
 
-    xattr_len = fidp->fs.xattr.len;
-    write_count = xattr_len - off;
-    if (write_count > count) {
-        write_count = count;
-    } else if (write_count < 0) {
-        /*
-         * write beyond XATTR value len specified in
-         * xattrcreate
-         */
+    if (fidp->fs.xattr.len < off) {
         err = -ENOSPC;
         goto out;
     }
+    write_count = fidp->fs.xattr.len - off;
+    if (write_count > count) {
+        write_count = count;
+    }
     err = pdu_marshal(pdu, offset, "d", write_count);
     if (err < 0) {
         return err;
@@ -3254,7 +3246,7 @@ static void v9fs_xattrcreate(void *opaque)
 {
     int flags;
     int32_t fid;
-    int64_t size;
+    uint64_t size;
     ssize_t err = 0;
     V9fsString name;
     size_t offset = 7;
hw/9pfs/9p.h +0 -0
index d539d2e..22198f6 100644

hw/9pfs/9p.h typedef struct V9fsConf · line 163

Quoted 7 lines
hw/9pfs/9p.h +2 -2
@@ -159,8 +159,8 @@ typedef struct V9fsConf
 
 typedef struct V9fsXattr
 {
-    int64_t copied_len;
-    int64_t len;
+    uint64_t copied_len;
+    uint64_t len;

I had asked this to go to a separate patch... with the appropriate justifications that these are byte counts and size_t/u64 in the guest.

Quoted 3 lines
hw/9pfs/9p.h +0 -0
     void *value;
     V9fsString name;
     int flags;