diff options
author | Ian Jackson <ian.jackson@eu.citrix.com> | 2013-06-14 16:39:36 +0100 |
---|---|---|
committer | Ian Jackson <Ian.Jackson@eu.citrix.com> | 2013-06-14 16:39:36 +0100 |
commit | a004800f8fc607b96527815c8e3beabcb455d8e0 (patch) | |
tree | bedfb7dc80066010059b5ffd17b8affdb138b8cd /tools | |
parent | 7a549a6aa04dba807f8dd4c1577ab6a7592c4c76 (diff) | |
download | xen-a004800f8fc607b96527815c8e3beabcb455d8e0.tar.gz xen-a004800f8fc607b96527815c8e3beabcb455d8e0.tar.bz2 xen-a004800f8fc607b96527815c8e3beabcb455d8e0.zip |
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.
Diffstat (limited to 'tools')
-rw-r--r-- | tools/libxc/Makefile | 9 | ||||
-rw-r--r-- | tools/libxc/xc_dom.h | 7 | ||||
-rw-r--r-- | tools/libxc/xc_dom_elfloader.c | 42 | ||||
-rw-r--r-- | tools/xcutils/readnotes.c | 15 |
4 files changed, 42 insertions, 31 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); diff --git a/tools/xcutils/readnotes.c b/tools/xcutils/readnotes.c index d1f7a30f73..2ca7732e2d 100644 --- a/tools/xcutils/readnotes.c +++ b/tools/xcutils/readnotes.c @@ -70,7 +70,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); @@ -79,7 +79,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. */ @@ -99,10 +99,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) ) @@ -161,7 +161,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; } } @@ -171,12 +171,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; struct setup_header *hdr; uint64_t payload_offset, payload_length; |