Xen devel - Nov 2013 - [PATCH 0/5] Coverity fixes for misc tools

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

-- 
1.7.10.4

If the console or store mfn chunks are not present in the migration stream,
stack junk gets reported for the mfns.

XenServer had a very hard to track down VM corruption issue caused by exactly
this issue.  Xenconsoled would connect to a junk mfn and incremented the ring
pointer if the junk happend to look like a valid gfn.

Coverity ID: 1056093 1056094

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/xcutils/xc_restore.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/xcutils/xc_restore.c b/tools/xcutils/xc_restore.c
index 5ec90ac..4ea261b 100644
--- a/tools/xcutils/xc_restore.c
+++ b/tools/xcutils/xc_restore.c
@@ -23,7 +23,7 @@ main(int argc, char **argv)
     xc_interface *xch;
     int io_fd, ret;
     int superpages;
-    unsigned long store_mfn, console_mfn;
+    unsigned long store_mfn = 0, console_mfn = 0;
     xentoollog_level lvl;
     xentoollog_logger *l;
 
-- 
1.7.10.4

Coverity ID: 1126110

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/xentrace/xenctx.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/xentrace/xenctx.c b/tools/xentrace/xenctx.c
index 57e3277..ba502fd 100644
--- a/tools/xentrace/xenctx.c
+++ b/tools/xentrace/xenctx.c
@@ -208,13 +208,16 @@ static void read_symbol_table(const char *symtab)
             continue;
         default:
             symbol = malloc(sizeof(*symbol));
-            if (symbol == NULL)
+            if (symbol == NULL) {
+                fclose(f);
                 return;
+            }
 
             symbol->address = address;
             symbol->name = strdup(p);
             if (symbol->name == NULL) {
                 free(symbol);
+                fclose(f);
                 return;
             }
 
-- 
1.7.10.4

Coverity ID 1055563

In the original function, mmap() could be called with a length of -1 if the
second lseek failed and the caller had not provided max_size.

While fixing up this error, improve the logging of other error paths.  I know
from personal experience that debugging failures function is rather difficult
given only "xc_dom_malloc_filemap: failed (on file <somefile>)"
in the logs.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxc/xc_dom_core.c |   35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
index 705694a..77a4e64 100644
--- a/tools/libxc/xc_dom_core.c
+++ b/tools/libxc/xc_dom_core.c
@@ -176,13 +176,25 @@ void *xc_dom_malloc_filemap(struct xc_dom_image *dom,
 {
     struct xc_dom_mem *block = NULL;
     int fd = -1;
+    off_t offset;
 
     fd = open(filename, O_RDONLY);
-    if ( fd == -1 )
+    if ( fd == -1 ) {
+        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
+                     "failed to open file: %s",
+                     strerror(errno));
+        goto err;
+    }
+
+    if ( (lseek(fd, 0, SEEK_SET) == -1) ||
+         ((offset = lseek(fd, 0, SEEK_END)) == -1) ) {
+        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
+                     "failed to seek on file: %s",
+                     strerror(errno));
         goto err;
+    }
 
-    lseek(fd, 0, SEEK_SET);
-    *size = lseek(fd, 0, SEEK_END);
+    *size = offset;
 
     if ( max_size && *size > max_size )
     {
@@ -192,14 +204,24 @@ void *xc_dom_malloc_filemap(struct xc_dom_image *dom,
     }
 
     block = malloc(sizeof(*block));
-    if ( block == NULL )
+    if ( block == NULL ) {
+        xc_dom_panic(dom->xch, XC_OUT_OF_MEMORY,
+                     "failed to allocate block (%zu bytes)",
+                     sizeof(*block));
         goto err;
+    }
+
     memset(block, 0, sizeof(*block));
     block->mmap_len = *size;
     block->mmap_ptr = mmap(NULL, block->mmap_len, PROT_READ,
                            MAP_SHARED, fd, 0);
-    if ( block->mmap_ptr == MAP_FAILED )
+    if ( block->mmap_ptr == MAP_FAILED ) {
+        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
+                     "failed to mmap file: %s",
+                     strerror(errno));
         goto err;
+    }
+
     block->next = dom->memblocks;
     dom->memblocks = block;
     dom->alloc_malloc += sizeof(*block);
@@ -212,8 +234,7 @@ void *xc_dom_malloc_filemap(struct xc_dom_image *dom,
  err:
     if ( fd != -1 )
         close(fd);
-    if ( block != NULL )
-        free(block);
+    free(block);
     DOMPRINTF("%s: failed (on file `%s'')", __FUNCTION__,
filename);
     return NULL;
 }
-- 
1.7.10.4

Coverity ID 1090361 1090362 1090363

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/misc/xen-mfndump.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/misc/xen-mfndump.c b/tools/misc/xen-mfndump.c
index ce73e5b..88cfafa 100644
--- a/tools/misc/xen-mfndump.c
+++ b/tools/misc/xen-mfndump.c
@@ -240,7 +240,8 @@ int dump_ptes_func(int argc, char *argv[])
            domid, pfn, minfo.p2m_table[pfn]);
 
  out:
-    munmap(page, PAGE_SIZE);
+    if ( page )
+        munmap(page, PAGE_SIZE);
     xc_unmap_domain_meminfo(xch, &minfo);
     munmap(m2p_table, M2P_SIZE(max_mfn));
     return rc;
@@ -359,8 +360,10 @@ int memcmp_mfns_func(int argc, char *argv[])
     printf("  memcpy(1, 2) = %d\n", memcmp(page1, page2, PAGE_SIZE));
 
  out:
