[PATCH] tmem fixes for Xen 4.5 - coverity inspired.

[PATCH] tmem fixes for Xen 4.5 - coverity inspired.

[Konrad Rzeszutek Wilk]

These patches are inspired by Coverity results - and I hope the
last of them in the 'tmem' department.

Please review, especially the:
 [PATCH 4/4] tmem: fix Out-of-bounds read reported by Coverity

at your leisure.


 tools/libxc/xc_domain_save.c |  8 +++++--
 tools/libxc/xc_tmem.c        | 51 ++++++++++++++++++++++++++------------------
 tools/libxc/xenctrl.h        |  2 +-
 xen/common/tmem.c            | 10 ++-------
 4 files changed, 39 insertions(+), 32 deletions(-)

Bob Liu (2):
      tmem: drop unnecessary lock in tmem_relinquish_pages()
      tmem: fix Out-of-bounds read reported by Coverity

Konrad Rzeszutek Wilk (2):
      xc/tmem: Free temporary buffer used during migration.
      xc/tmem: Unchecked return value (v2).

[PATCH 1/4] xc/tmem: Free temporary buffer used during migration.

[Konrad Rzeszutek Wilk]

CID 1090388.

Within the loop reading the pool_id we set the buf:

 if ( (buf = realloc(buf,bufsize)) == NULL )

and then continue on using it without freeing. Worst yet
there are multiple 'if (..) return -1' which do not free
the buffer.

As such insert a 'fail' goto label to free the buffer
and also add on the OK path a mechanism to free the buffer.

Replace all of the 'return -1' with a jump to the failed
label.

CC: Bob Liu <bob.liu@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 tools/libxc/xc_tmem.c | 47 ++++++++++++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 19 deletions(-)

diff --git a/tools/libxc/xc_tmem.c b/tools/libxc/xc_tmem.c
index 3261e10..12fa105 100644
--- a/tools/libxc/xc_tmem.c
+++ b/tools/libxc/xc_tmem.c
@@ -215,6 +215,7 @@ int xc_tmem_save(xc_interface *xch,
     uint32_t pool_id;
     uint32_t minusone = -1;
     struct tmem_handle *h;
+    char *buf = NULL;
 
     if ( xc_tmem_control(xch,0,TMEMC_SAVE_BEGIN,dom,live,0,0,NULL) <= 0 )
         return 0;
@@ -249,7 +250,6 @@ int xc_tmem_save(xc_interface *xch,
         uint64_t uuid[2];
         uint32_t n_pages;
         uint32_t pagesize;
-        char *buf = NULL;
         int bufsize = 0;
         int checksum = 0;
 
@@ -263,13 +263,13 @@ int xc_tmem_save(xc_interface *xch,
                 n_pages = 0;
             (void)xc_tmem_control(xch,i,TMEMC_SAVE_GET_POOL_UUID,dom,sizeof(uuid),0,0,&uuid);
             if ( write_exact(io_fd, &pool_id, sizeof(pool_id)) )
-                return -1;
+                goto failed;
             if ( write_exact(io_fd, &flags, sizeof(flags)) )
-                return -1;
+                goto failed;
             if ( write_exact(io_fd, &n_pages, sizeof(n_pages)) )
-                return -1;
+                goto failed;
             if ( write_exact(io_fd, &uuid, sizeof(uuid)) )
-                return -1;
+                goto failed;
             if ( n_pages == 0 )
                 continue;
 
@@ -279,7 +279,7 @@ int xc_tmem_save(xc_interface *xch,
             {
                 bufsize = pagesize + sizeof(struct tmem_handle);
                 if ( (buf = realloc(buf,bufsize)) == NULL )
-                    return -1;
+                    goto failed;
             }
             for ( j = n_pages; j > 0; j-- )
             {
@@ -290,21 +290,22 @@ int xc_tmem_save(xc_interface *xch,
                 {
                     h = (struct tmem_handle *)buf;
                     if ( write_exact(io_fd, &h->oid, sizeof(h->oid)) )
-                        return -1;
+                        goto failed;
                     if ( write_exact(io_fd, &h->index, sizeof(h->index)) )
-                        return -1;
+                        goto failed;
                     h++;
                     checksum += *(char *)h;
                     if ( write_exact(io_fd, h, pagesize) )
-                        return -1;
+                        goto failed;
                 } else if ( ret == 0 ) {
                     continue;
                 } else {
                     /* page list terminator */
                     h = (struct tmem_handle *)buf;
                     h->oid[0] = h->oid[1] = h->oid[2] = -1L;
-                    if ( write_exact(io_fd, &h->oid, sizeof(h->oid)) )
-                        return -1;
+                    if ( write_exact(io_fd, &h->oid, sizeof(h->oid)) ) {
+                        goto failed;
+                    }
                     break;
                 }
             }
@@ -315,9 +316,13 @@ int xc_tmem_save(xc_interface *xch,
     /* pool list terminator */
     minusone = -1;
     if ( write_exact(io_fd, &minusone, sizeof(minusone)) )
-        return -1;
+        goto failed;
 
+    free(buf);
     return 1;
+failed:
+    free(buf);
+    return -1;
 }
 
 /* only called for live migration */
@@ -386,6 +391,7 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd)
     uint32_t minusone;
     uint32_t weight, cap, flags;
     int checksum = 0;
+    char *buf = NULL;
 
     save_version = xc_tmem_control(xch,0,TMEMC_SAVE_GET_VERSION,dom,0,0,0,NULL);
     if ( save_version == -1 )
@@ -423,7 +429,6 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd)
     {
         uint64_t uuid[2];
         uint32_t n_pages;
-        char *buf = NULL;
         int bufsize = 0, pagesize;
         int j;
 
@@ -445,7 +450,7 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd)
         {
             bufsize = pagesize;
             if ( (buf = realloc(buf,bufsize)) == NULL )
-                return -1;
+                goto failed;
         }
         for ( j = n_pages; j > 0; j-- )
         {
@@ -453,20 +458,20 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd)
             uint32_t index;
             int rc;
             if ( read_exact(io_fd, &oid, sizeof(oid)) )
-                return -1;
+                goto failed;
             if ( oid.oid[0] == -1L && oid.oid[1] == -1L && oid.oid[2] == -1L )
                 break;
             if ( read_exact(io_fd, &index, sizeof(index)) )
-                return -1;
+                goto failed;
             if ( read_exact(io_fd, buf, pagesize) )
-                return -1;
+                goto failed;
             checksum += *buf;
             if ( (rc = xc_tmem_control_oid(xch, pool_id,
                                            TMEMC_RESTORE_PUT_PAGE, dom,
                                            bufsize, index, oid, buf)) <= 0 )
             {
                 DPRINTF("xc_tmem_restore: putting page failed, rc=%d\n",rc);
-                return -1;
+                goto failed;
             }
         }
         if ( n_pages )
@@ -474,9 +479,13 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd)
                     n_pages-j,dom,pool_id,checksum);
     }
     if ( pool_id != -1 )
