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

[Greg Kurz]

On Wed, 12 Oct 2016 19:53:39 -0700
Li Qiang  wrote:

> From: Li Qiang 
> 
> 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.

> Signed-off-by: Li Qiang 
> ---
>  hw/9pfs/9p.c | 34 +-
>  hw/9pfs/9p.h |  4 ++--
>  2 files changed, 15 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index e4040dc..8c7488f 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;
> @@ -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;
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index d539d2e..22198f6 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;

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.

>  void *value;
>  V9fsString name;
>  int flags;

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

[Li Qiang]

From: Li Qiang 

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.

Signed-off-by: Li Qiang 
---
 hw/9pfs/9p.c | 34 +-
 hw/9pfs/9p.h |  4 ++--
 2 files changed, 15 insertions(+), 23 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index e4040dc..8c7488f 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;
@@ -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;
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index d539d2e..22198f6 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