libelf: use only unsigned integers

Signed integers have undesirable undefined behaviours on overflow.
Malicious compilers can turn apparently-correct code into code with
security vulnerabilities etc.

So use only unsigned integers.  Exceptions are booleans (which we have
already changed) and error codes.

We _do_ change all the chars which aren't fixed constants from our own
text segment, but not the char*s.  This is because it is safe to
access an arbitrary byte through a char*, but not necessarily safe to
convert an arbitrary value to a char.

As a consequence we need to compile libelf with -Wno-pointer-sign.

It is OK to change all the signed integers to unsigned because all the
inequalities in libelf are in contexts where we don't "expect"
negative numbers.

In libelf-dominfo.c:elf_xen_parse we rename a variable "rc" to
"more_notes" as it actually contains a note count derived from the
input image.  The "error" return value from elf_xen_parse_notes is
changed from -1 to ~0U.

grepping shows only one occurrence of "PRId" or "%d" or "%ld" in
libelf and xc_dom_elfloader.c (a "%d" which becomes "%u").

This is part of the fix to a security issue, XSA-55.

For those concerned about unintentional functional changes, the
following rune produces a version of the patch which is much smaller
and eliminates only non-functional changes:

 GIT_EXTERNAL_DIFF=.../unsigned-differ git-diff <before>..<after>

where <before> and <after> are git refs for the code before and after
this patch, and unsigned-differ is this shell script:

    #!/bin/bash
    set -e

    seddery () {
            perl -pe 's/\b(?:elf_errorstatus|elf_negerrnoval)\b/int/g'
    }

    path="$1"
    in="$2"
    out="$5"

    set +e
    diff -pu --label "$path~" <(seddery <"$in") --label "$path" <(seddery <"$out")
    rc=$?
    set -e
    if [ $rc = 1 ]; then rc=0; fi
    exit $rc

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

v8: Use "?!?!" to express consternation instead of a ruder phrase.

v5: Introduce ELF_NOTE_INVALID, instead of using a literal ~0U.

v4: Fix regression in elf_round_up; use uint64_t here.

v3: Changes to booleans split off into separate patch.

v2: BUGFIX: Eliminate conversion to int of return from elf_xen_parse_notes.
    BUGFIX: Fix the one printf format thing which needs changing.
    Remove irrelevant change to constify note_desc.name in libelf-dominfo.c.
    In xc_dom_load_elf_symtab change one sizeof(int) to sizeof(unsigned).
    Do not change type of 2nd argument to memset.
    Provide seddery for easier review.
    Style fix.

3 files changed, 34 insertions, 24 deletions

diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
index 4a31282996..512a994152 100644
--- a/tools/libxc/Makefile
+++ b/tools/libxc/Makefile
@@ -51,8 +51,13 @@ endif
 vpath %.c ../../xen/common/libelf
 CFLAGS += -I../../xen/common/libelf
 
-GUEST_SRCS-y += libelf-tools.c libelf-loader.c
-GUEST_SRCS-y += libelf-dominfo.c
+ELF_SRCS-y += libelf-tools.c libelf-loader.c
+ELF_SRCS-y += libelf-dominfo.c
+
+GUEST_SRCS-y += $(ELF_SRCS-y)
+
+$(patsubst %.c,%.o,$(ELF_SRCS-y)): CFLAGS += -Wno-pointer-sign
+$(patsubst %.c,%.opic,$(ELF_SRCS-y)): CFLAGS += -Wno-pointer-sign
 
 # new domain builder
 GUEST_SRCS-y                 += xc_dom_core.c xc_dom_boot.c