-        return -1;
+        goto failed;
 
+    free(buf);
     return 0;
+failed:
+    free(buf);
+    return -1;
 }
 
 /* only called for live migration, must be called after suspend */
-- 
1.8.5.3

Re: [PATCH 1/4] xc/tmem: Free temporary buffer used during migration.

[Andrew Cooper]

On 30/04/2014 21:29, Konrad Rzeszutek Wilk wrote:
> CID 1090388.
>
> Within the loop reading the pool_id we set the buf:
>
>  if ( (buf = realloc(buf,bufsize)) == NULL )
>
> and then continue on using it without freeing. Worst yet
> there are multiple 'if (..) return -1' which do not free
> the buffer.
>
> As such insert a 'fail' goto label to free the buffer
> and also add on the OK path a mechanism to free the buffer.
>
> Replace all of the 'return -1' with a jump to the failed
> label.
>
> CC: Bob Liu <bob.liu@oracle.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  tools/libxc/xc_tmem.c | 47 ++++++++++++++++++++++++++++-------------------
>  1 file changed, 28 insertions(+), 19 deletions(-)
>
> diff --git a/tools/libxc/xc_tmem.c b/tools/libxc/xc_tmem.c
> index 3261e10..12fa105 100644
> --- a/tools/libxc/xc_tmem.c
> +++ b/tools/libxc/xc_tmem.c
> @@ -215,6 +215,7 @@ int xc_tmem_save(xc_interface *xch,
>      uint32_t pool_id;
>      uint32_t minusone = -1;
>      struct tmem_handle *h;
> +    char *buf = NULL;
>  
>      if ( xc_tmem_control(xch,0,TMEMC_SAVE_BEGIN,dom,live,0,0,NULL) <= 0 )
>          return 0;
> @@ -249,7 +250,6 @@ int xc_tmem_save(xc_interface *xch,
>          uint64_t uuid[2];
>          uint32_t n_pages;
>          uint32_t pagesize;
> -        char *buf = NULL;
>          int bufsize = 0;
>          int checksum = 0;
>  
> @@ -263,13 +263,13 @@ int xc_tmem_save(xc_interface *xch,
>                  n_pages = 0;
>              (void)xc_tmem_control(xch,i,TMEMC_SAVE_GET_POOL_UUID,dom,sizeof(uuid),0,0,&uuid);
>              if ( write_exact(io_fd, &pool_id, sizeof(pool_id)) )
> -                return -1;
> +                goto failed;
>              if ( write_exact(io_fd, &flags, sizeof(flags)) )
> -                return -1;
> +                goto failed;
>              if ( write_exact(io_fd, &n_pages, sizeof(n_pages)) )
> -                return -1;
> +                goto failed;
>              if ( write_exact(io_fd, &uuid, sizeof(uuid)) )
> -                return -1;
> +                goto failed;
>              if ( n_pages == 0 )
>                  continue;
>  
> @@ -279,7 +279,7 @@ int xc_tmem_save(xc_interface *xch,
>              {
>                  bufsize = pagesize + sizeof(struct tmem_handle);
>                  if ( (buf = realloc(buf,bufsize)) == NULL )
> -                    return -1;
> +                    goto failed;
>              }
>              for ( j = n_pages; j > 0; j-- )
>              {
> @@ -290,21 +290,22 @@ int xc_tmem_save(xc_interface *xch,
>                  {
>                      h = (struct tmem_handle *)buf;
>                      if ( write_exact(io_fd, &h->oid, sizeof(h->oid)) )
> -                        return -1;
> +                        goto failed;
>                      if ( write_exact(io_fd, &h->index, sizeof(h->index)) )
> -                        return -1;
> +                        goto failed;
>                      h++;
>                      checksum += *(char *)h;
>                      if ( write_exact(io_fd, h, pagesize) )
> -                        return -1;
> +                        goto failed;
>                  } else if ( ret == 0 ) {
>                      continue;
>                  } else {
>                      /* page list terminator */
>                      h = (struct tmem_handle *)buf;
>                      h->oid[0] = h->oid[1] = h->oid[2] = -1L;
> -                    if ( write_exact(io_fd, &h->oid, sizeof(h->oid)) )
> -                        return -1;
> +                    if ( write_exact(io_fd, &h->oid, sizeof(h->oid)) ) {
> +                        goto failed;
> +                    }

Superfluous braces.

Otherwise, still fine.

~Andrew

>                      break;
>                  }
>              }
> @@ -315,9 +316,13 @@ int xc_tmem_save(xc_interface *xch,
>      /* pool list terminator */
>      minusone = -1;
>      if ( write_exact(io_fd, &minusone, sizeof(minusone)) )
> -        return -1;
> +        goto failed;
>  
> +    free(buf);
>      return 1;
> +failed:
> +    free(buf);
> +    return -1;
>  }
>  
>  /* only called for live migration */
> @@ -386,6 +391,7 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd)
>      uint32_t minusone;
>      uint32_t weight, cap, flags;
>      int checksum = 0;
> +    char *buf = NULL;
>  
>      save_version = xc_tmem_control(xch,0,TMEMC_SAVE_GET_VERSION,dom,0,0,0,NULL);
>      if ( save_version == -1 )
> @@ -423,7 +429,6 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd)
>      {
>          uint64_t uuid[2];
>          uint32_t n_pages;
> -        char *buf = NULL;
>          int bufsize = 0, pagesize;
>          int j;
>  
> @@ -445,7 +450,7 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd)
>          {
>              bufsize = pagesize;
>              if ( (buf = realloc(buf,bufsize)) == NULL )
> -                return -1;
> +                goto failed;
>          }
>          for ( j = n_pages; j > 0; j-- )
>          {
> @@ -453,20 +458,20 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd)
>              uint32_t index;
>              int rc;
>              if ( read_exact(io_fd, &oid, sizeof(oid)) )
> -                return -1;
> +                goto failed;
>              if ( oid.oid[0] == -1L && oid.oid[1] == -1L && oid.oid[2] == -1L )
>                  break;
>              if ( read_exact(io_fd, &index, sizeof(index)) )
> -                return -1;
> +                goto failed;
>              if ( read_exact(io_fd, buf, pagesize) )
> -                return -1;
> +                goto failed;
>              checksum += *buf;
>              if ( (rc = xc_tmem_control_oid(xch, pool_id,
>                                             TMEMC_RESTORE_PUT_PAGE, dom,
>                                             bufsize, index, oid, buf)) <= 0 )
>              {
>                  DPRINTF("xc_tmem_restore: putting page failed, rc=%d\n",rc);
> -                return -1;
> +                goto failed;
>              }
>          }
>          if ( n_pages )
> @@ -474,9 +479,13 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd)
>                      n_pages-j,dom,pool_id,checksum);
>      }
>      if ( pool_id != -1 )
> -        return -1;
> +        goto failed;
>  
> +    free(buf);
>      return 0;
> +failed:
> +    free(buf);
> +    return -1;
>  }
>  
>  /* only called for live migration, must be called after suspend */