-    munmap(page1, PAGE_SIZE);
-    munmap(page2, PAGE_SIZE);
+    if ( page1 )
+        munmap(page1, PAGE_SIZE);
+    if ( page2 )
+        munmap(page2, PAGE_SIZE);
     return rc;
 }
 
-- 
1.7.10.4

Coverity ID: 1090375

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/misc/xen-mfndump.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/misc/xen-mfndump.c b/tools/misc/xen-mfndump.c
index 88cfafa..dea9fa9 100644
--- a/tools/misc/xen-mfndump.c
+++ b/tools/misc/xen-mfndump.c
@@ -30,7 +30,8 @@ int help_func(int argc, char *argv[])
 
 int dump_m2p_func(int argc, char *argv[])
 {
-    unsigned long i, max_mfn;
+    unsigned long i;
+    long max_mfn;
     xen_pfn_t *m2p_table;
 
     if ( argc > 0 )
@@ -41,6 +42,12 @@ int dump_m2p_func(int argc, char *argv[])
 
     /* Map M2P and obtain gpfn */
     max_mfn = xc_maximum_ram_page(xch);
+    if ( max_mfn < 0 )
+    {
+        ERROR("Failed to get the maximum mfn");
+        return -1;
+    }
+
     if ( !(m2p_table = xc_map_m2p(xch, max_mfn, PROT_READ, NULL)) )
     {
         ERROR("Failed to map live M2P table");
-- 
1.7.10.4

Andrew Cooper writes ("[PATCH 1/5] tools/xc_restore: Initialise console and
store mfns"):
> If the console or store mfn chunks are not present in the migration stream,
> stack junk gets reported for the mfns.
> 
> XenServer had a very hard to track down VM corruption issue caused by
exactly
> this issue.  Xenconsoled would connect to a junk mfn and incremented the
ring
> pointer if the junk happend to look like a valid gfn.
> 
> Coverity ID: 1056093 1056094
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Andrew Cooper writes ("[PATCH 2/5] tools/xenctx: Prevent leaking a file
handle on error paths"):
> Coverity ID: 1126110
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Andrew Cooper writes ("[PATCH 1/5] tools/xc_restore: Initialise console and
store mfns"):
> If the console or store mfn chunks are not present in the migration stream,
> stack junk gets reported for the mfns.
> 
> XenServer had a very hard to track down VM corruption issue caused by
exactly
> this issue.  Xenconsoled would connect to a junk mfn and incremented the
ring
> pointer if the junk happend to look like a valid gfn.
A question that arises here is this:

How come this was going undetected ?

If the junk mfn _doesn''t_ look like a valid gfn (which presumably it
mostly doesn''t), surely this generates some error message somewhere
(even if it only causes the console not to work) ?

Ian.

Andrew Cooper writes ("[PATCH 3/5] tools/libxc: Improve
xc_dom_malloc_filemap() error handling"):
> Coverity ID 1055563
> 
> In the original function, mmap() could be called with a length of -1 if the
> second lseek failed and the caller had not provided max_size.
> 
> While fixing up this error, improve the logging of other error paths.  I
know
> from personal experience that debugging failures function is rather
difficult
> given only "xc_dom_malloc_filemap: failed (on file
<somefile>)" in the logs.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Andrew Cooper writes ("[PATCH 5/5] tools/xen-mfndump: Avoid using -ERROR as
an upper loop bound"):
> Coverity ID: 1090375
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Andrew Cooper writes ("[PATCH 4/5] tools/xen-mfndump: Avoid munmap(NULL,
PAGE_SIZE) on certain error paths"):
> Coverity ID 1090361 1090362 1090363
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Andrew Cooper writes ("[PATCH 1/5] tools/xc_restore: Initialise console and
store mfns"):
> If the console or store mfn chunks are not present in the migration stream,
> stack junk gets reported for the mfns.
> 
> XenServer had a very hard to track down VM corruption issue caused by
exactly
> this issue.  Xenconsoled would connect to a junk mfn and incremented the
ring
> pointer if the junk happend to look like a valid gfn.
Andrew Cooper writes ("[PATCH 2/5] tools/xenctx: Prevent leaking a file
handle on error paths"):
> Coverity ID: 1126110
Andrew Cooper writes ("[PATCH 3/5] tools/libxc: Improve
xc_dom_malloc_filemap() error handling"):
> In the original function, mmap() could be called with a length of -1 if the
> second lseek failed and the caller had not provided max_size.
> 
> While fixing up this error, improve the logging of other error paths.  I
know
> from personal experience that debugging failures function is rather
difficult
> given only "xc_dom_malloc_filemap: failed (on file
<somefile>)" in the logs.
I have queued these three for backport.

Ian.

On 25/11/13 12:12, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH 1/5] tools/xc_restore: Initialise
console and store mfns"):
>> If the console or store mfn chunks are not present in the migration
stream,
>> stack junk gets reported for the mfns.
>>
>> XenServer had a very hard to track down VM corruption issue caused by
exactly
>> this issue.  Xenconsoled would connect to a junk mfn and incremented
the ring
>> pointer if the junk happend to look like a valid gfn.
> A question that arises here is this:
>
> How come this was going undetected ?
>
> If the junk mfn _doesn''t_ look like a valid gfn (which presumably
it
> mostly doesn''t), surely this generates some error message
somewhere
> (even if it only causes the console not to work) ?
>
> Ian.
This was a XenServer optimisation to avoid allocating an event channel
in dom0 for hvm domains which wouldn''t be using it anyway.  We only
officially support Windows hvm domains, so this is a blanket change for
all hvm domains.

We were not expecting any hvm consoles to work in the slightest, and
xenconsoled gave no hint of errors.

~Andrew