diff --git a/tools/libxc/xc_dom.h b/tools/libxc/xc_dom.h
index ad6fdd49e2..5968e7bc2c 100644
--- a/tools/libxc/xc_dom.h
+++ b/tools/libxc/xc_dom.h
@@ -155,9 +155,10 @@ struct xc_dom_image {
 
 struct xc_dom_loader {
     char *name;
-    int (*probe) (struct xc_dom_image * dom);
-    int (*parser) (struct xc_dom_image * dom);
-    int (*loader) (struct xc_dom_image * dom);
+    /* Sadly the error returns from these functions are not consistent: */
+    elf_negerrnoval (*probe) (struct xc_dom_image * dom);
+    elf_negerrnoval (*parser) (struct xc_dom_image * dom);
+    elf_errorstatus (*loader) (struct xc_dom_image * dom);
 
     struct xc_dom_loader *next;
 };
diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
index 8f9c2fbb3b..75e469a451 100644
--- a/tools/libxc/xc_dom_elfloader.c
+++ b/tools/libxc/xc_dom_elfloader.c
@@ -82,7 +82,7 @@ static char *xc_dom_guest_type(struct xc_dom_image *dom,
 /* ------------------------------------------------------------------------ */
 /* parse elf binary                                                         */
 
-static int check_elf_kernel(struct xc_dom_image *dom, bool verbose)
+static elf_negerrnoval check_elf_kernel(struct xc_dom_image *dom, bool verbose)
 {
     if ( dom->kernel_blob == NULL )
     {
@@ -104,12 +104,12 @@ static int check_elf_kernel(struct xc_dom_image *dom, bool verbose)
     return 0;
 }
 
-static int xc_dom_probe_elf_kernel(struct xc_dom_image *dom)
+static elf_negerrnoval xc_dom_probe_elf_kernel(struct xc_dom_image *dom)
 {
     return check_elf_kernel(dom, 0);
 }
 
-static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
+static elf_errorstatus xc_dom_load_elf_symtab(struct xc_dom_image *dom,
                                   struct elf_binary *elf, bool load)
 {
     struct elf_binary syms;
@@ -117,7 +117,7 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
     xen_vaddr_t symtab, maxaddr;
     ELF_PTRVAL_CHAR hdr;
     size_t size;
-    int h, count, type, i, tables = 0;
+    unsigned h, count, type, i, tables = 0;
 
     if ( elf_swap(elf) )
     {
@@ -138,13 +138,13 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
         elf->caller_xdest_base = hdr_ptr;
         elf->caller_xdest_size = allow_size;
         hdr = ELF_REALPTR2PTRVAL(hdr_ptr);
-        elf_store_val(elf, int, hdr, size - sizeof(int));
+        elf_store_val(elf, unsigned, hdr, size - sizeof(unsigned));
     }
     else
     {
         char *hdr_ptr;
 
-        size = sizeof(int) + elf_size(elf, elf->ehdr) +
+        size = sizeof(unsigned) + elf_size(elf, elf->ehdr) +
             elf_shdr_count(elf) * elf_size(elf, shdr);
         hdr_ptr = xc_dom_malloc(dom, size);
         if ( hdr_ptr == NULL )
@@ -155,15 +155,15 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
         dom->bsd_symtab_start = elf_round_up(elf, dom->kernel_seg.vend);
     }
 
-    elf_memcpy_safe(elf, hdr + sizeof(int),
+    elf_memcpy_safe(elf, hdr + sizeof(unsigned),
            ELF_IMAGE_BASE(elf),
            elf_size(elf, elf->ehdr));
-    elf_memcpy_safe(elf, hdr + sizeof(int) + elf_size(elf, elf->ehdr),
+    elf_memcpy_safe(elf, hdr + sizeof(unsigned) + elf_size(elf, elf->ehdr),
            ELF_IMAGE_BASE(elf) + elf_uval(elf, elf->ehdr, e_shoff),
            elf_shdr_count(elf) * elf_size(elf, shdr));
     if ( elf_64bit(elf) )
     {
-        Elf64_Ehdr *ehdr = (Elf64_Ehdr *)(hdr + sizeof(int));
+        Elf64_Ehdr *ehdr = (Elf64_Ehdr *)(hdr + sizeof(unsigned));
         ehdr->e_phoff = 0;
         ehdr->e_phentsize = 0;
         ehdr->e_phnum = 0;
@@ -172,22 +172,22 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
     }
     else
     {
-        Elf32_Ehdr *ehdr = (Elf32_Ehdr *)(hdr + sizeof(int));
+        Elf32_Ehdr *ehdr = (Elf32_Ehdr *)(hdr + sizeof(unsigned));
         ehdr->e_phoff = 0;
         ehdr->e_phentsize = 0;
         ehdr->e_phnum = 0;
         ehdr->e_shoff = elf_size(elf, elf->ehdr);
         ehdr->e_shstrndx = SHN_UNDEF;
     }
-    if ( elf->caller_xdest_size < sizeof(int) )
+    if ( elf->caller_xdest_size < sizeof(unsigned) )
     {
         DOMPRINTF("%s/%s: header size %"PRIx64" too small",
                   __FUNCTION__, load ? "load" : "parse",
                   (uint64_t)elf->caller_xdest_size);
         return -1;
     }
-    if ( elf_init(&syms, elf->caller_xdest_base + sizeof(int),
-                  elf->caller_xdest_size - sizeof(int)) )
+    if ( elf_init(&syms, elf->caller_xdest_base + sizeof(unsigned),
+                  elf->caller_xdest_size - sizeof(unsigned)) )
         return -1;
 
     /*
@@ -207,7 +207,7 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
 
     xc_elf_set_logfile(dom->xch, &syms, 1);
 
-    symtab = dom->bsd_symtab_start + sizeof(int);
+    symtab = dom->bsd_symtab_start + sizeof(unsigned);
     maxaddr = elf_round_up(&syms, symtab + elf_size(&syms, syms.ehdr) +
                            elf_shdr_count(&syms) * elf_size(&syms, shdr));
 
@@ -253,7 +253,7 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
             size = elf_uval(&syms, shdr, sh_size);
             maxaddr = elf_round_up(&syms, maxaddr + size);
             tables++;
-            DOMPRINTF("%s: h=%d %s, size=0x%zx, maxaddr=0x%" PRIx64 "",
+            DOMPRINTF("%s: h=%u %s, size=0x%zx, maxaddr=0x%" PRIx64 "",
                       __FUNCTION__, h,
                       type == SHT_SYMTAB ? "symtab" : "strtab",
                       size, maxaddr);
@@ -292,10 +292,14 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
     return 0;
 }
 
-static int xc_dom_parse_elf_kernel(struct xc_dom_image *dom)
+static elf_errorstatus xc_dom_parse_elf_kernel(struct xc_dom_image *dom)
+    /*
+     * This function sometimes returns -1 for error and sometimes
+     * an errno value.  ?!?!
+     */
 {
     struct elf_binary *elf;
-    int rc;
+    elf_errorstatus rc;
 
     rc = check_elf_kernel(dom, 1);
     if ( rc != 0 )
@@ -356,10 +360,10 @@ out:
     return rc;
 }
 
-static int xc_dom_load_elf_kernel(struct xc_dom_image *dom)
+static elf_errorstatus xc_dom_load_elf_kernel(struct xc_dom_image *dom)
 {
     struct elf_binary *elf = dom->private_loader;
-    int rc;
+    elf_errorstatus rc;
     xen_pfn_t pages;
 
     elf->dest_base = xc_dom_seg_to_ptr_pages(dom, &dom->kernel_seg, &pages);