[PATCH 2/4] xc/tmem: Unchecked return value (v2).

[Konrad Rzeszutek Wilk]

CID 1055045 complains that the return value needs checking.

The tmem op is a flush call and there is no expectation
right now of any return besides zero. But just in case
this changes lets blow up.

CC: Bob Liu <bob.liu@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
[v2: Add a comment why bailing out on xc_tmem_save_done is not needed]
---
 tools/libxc/xc_domain_save.c | 8 ++++++--
 tools/libxc/xc_tmem.c        | 4 ++--
 tools/libxc/xenctrl.h        | 2 +-
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
index 71f9b59..b04e387 100644
--- a/tools/libxc/xc_domain_save.c
+++ b/tools/libxc/xc_domain_save.c
@@ -2107,8 +2107,12 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
         goto copypages;
     }
 
-    if ( tmem_saved != 0 && live )
-        xc_tmem_save_done(xch, dom);
+    if ( tmem_saved != 0 && live && xc_tmem_save_done(xch, dom))
+    {
+        /* N.B. No need to bail out - we leak memory, but that is all assigned
+         * to the zombie guest. */
+       DPRINTF("Warning - failed to flush tmem on originating server.");
+    }
 
     if ( live )
     {
diff --git a/tools/libxc/xc_tmem.c b/tools/libxc/xc_tmem.c
index 12fa105..6e01a6a 100644
--- a/tools/libxc/xc_tmem.c
+++ b/tools/libxc/xc_tmem.c
@@ -356,9 +356,9 @@ int xc_tmem_save_extra(xc_interface *xch, int dom, int io_fd, int field_marker)
 }
 
 /* only called for live migration */
