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.

Conflicts in 4.1 series:
 * xc_dom_load_elf_kernel has no rc variable to change.
 * elf_load_image doesn't exist.

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>

4 files changed, 41 insertions, 30 deletions

diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
index 6f5c3074b6..139b7913b8 100644
--- a/tools/libxc/Makefile
+++ b/tools/libxc/Makefile
@@ -49,8 +49,13 @@ GUEST_SRCS-$(CONFIG_HVM) += xc_hvm_build.c
 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 6b118ad61e..9dffbe6c1c 100644
--- a/tools/libxc/xc_dom.h
+++ b/tools/libxc/xc_dom.h
@@ -135,9 +135,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 c3da5b98b1..77b2e5b8d2 100644
--- a/tools/libxc/xc_dom_elfloader.c
+++ b/tools/libxc/xc_dom_elfloader.c
@@ -84,7 +84,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 )
     {
@@ -106,12 +106,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;
@@ -119,7 +119,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) )
     {
@@ -140,13 +140,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 )
@@ -157,15 +157,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;
@@ -174,22 +174,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;
 
     /*
@@ -209,7 +209,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));
 
@@ -255,7 +255,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);
@@ -294,10 +294,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 )
@@ -350,7 +354,7 @@ 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;
     xen_pfn_t pages;
diff --git a/tools/xcutils/readnotes.c b/tools/xcutils/readnotes.c
index b940a36722..543e0f5188 100644
--- a/tools/xcutils/readnotes.c
+++ b/tools/xcutils/readnotes.c
@@ -28,7 +28,7 @@ static void print_numeric_note(const char *prefix, struct elf_binary *elf,
 			       ELF_HANDLE_DECL(elf_note) note)
 {
 	uint64_t value = elf_note_numeric(elf, note);
-	int descsz = elf_uval(elf, note, descsz);
+	unsigned descsz = elf_uval(elf, note, descsz);
 
 	printf("%s: %#*" PRIx64 " (%d bytes)\n",
 	       prefix, 2+2*descsz, value, descsz);
@@ -37,7 +37,7 @@ static void print_numeric_note(const char *prefix, struct elf_binary *elf,
 static void print_l1_mfn_valid_note(const char *prefix, struct elf_binary *elf,
 				    ELF_HANDLE_DECL(elf_note) note)
 {
-	int descsz = elf_uval(elf, note, descsz);
+	unsigned descsz = elf_uval(elf, note, descsz);
 	ELF_PTRVAL_CONST_VOID desc = elf_note_desc(elf, note);
 
 	/* XXX should be able to cope with a list of values. */
@@ -57,10 +57,10 @@ static void print_l1_mfn_valid_note(const char *prefix, struct elf_binary *elf,
 
 }
 
-static int print_notes(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) start, ELF_HANDLE_DECL(elf_note) end)
+static unsigned print_notes(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) start, ELF_HANDLE_DECL(elf_note) end)
 {
 	ELF_HANDLE_DECL(elf_note) note;
-	int notes_found = 0;
+	unsigned notes_found = 0;
 	const char *this_note_name;
 
 	for ( note = start; ELF_HANDLE_PTRVAL(note) < ELF_HANDLE_PTRVAL(end); note = elf_note_next(elf, note) )
@@ -119,7 +119,7 @@ static int print_notes(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) start,
 			break;
 		default:
 			printf("unknown note type %#x\n",
-			       (int)elf_uval(elf, note, type));
+			       (unsigned)elf_uval(elf, note, type));
 			break;
 		}
 	}
@@ -129,12 +129,13 @@ static int print_notes(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) start,
 int main(int argc, char **argv)
 {
 	const char *f;
-	int fd,h,size,usize,count;
+	int fd;
+	unsigned h,size,usize,count;
 	void *image,*tmp;
 	struct stat st;
 	struct elf_binary elf;
 	ELF_HANDLE_DECL(elf_shdr) shdr;
-	int notes_found = 0;
+	unsigned notes_found = 0;
 
 	if (argc != 2)
 	{