diff options
author | Ian Jackson <ian.jackson@eu.citrix.com> | 2013-06-14 16:45:38 +0100 |
---|---|---|
committer | Ian Jackson <Ian.Jackson@eu.citrix.com> | 2013-06-14 16:45:38 +0100 |
commit | 39923542bb43e67776c4e8292d4a5a1adef2bd3b (patch) | |
tree | ba226e85816a69ccc08f353c20b3dc60a18149fd /tools | |
parent | 8ce60b35beaac91a97b79c004ca6bf5d58e7390b (diff) | |
download | xen-39923542bb43e67776c4e8292d4a5a1adef2bd3b.tar.gz xen-39923542bb43e67776c4e8292d4a5a1adef2bd3b.tar.bz2 xen-39923542bb43e67776c4e8292d4a5a1adef2bd3b.zip |
libelf: check all pointer accesses
We change the ELF_PTRVAL and ELF_HANDLE types and associated macros:
* PTRVAL becomes a uintptr_t, for which we provide a typedef
elf_ptrval. This means no arithmetic done on it can overflow so
the compiler cannot do any malicious invalid pointer arithmetic
"optimisations". It also means that any places where we
dereference one of these pointers without using the appropriate
macros or functions become a compilation error.
So we can be sure that we won't miss any memory accesses.
All the PTRVAL variables were previously void* or char*, so
the actual address calculations are unchanged.
* ELF_HANDLE becomes a union, one half of which keeps the pointer
value and the other half of which is just there to record the
type.
The new type is not a pointer type so there can be no address
calculations on it whose meaning would change. Every assignment or
access has to go through one of our macros.
* The distinction between const and non-const pointers and char*s
and void*s in libelf goes away. This was not important (and
anyway libelf tended to cast away const in various places).
* The fields elf->image and elf->dest are renamed. That proves
that we haven't missed any unchecked uses of these actual
pointer values.
* The caller may fill in elf->caller_xdest_base and _size to
specify another range of memory which is safe for libelf to
access, besides the input and output images.
* When accesses fail due to being out of range, we mark the elf
"broken". This will be checked and used for diagnostics in
a following patch.
We do not check for write accesses to the input image. This is
because libelf actually does this in a number of places. So we
simply permit that.
* Each caller of libelf which used to set dest now sets
dest_base and dest_size.
* In xc_dom_load_elf_symtab we provide a new actual-pointer
value hdr_ptr which we get from mapping the guest's kernel
area and use (checking carefully) as the caller_xdest area.
* The STAR(h) macro in libelf-dominfo.c now uses elf_access_unsigned.
* elf-init uses the new elf_uval_3264 accessor to access the 32-bit
fields, rather than an unchecked field access (ie, unchecked
pointer access).
* elf_uval has been reworked to use elf_uval_3264. Both of these
macros are essentially new in this patch (although they are derived
from the old elf_uval) and need careful review.
* ELF_ADVANCE_DEST is now safe in the sense that you can use it to
chop parts off the front of the dest area but if you chop more than
is available, the dest area is simply set to be empty, preventing
future accesses.
* We introduce some #defines for memcpy, memset, memmove and strcpy:
- We provide elf_memcpy_safe and elf_memset_safe which take
PTRVALs and do checking on the supplied pointers.
- Users inside libelf must all be changed to either
elf_mem*_unchecked (which are just like mem*), or
elf_mem*_safe (which take PTRVALs) and are checked. Any
unchanged call sites become compilation errors.
* We do _not_ at this time fix elf_access_unsigned so that it doesn't
make unaligned accesses. We hope that unaligned accesses are OK on
every supported architecture. But it does check the supplied
pointer for validity.
This is part of the fix to a security issue, XSA-55.
Additional change in 4.1 backport:
* ELF_PRPTRVAL needs to be defined oddly on 4.1 and earlier because
Xen's headers provide no definitions of uintptr_t or PRIuPTR.
Conflicts:
* Callers of elf_load_binary don't check its return value in 4.1.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Diffstat (limited to 'tools')
-rw-r--r-- | tools/libxc/xc_dom_elfloader.c | 49 | ||||
-rw-r--r-- | tools/libxc/xc_hvm_build.c | 10 |
2 files changed, 48 insertions, 11 deletions
diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c index 0fd8c42326..b10790a537 100644 --- a/tools/libxc/xc_dom_elfloader.c +++ b/tools/libxc/xc_dom_elfloader.c @@ -130,20 +130,30 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom, if ( load ) { - size_t allow_size; /* will be used in a forthcoming XSA-55 patch */ + char *hdr_ptr; + size_t allow_size; + if ( !dom->bsd_symtab_start ) return 0; size = dom->kernel_seg.vend - dom->bsd_symtab_start; - hdr = xc_dom_vaddr_to_ptr(dom, dom->bsd_symtab_start, &allow_size); - *(int *)hdr = size - sizeof(int); + hdr_ptr = xc_dom_vaddr_to_ptr(dom, dom->bsd_symtab_start, &allow_size); + 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)); } else { + char *hdr_ptr; + size = sizeof(int) + elf_size(elf, elf->ehdr) + elf_shdr_count(elf) * elf_size(elf, shdr); - hdr = xc_dom_malloc(dom, size); - if ( hdr == NULL ) + hdr_ptr = xc_dom_malloc(dom, size); + if ( hdr_ptr == NULL ) return 0; + elf->caller_xdest_base = hdr_ptr; + elf->caller_xdest_size = size; + hdr = ELF_REALPTR2PTRVAL(hdr_ptr); dom->bsd_symtab_start = elf_round_up(elf, dom->kernel_seg.vend); } @@ -171,9 +181,32 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom, ehdr->e_shoff = elf_size(elf, elf->ehdr); ehdr->e_shstrndx = SHN_UNDEF; } - if ( elf_init(&syms, hdr + sizeof(int), size - sizeof(int)) ) + if ( elf->caller_xdest_size < sizeof(int) ) + { + 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)) ) return -1; + /* + * The caller_xdest_{base,size} and dest_{base,size} need to + * remain valid so long as each struct elf_image does. The + * principle we adopt is that these values are set when the + * memory is allocated or mapped, and cleared when (and if) + * they are unmapped. + * + * Mappings of the guest are normally undone by xc_dom_unmap_all + * (directly or via xc_dom_release). We do not explicitly clear + * these because in fact that happens only at the end of + * xc_dom_boot_image, at which time all of these ELF loading + * functions have returned. No relevant struct elf_binary* + * escapes this file. + */ + xc_elf_set_logfile(dom->xch, &syms, 1); symtab = dom->bsd_symtab_start + sizeof(int); @@ -304,8 +337,10 @@ static int xc_dom_parse_elf_kernel(struct xc_dom_image *dom) static int xc_dom_load_elf_kernel(struct xc_dom_image *dom) { struct elf_binary *elf = dom->private_loader; + xen_pfn_t pages; - elf->dest = xc_dom_seg_to_ptr(dom, &dom->kernel_seg); + elf->dest_base = xc_dom_seg_to_ptr_pages(dom, &dom->kernel_seg, &pages); + elf->dest_size = pages * XC_DOM_PAGE_SIZE(dom); elf_load_binary(elf); if ( dom->parms.bsd_symtab ) xc_dom_load_elf_symtab(dom, elf, 1); diff --git a/tools/libxc/xc_hvm_build.c b/tools/libxc/xc_hvm_build.c index 4af08c7691..e3efca98e8 100644 --- a/tools/libxc/xc_hvm_build.c +++ b/tools/libxc/xc_hvm_build.c @@ -99,18 +99,20 @@ static int loadelfimage( for ( i = 0; i < pages; i++ ) entries[i].mfn = parray[(elf->pstart >> PAGE_SHIFT) + i]; - elf->dest = xc_map_foreign_ranges( + elf->dest_base = xc_map_foreign_ranges( xch, dom, pages << PAGE_SHIFT, PROT_READ | PROT_WRITE, 1 << PAGE_SHIFT, entries, pages); - if ( elf->dest == NULL ) + if ( elf->dest_base == NULL ) goto err; + elf->dest_size = pages * PAGE_SIZE; /* Load the initial elf image. */ elf_load_binary(elf); rc = 0; - munmap(elf->dest, pages << PAGE_SHIFT); - elf->dest = NULL; + munmap(elf->dest_base, pages << PAGE_SHIFT); + elf->dest_base = NULL; + elf->dest_size = 0; err: free(entries); |