-void xc_tmem_save_done(xc_interface *xch, int dom)
+int xc_tmem_save_done(xc_interface *xch, int dom)
 {
-    xc_tmem_control(xch,0,TMEMC_SAVE_END,dom,0,0,0,NULL);
+    return xc_tmem_control(xch,0,TMEMC_SAVE_END,dom,0,0,0,NULL);
 }
 
 /* restore routines */
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index 02129f7..23630a7 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -2011,7 +2011,7 @@ int xc_tmem_control(xc_interface *xch,
 int xc_tmem_auth(xc_interface *xch, int cli_id, char *uuid_str, int arg1);
 int xc_tmem_save(xc_interface *xch, int dom, int live, int fd, int field_marker);
 int xc_tmem_save_extra(xc_interface *xch, int dom, int fd, int field_marker);
-void xc_tmem_save_done(xc_interface *xch, int dom);
+int xc_tmem_save_done(xc_interface *xch, int dom);
 int xc_tmem_restore(xc_interface *xch, int dom, int fd);
 int xc_tmem_restore_extra(xc_interface *xch, int dom, int fd);
 
-- 
1.8.5.3

Re: [PATCH 2/4] xc/tmem: Unchecked return value (v2).

[Andrew Cooper]

On 30/04/2014 21:29, Konrad Rzeszutek Wilk wrote:
> CID 1055045 complains that the return value needs checking.
>
> The tmem op is a flush call and there is no expectation
> right now of any return besides zero. But just in case
> this changes lets blow up.
>
> CC: Bob Liu <bob.liu@oracle.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> [v2: Add a comment why bailing out on xc_tmem_save_done is not needed]
> ---
>  tools/libxc/xc_domain_save.c | 8 ++++++--
>  tools/libxc/xc_tmem.c        | 4 ++--
>  tools/libxc/xenctrl.h        | 2 +-
>  3 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
> index 71f9b59..b04e387 100644
> --- a/tools/libxc/xc_domain_save.c
> +++ b/tools/libxc/xc_domain_save.c
> @@ -2107,8 +2107,12 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
>          goto copypages;
>      }
>  
> -    if ( tmem_saved != 0 && live )
> -        xc_tmem_save_done(xch, dom);
> +    if ( tmem_saved != 0 && live && xc_tmem_save_done(xch, dom))

Missing space before final bracket

> +    {
> +        /* N.B. No need to bail out - we leak memory, but that is all assigned
> +         * to the zombie guest. */
> +       DPRINTF("Warning - failed to flush tmem on originating server.");
> +    }

And superfluous braces.

Functionally however, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

~Andrew

>  
>      if ( live )
>      {
> diff --git a/tools/libxc/xc_tmem.c b/tools/libxc/xc_tmem.c
> index 12fa105..6e01a6a 100644
> --- a/tools/libxc/xc_tmem.c
> +++ b/tools/libxc/xc_tmem.c
> @@ -356,9 +356,9 @@ int xc_tmem_save_extra(xc_interface *xch, int dom, int io_fd, int field_marker)
>  }
>  
>  /* only called for live migration */
> -void xc_tmem_save_done(xc_interface *xch, int dom)
> +int xc_tmem_save_done(xc_interface *xch, int dom)
>  {
> -    xc_tmem_control(xch,0,TMEMC_SAVE_END,dom,0,0,0,NULL);
> +    return xc_tmem_control(xch,0,TMEMC_SAVE_END,dom,0,0,0,NULL);
>  }
>  
>  /* restore routines */
> diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
> index 02129f7..23630a7 100644
> --- a/tools/libxc/xenctrl.h
> +++ b/tools/libxc/xenctrl.h
> @@ -2011,7 +2011,7 @@ int xc_tmem_control(xc_interface *xch,
>  int xc_tmem_auth(xc_interface *xch, int cli_id, char *uuid_str, int arg1);
>  int xc_tmem_save(xc_interface *xch, int dom, int live, int fd, int field_marker);
>  int xc_tmem_save_extra(xc_interface *xch, int dom, int fd, int field_marker);
> -void xc_tmem_save_done(xc_interface *xch, int dom);
> +int xc_tmem_save_done(xc_interface *xch, int dom);
>  int xc_tmem_restore(xc_interface *xch, int dom, int fd);
>  int xc_tmem_restore_extra(xc_interface *xch, int dom, int fd);
>  

[PATCH 3/4] tmem: drop unnecessary lock in tmem_relinquish_pages()

[Konrad Rzeszutek Wilk]

From: Bob Liu <bob.liu@oracle.com>

CID 1150562

tmem_rwlock is unnecessary in tmem_relinquish_pages(), as
such lock is used as gate for hypercalls. However
tmem_relinquish_pages deals with pages that are no longer
owned by any domain - hence there is no need for tmem_rwlock.

Also the function is protected by the 'heap_lock' which
is the only calleer of this function.

This patch drops said lock.

Signed-off-by: Bob Liu <bob.liu@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/common/tmem.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/xen/common/tmem.c b/xen/common/tmem.c
index f7f828f..f2dc26e 100644
--- a/xen/common/tmem.c
+++ b/xen/common/tmem.c
@@ -2794,9 +2794,6 @@ void *tmem_relinquish_pages(unsigned int order, unsigned int memflags)
         return NULL;
     }
 
