[Qemu-devel] [PATCH v3 2/3] 9pfs: convert 'len/copied_len' field in V9fsXattr to the type of uint64_t

[Qemu-devel] [PATCH v3 2/3] 9pfs: convert 'len/copied_len' field in V9fsXattr to the type of uint64_t

[Li Qiang]

From: Li Qiang <liqiang6-s@360.cn>

The 'len' in V9fsXattr comes from the 'size' argument in setxattr()
function in guest. The setxattr() function's declaration is this:

int setxattr(const char *path, const char *name,
             const void *value, size_t size, int flags);

and 'size' is treated as u64 in linux kernel client code:

int p9_client_xattrcreate(struct p9_fid *fid, const char *name,
                          u64 attr_size, int flags)

So the 'len' should have an type of 'uint64_t'.
The 'copied_len' in V9fsXattr is used to account for copied bytes, it
should also have an type of 'uint64_t'.

Suggested-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Li Qiang <liqiang6-s@360.cn>
---
 hw/9pfs/9p.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index aa18da1..7fb075f 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -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;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 2/3] 9pfs: convert 'len/copied_len' field in V9fsXattr to the type of uint64_t

[Greg Kurz]

On Thu, 13 Oct 2016 03:09:42 -0700
Li Qiang <liq3ea@gmail.com> wrote:

> From: Li Qiang <liqiang6-s@360.cn>
> 
> The 'len' in V9fsXattr comes from the 'size' argument in setxattr()
> function in guest. The setxattr() function's declaration is this:
> 
> int setxattr(const char *path, const char *name,
>              const void *value, size_t size, int flags);
> 
> and 'size' is treated as u64 in linux kernel client code:
> 
> int p9_client_xattrcreate(struct p9_fid *fid, const char *name,
>                           u64 attr_size, int flags)
> 
> So the 'len' should have an type of 'uint64_t'.
> The 'copied_len' in V9fsXattr is used to account for copied bytes, it
> should also have an type of 'uint64_t'.
> 
> Suggested-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/9pfs/9p.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index aa18da1..7fb075f 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -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;

[Qemu-devel] [PATCH v3 3/3] 9pfs: fix integer overflow issue in xattr read/write

[Li Qiang]

From: Li Qiang <liqiang6-s@360.cn>

The v9fs_xattr_read() and v9fs_xattr_write() are passed a guest
originated offset: they must ensure this offset does not go beyond
the size of the extended attribute that was set in v9fs_xattrcreate().
Unfortunately, the current code implement these checks with unsafe
calculations on 32 and 64 bit values, which may allow a malicious
guest to cause OOB access anyway.

Fix this by comparing the offset and the xattr size, which are
both uint64_t, before trying to compute the effective number of bytes
to read or write.

Suggested-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Li Qiang <liqiang6-s@360.cn>
---

Changes since v2:
-make the solution of 'copied_len/len' in V9fsXattr type issue to a separate patch.
-add detailed changelog.

Changes since v1:
-delete 'xattr_len'.

 hw/9pfs/9p.c | 32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index e902eed..6df85b8 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -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;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 3/3] 9pfs: fix integer overflow issue in xattr read/write

[Greg Kurz]

On Thu, 13 Oct 2016 03:09:43 -0700
Li Qiang <liq3ea@gmail.com> wrote:

> From: Li Qiang <liqiang6-s@360.cn>
> 
> The v9fs_xattr_read() and v9fs_xattr_write() are passed a guest
> originated offset: they must ensure this offset does not go beyond
> the size of the extended attribute that was set in v9fs_xattrcreate().
> Unfortunately, the current code implement these checks with unsafe
> calculations on 32 and 64 bit values, which may allow a malicious
> guest to cause OOB access anyway.
> 
> Fix this by comparing the offset and the xattr size, which are
> both uint64_t, before trying to compute the effective number of bytes
> to read or write.
> 
> Suggested-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> ---
> 

Reviewed-by: Greg Kurz <groug@kaod.org>

> Changes since v2:
> -make the solution of 'copied_len/len' in V9fsXattr type issue to a separate patch.
> -add detailed changelog.
> 
> Changes since v1:
> -delete 'xattr_len'.
> 
>  hw/9pfs/9p.c | 32 ++++++++++++--------------------
>  1 file changed, 12 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index e902eed..6df85b8 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -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;

Re: [Qemu-devel] [PATCH v3 1/3] 9pfs: add xattrwalk_fid field in V9fsXattr struct

[Greg Kurz]

On Thu, 13 Oct 2016 03:09:41 -0700
Li Qiang <liq3ea@gmail.com> wrote:

> From: Li Qiang <liqiang6-s@360.cn>
> 
> Currently, 9pfs sets the 'copied_len' field in V9fsXattr
> to -1 to tag xattr walk fid. As the 'copied_len' is also
> used to account for copied bytes, this may make confusion. This patch
> add a bool 'xattrwalk_fid' to tag the xattr walk fid.
> 
> Suggested-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> ---
> 

Reviewed-by: Greg Kurz <groug@kaod.org>

> Changes since the v1:
> -move 'xattrwalk_fid' to 'V9fsXattr' struct.
> -add detailed changelog.
> 
>  hw/9pfs/9p.c | 7 ++++---
>  hw/9pfs/9p.h | 1 +
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index e4040dc..e902eed 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -325,7 +325,7 @@ static int v9fs_xattr_fid_clunk(V9fsPDU *pdu, V9fsFidState *fidp)
>  {
>      int retval = 0;
>  
> -    if (fidp->fs.xattr.copied_len == -1) {
> +    if (fidp->fs.xattr.xattrwalk_fid) {
>          /* getxattr/listxattr fid */
>          goto free_value;
>      }
> @@ -3189,7 +3189,7 @@ static void v9fs_xattrwalk(void *opaque)
>           */
>          xattr_fidp->fs.xattr.len = size;
>          xattr_fidp->fid_type = P9_FID_XATTR;
> -        xattr_fidp->fs.xattr.copied_len = -1;
> +        xattr_fidp->fs.xattr.xattrwalk_fid = true;
>          if (size) {
>              xattr_fidp->fs.xattr.value = g_malloc(size);
>              err = v9fs_co_llistxattr(pdu, &xattr_fidp->path,
> @@ -3222,7 +3222,7 @@ static void v9fs_xattrwalk(void *opaque)
>           */
>          xattr_fidp->fs.xattr.len = size;
>          xattr_fidp->fid_type = P9_FID_XATTR;
> -        xattr_fidp->fs.xattr.copied_len = -1;
> +        xattr_fidp->fs.xattr.xattrwalk_fid = true;
>          if (size) {
>              xattr_fidp->fs.xattr.value = g_malloc(size);
>              err = v9fs_co_lgetxattr(pdu, &xattr_fidp->path,
> @@ -3278,6 +3278,7 @@ static void v9fs_xattrcreate(void *opaque)
>      xattr_fidp = file_fidp;
>      xattr_fidp->fid_type = P9_FID_XATTR;
>      xattr_fidp->fs.xattr.copied_len = 0;
> +    xattr_fidp->fs.xattr.xattrwalk_fid = false;
>      xattr_fidp->fs.xattr.len = size;
>      xattr_fidp->fs.xattr.flags = flags;
>      v9fs_string_init(&xattr_fidp->fs.xattr.name);
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index d539d2e..aa18da1 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -164,6 +164,7 @@ typedef struct V9fsXattr
>      void *value;
>      V9fsString name;
>      int flags;
> +    bool xattrwalk_fid;
>  } V9fsXattr;
>  
>  typedef struct V9fsDir {