-    if ( memflags & MEMF_tmem )
-        read_lock(&tmem_rwlock);
-
     while ( (pfp = tmem_page_list_get()) == NULL )
     {
         if ( (max_evictions-- <= 0) || !tmem_evict())
@@ -2812,9 +2809,6 @@ void *tmem_relinquish_pages(unsigned int order, unsigned int memflags)
         relinq_pgs++;
     }
 
-    if ( memflags & MEMF_tmem )
-        read_unlock(&tmem_rwlock);
-
     return pfp;
 }
 
-- 
1.8.5.3

[PATCH 4/4] tmem: fix Out-of-bounds read reported by Coverity

[Konrad Rzeszutek Wilk]

From: Bob Liu <bob.liu@oracle.com>

CID 1198729, CID 1198730 and CID 1198734 complain about
"Out-of-bounds read".

This patch fixes them by casting the 'firstbyte' to (uint8_t).

Signed-off-by: Bob Liu <bob.liu@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/common/tmem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/common/tmem.c b/xen/common/tmem.c
index f2dc26e..506c6be 100644
--- a/xen/common/tmem.c
+++ b/xen/common/tmem.c
@@ -399,7 +399,7 @@ static void pcd_disassociate(struct tmem_page_descriptor *pgp, struct tmem_pool
 {
     struct tmem_page_content_descriptor *pcd = pgp->pcd;
     struct page_info *pfp = pgp->pcd->pfp;
-    uint16_t firstbyte = pgp->firstbyte;
+    uint8_t firstbyte = pgp->firstbyte;
     char *pcd_tze = pgp->pcd->tze;
     pagesize_t pcd_size = pcd->size;
     pagesize_t pgp_size = pgp->size;
@@ -1231,7 +1231,7 @@ static bool_t tmem_try_to_evict_pgp(struct tmem_page_descriptor *pgp, bool_t *ho
     struct tmem_object_root *obj = pgp->us.obj;
     struct tmem_pool *pool = obj->pool;
     struct client *client = pool->client;
-    uint16_t firstbyte = pgp->firstbyte;
+    uint8_t firstbyte = pgp->firstbyte;
 
     if ( pool->is_dying )
         return 0;
-- 
1.8.5.3

Re: [PATCH 4/4] tmem: fix Out-of-bounds read reported by Coverity

[Andrew Cooper]

On 30/04/2014 21:29, Konrad Rzeszutek Wilk wrote:
> From: Bob Liu <bob.liu@oracle.com>
>
> CID 1198729, CID 1198730 and CID 1198734 complain about
> "Out-of-bounds read".
>
> This patch fixes them by casting the 'firstbyte' to (uint8_t).
>
> Signed-off-by: Bob Liu <bob.liu@oracle.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  xen/common/tmem.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/common/tmem.c b/xen/common/tmem.c
> index f2dc26e..506c6be 100644
> --- a/xen/common/tmem.c
> +++ b/xen/common/tmem.c
> @@ -399,7 +399,7 @@ static void pcd_disassociate(struct tmem_page_descriptor *pgp, struct tmem_pool
>  {
>      struct tmem_page_content_descriptor *pcd = pgp->pcd;
>      struct page_info *pfp = pgp->pcd->pfp;
> -    uint16_t firstbyte = pgp->firstbyte;
> +    uint8_t firstbyte = pgp->firstbyte;

Actually looking at these CIDs, I think this is a coverity bug rather
than a tmem bug.

The two asserts

ASSERT(firstbyte != NOT_SHAREABLE); /* for NOT_SHAREABLE being
(uint16_t)-1UL; */
ASSERT(firstbyte < 256);

Cause the coverity analysis engine to decide:

"cond_const: Checking firstbyte != 65535 implies that firstbyte and
pgp->firstbyte have the value 65535 on the false branch."

despite the fact that the second assert entirely covering the first. 
Furthermore, I don't understand why the ASSERT() killpath isn't
invalidating any analysis on the false branch of an ASSERT().

If you are changing uint16_t to uint8_t, you can drop those two asserts
as well, as they become unconditionally true.

>      char *pcd_tze = pgp->pcd->tze;
>      pagesize_t pcd_size = pcd->size;
>      pagesize_t pgp_size = pgp->size;
> @@ -1231,7 +1231,7 @@ static bool_t tmem_try_to_evict_pgp(struct tmem_page_descriptor *pgp, bool_t *ho
>      struct tmem_object_root *obj = pgp->us.obj;
>      struct tmem_pool *pool = obj->pool;
>      struct client *client = pool->client;
> -    uint16_t firstbyte = pgp->firstbyte;
> +    uint8_t firstbyte = pgp->firstbyte;
>  
>      if ( pool->is_dying )
>          return 0;

Given the "if ( firstbyte ==  NOT_SHAREABLE ) goto obj_unlock;", are you
certain this change is safe?

~Andrew

Re: [PATCH 4/4] tmem: fix Out-of-bounds read reported by Coverity

[Bob Liu]

Hi Andrew,

On 05/01/2014 05:04 AM, Andrew Cooper wrote:
> On 30/04/2014 21:29, Konrad Rzeszutek Wilk wrote:
>> From: Bob Liu <bob.liu@oracle.com>
>>
>> CID 1198729, CID 1198730 and CID 1198734 complain about
>> "Out-of-bounds read".
>>
>> This patch fixes them by casting the 'firstbyte' to (uint8_t).
>>
>> Signed-off-by: Bob Liu <bob.liu@oracle.com>
>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> ---
>>  xen/common/tmem.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/common/tmem.c b/xen/common/tmem.c
>> index f2dc26e..506c6be 100644
>> --- a/xen/common/tmem.c
>> +++ b/xen/common/tmem.c
>> @@ -399,7 +399,7 @@ static void pcd_disassociate(struct tmem_page_descriptor *pgp, struct tmem_pool
>>  {
>>      struct tmem_page_content_descriptor *pcd = pgp->pcd;
>>      struct page_info *pfp = pgp->pcd->pfp;
>> -    uint16_t firstbyte = pgp->firstbyte;
>> +    uint8_t firstbyte = pgp->firstbyte;
> 
> Actually looking at these CIDs, I think this is a coverity bug rather
> than a tmem bug.
> 

Thanks for you review!
Yes, I also think so.

> The two asserts
> 
> ASSERT(firstbyte != NOT_SHAREABLE); /* for NOT_SHAREABLE being
> (uint16_t)-1UL; */
> ASSERT(firstbyte < 256);
> 
> Cause the coverity analysis engine to decide:
> 
> "cond_const: Checking firstbyte != 65535 implies that firstbyte and
> pgp->firstbyte have the value 65535 on the false branch."
> 
> despite the fact that the second assert entirely covering the first. 
> Furthermore, I don't understand why the ASSERT() killpath isn't
> invalidating any analysis on the false branch of an ASSERT().
> 
> If you are changing uint16_t to uint8_t, you can drop those two asserts
> as well, as they become unconditionally true.
> 

Okay.

And since NOT_SHAREABLE has been checked before pcd_disassociate() every
time.

if ( tmem_dedup_enabled() && pgp->firstbyte != NOT_SHAREABLE )
    pcd_disassociate(pgp,pool,0); /* pgp->size lost */

I think the casting from uint16_t to uint8_t in pcd_disassociate() is safe.

>>      char *pcd_tze = pgp->pcd->tze;
>>      pagesize_t pcd_size = pcd->size;
>>      pagesize_t pgp_size = pgp->size;
>> @@ -1231,7 +1231,7 @@ static bool_t tmem_try_to_evict_pgp(struct tmem_page_descriptor *pgp, bool_t *ho
>>      struct tmem_object_root *obj = pgp->us.obj;
>>      struct tmem_pool *pool = obj->pool;
>>      struct client *client = pool->client;
>> -    uint16_t firstbyte = pgp->firstbyte;
>> +    uint8_t firstbyte = pgp->firstbyte;
>>  
>>      if ( pool->is_dying )
>>          return 0;
> 
> Given the "if ( firstbyte ==  NOT_SHAREABLE ) goto obj_unlock;", are you
> certain this change is safe?
> 

No, it's unsafe here. I think we can use pgp->firstbyte directly here.

How about this one?

>From 91469a2d85d0145d06fa048017d25d273ce4c0dc Mon Sep 17 00:00:00 2001
From: Bob Liu <bob.liu@oracle.com>
Date: Sun, 4 May 2014 15:59:09 +0800
Subject: [PATCH v2 4/4] tmem: fix Out-of-bounds read reported by Coverity

CID 1198729, CID 1198730 and CID 1198734 complain about
"Out-of-bounds read".

This patch fixes them by casting the 'firstbyte' to (uint8_t), some
unnecessary assertion also be dropped.

Signed-off-by: Bob Liu <bob.liu@oracle.com>
---
 xen/common/tmem.c |   11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/xen/common/tmem.c b/xen/common/tmem.c
index f2dc26e..93235c6 100644
--- a/xen/common/tmem.c
+++ b/xen/common/tmem.c
@@ -399,7 +399,7 @@ static void pcd_disassociate(struct
tmem_page_descriptor *pgp, struct tmem_pool
 {
     struct tmem_page_content_descriptor *pcd = pgp->pcd;
     struct page_info *pfp = pgp->pcd->pfp;
-    uint16_t firstbyte = pgp->firstbyte;
+    uint8_t firstbyte = pgp->firstbyte;
     char *pcd_tze = pgp->pcd->tze;
     pagesize_t pcd_size = pcd->size;
     pagesize_t pgp_size = pgp->size;
@@ -407,8 +407,6 @@ static void pcd_disassociate(struct
tmem_page_descriptor *pgp, struct tmem_pool
     pagesize_t pcd_csize = pgp->pcd->size;

     ASSERT(tmem_dedup_enabled());
-    ASSERT(firstbyte != NOT_SHAREABLE);
-    ASSERT(firstbyte < 256);

     if ( have_pcd_rwlock )
         ASSERT_WRITELOCK(&pcd_tree_rwlocks[firstbyte]);
@@ -1231,7 +1229,7 @@ static bool_t tmem_try_to_evict_pgp(struct
tmem_page_descriptor *pgp, bool_t *ho
     struct tmem_object_root *obj = pgp->us.obj;
     struct tmem_pool *pool = obj->pool;
     struct client *client = pool->client;
-    uint16_t firstbyte = pgp->firstbyte;
+    uint8_t firstbyte = pgp->firstbyte;

     if ( pool->is_dying )
         return 0;
@@ -1239,10 +1237,9 @@ static bool_t tmem_try_to_evict_pgp(struct
tmem_page_descriptor *pgp, bool_t *ho
     {
         if ( tmem_dedup_enabled() )
         {
-            firstbyte = pgp->firstbyte;
-            if ( firstbyte ==  NOT_SHAREABLE )
+            if ( pgp->firstbyte ==  NOT_SHAREABLE )
                 goto obj_unlock;
-            ASSERT(firstbyte < 256);
+            firstbyte = pgp->firstbyte;
             if ( !write_trylock(&pcd_tree_rwlocks[firstbyte]) )
                 goto obj_unlock;
             if ( pgp->pcd->pgp_ref_count > 1 && !pgp->eviction_attempted )
-- 
1.7.10.4

Re: [PATCH 4/4] tmem: fix Out-of-bounds read reported by Coverity

[Andrew Cooper]

On 04/05/2014 09:10, Bob Liu wrote:
> Hi Andrew,
>
> On 05/01/2014 05:04 AM, Andrew Cooper wrote:
>> On 30/04/2014 21:29, Konrad Rzeszutek Wilk wrote:
>>> From: Bob Liu <bob.liu@oracle.com>
>>>
>>> CID 1198729, CID 1198730 and CID 1198734 complain about
>>> "Out-of-bounds read".
>>>
>>> This patch fixes them by casting the 'firstbyte' to (uint8_t).
>>>
>>> Signed-off-by: Bob Liu <bob.liu@oracle.com>
>>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>> ---
>>>  xen/common/tmem.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/common/tmem.c b/xen/common/tmem.c
>>> index f2dc26e..506c6be 100644
>>> --- a/xen/common/tmem.c
>>> +++ b/xen/common/tmem.c
>>> @@ -399,7 +399,7 @@ static void pcd_disassociate(struct tmem_page_descriptor *pgp, struct tmem_pool
>>>  {
>>>      struct tmem_page_content_descriptor *pcd = pgp->pcd;
>>>      struct page_info *pfp = pgp->pcd->pfp;
>>> -    uint16_t firstbyte = pgp->firstbyte;
>>> +    uint8_t firstbyte = pgp->firstbyte;
>> Actually looking at these CIDs, I think this is a coverity bug rather
>> than a tmem bug.
>>
> Thanks for you review!
> Yes, I also think so.
>
>> The two asserts
>>
>> ASSERT(firstbyte != NOT_SHAREABLE); /* for NOT_SHAREABLE being
>> (uint16_t)-1UL; */
>> ASSERT(firstbyte < 256);
>>
>> Cause the coverity analysis engine to decide:
>>
>> "cond_const: Checking firstbyte != 65535 implies that firstbyte and
>> pgp->firstbyte have the value 65535 on the false branch."
>>
>> despite the fact that the second assert entirely covering the first. 
>> Furthermore, I don't understand why the ASSERT() killpath isn't
>> invalidating any analysis on the false branch of an ASSERT().
>>
>> If you are changing uint16_t to uint8_t, you can drop those two asserts
>> as well, as they become unconditionally true.
>>
> Okay.
>
> And since NOT_SHAREABLE has been checked before pcd_disassociate() every
> time.
>
> if ( tmem_dedup_enabled() && pgp->firstbyte != NOT_SHAREABLE )
>     pcd_disassociate(pgp,pool,0); /* pgp->size lost */
>
> I think the casting from uint16_t to uint8_t in pcd_disassociate() is safe.
>
>>>      char *pcd_tze = pgp->pcd->tze;
>>>      pagesize_t pcd_size = pcd->size;
>>>      pagesize_t pgp_size = pgp->size;
>>> @@ -1231,7 +1231,7 @@ static bool_t tmem_try_to_evict_pgp(struct tmem_page_descriptor *pgp, bool_t *ho
>>>      struct tmem_object_root *obj = pgp->us.obj;
>>>      struct tmem_pool *pool = obj->pool;
>>>      struct client *client = pool->client;
>>> -    uint16_t firstbyte = pgp->firstbyte;
>>> +    uint8_t firstbyte = pgp->firstbyte;
>>>  
>>>      if ( pool->is_dying )
>>>          return 0;
>> Given the "if ( firstbyte ==  NOT_SHAREABLE ) goto obj_unlock;", are you
>> certain this change is safe?
>>
> No, it's unsafe here. I think we can use pgp->firstbyte directly here.
>
> How about this one?
>
> From 91469a2d85d0145d06fa048017d25d273ce4c0dc Mon Sep 17 00:00:00 2001
> From: Bob Liu <bob.liu@oracle.com>
> Date: Sun, 4 May 2014 15:59:09 +0800
> Subject: [PATCH v2 4/4] tmem: fix Out-of-bounds read reported by Coverity
>
> CID 1198729, CID 1198730 and CID 1198734 complain about
> "Out-of-bounds read".
>
> This patch fixes them by casting the 'firstbyte' to (uint8_t), some
> unnecessary assertion also be dropped.
>
> Signed-off-by: Bob Liu <bob.liu@oracle.com>

That looks better.  Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
>  xen/common/tmem.c |   11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/xen/common/tmem.c b/xen/common/tmem.c
> index f2dc26e..93235c6 100644
> --- a/xen/common/tmem.c
> +++ b/xen/common/tmem.c
> @@ -399,7 +399,7 @@ static void pcd_disassociate(struct
> tmem_page_descriptor *pgp, struct tmem_pool
>  {
>      struct tmem_page_content_descriptor *pcd = pgp->pcd;
>      struct page_info *pfp = pgp->pcd->pfp;
> -    uint16_t firstbyte = pgp->firstbyte;
> +    uint8_t firstbyte = pgp->firstbyte;
>      char *pcd_tze = pgp->pcd->tze;
>      pagesize_t pcd_size = pcd->size;
>      pagesize_t pgp_size = pgp->size;
> @@ -407,8 +407,6 @@ static void pcd_disassociate(struct
> tmem_page_descriptor *pgp, struct tmem_pool
>      pagesize_t pcd_csize = pgp->pcd->size;
>
>      ASSERT(tmem_dedup_enabled());
> -    ASSERT(firstbyte != NOT_SHAREABLE);
> -    ASSERT(firstbyte < 256);
>
>      if ( have_pcd_rwlock )
>          ASSERT_WRITELOCK(&pcd_tree_rwlocks[firstbyte]);
> @@ -1231,7 +1229,7 @@ static bool_t tmem_try_to_evict_pgp(struct
> tmem_page_descriptor *pgp, bool_t *ho
>      struct tmem_object_root *obj = pgp->us.obj;
>      struct tmem_pool *pool = obj->pool;
>      struct client *client = pool->client;
> -    uint16_t firstbyte = pgp->firstbyte;
> +    uint8_t firstbyte = pgp->firstbyte;
>
>      if ( pool->is_dying )
>          return 0;
> @@ -1239,10 +1237,9 @@ static bool_t tmem_try_to_evict_pgp(struct
> tmem_page_descriptor *pgp, bool_t *ho
>      {
>          if ( tmem_dedup_enabled() )
>          {
> -            firstbyte = pgp->firstbyte;
> -            if ( firstbyte ==  NOT_SHAREABLE )
> +            if ( pgp->firstbyte ==  NOT_SHAREABLE )
>                  goto obj_unlock;
> -            ASSERT(firstbyte < 256);
> +            firstbyte = pgp->firstbyte;
>              if ( !write_trylock(&pcd_tree_rwlocks[firstbyte]) )
>                  goto obj_unlock;
>              if ( pgp->pcd->pgp_ref_count > 1 && !pgp->eviction_attempted )