This is version 8 of my series to try to fix libelf and the domain loader. I hope this will be the final version. Barring further showstoppers, and in the absence of objections, I intend to push this to xen.git#staging tomorrow, and include it in a revised version of the XSA-55 advisory. I think further problems with this code should be treated, as far as feasible, as new problems. In particular, further security bugs should be treated as new security bugs and go only to security@xen.org, and be handled with a predisclosure period. This is available via git: http://xenbits.xen.org/gitweb/?p=people/iwj/xen-unstable.git;a=summary git://xenbits.xen.org/people/iwj/xen-unstable.git in the commits xsa55-unstable-base-rebasing..xsa55-unstable-rebasing Here is a summary of the state of series: A 01 libelf: abolish libelf-relocate.c A 02 libxc: introduce xc_dom_seg_to_ptr_pages a 03 libxc: Fix range checking in xc_dom_pfn_to_ptr etc. A 04 libelf: add `struct elf_binary*'' parameter to elf_load_image A 05 libelf: abolish elf_sval and elf_access_signed A 06 libelf: move include of <asm/guest_access.h> to top of file A 07 libelf/xc_dom_load_elf_symtab: Do not use "syms" uninitialised A 08 libxl: introduce macros for memory access and pointer handling A 09 tools/xcutils/readnotes: adjust print_l1_mfn_valid_note A 10 libelf: check nul-terminated strings properly oa 11 libxl: check all pointer accesses A 12 libxl: Check pointer references in elf_is_elfbinary A 13 libelf: Make all callers call elf_check_broken A 14 libelf: use C99 bool for booleans A -15 libelf: use only unsigned integers a*16 libelf: check loops for running away A 17 libelf: abolish obsolete macros a 18 libxc: Add range checking to xc_dom_binloader a/19 libxc: check failure of xc_dom_*_to_ptr, xc_map_foreign_range a/20 libxc: check return values from malloc A 21 libxc: range checks in xc_dom_p2m_host and _guest A -22 libxc: check blob size before proceeding in xc_dom_check_gzip a+23 libxc: Better range check in xc_dom_alloc_segment Key to symbols: * Updated in this version of the series. + New patch in this version. / Updated but only to move changes between patches. - Updated with style, comment or commit message changes only. a Acked/reviwed by one reviewer. o Older version also acked (by other reviewers if applicable). A Acked/reviwed by more than one reviewer. Also in every patch: Updated commit msgs to correct email address for me. libelf, and some of its callers, did not do nearly enough checking on their input. Invalid inputs could cause signed integer arithmetic overflows and wild pointer dereferences. In this series we try to systematically eliminate this problem in a way which has a reasonable chance of (i) still accepting all previously-accepted ELF images (ii) not having remaining security bugs, in a form which can be reviewied to verify (i) and (ii). Additionally, we fix some related security bugs in the domain loading code. The approach is: (i) Remove all uses of signed integers (of any kind). That elmininates all integer overflows as sources of undefined behaviour. Of course it still means that we can get incorrect values throughout the code. (ii) Replace all uses of pointers, both pointers into the supplied ELF image, and pointers into the output (where we are loading) by uintptr_t. That eliminates all pointer arithmetic overflows as sources of undefined behaviour. Of course it still means that we can get incorrect and unreasonable "pointer" values. (iii) But these pointer values will be in uintptr_t, which cannot be simply dereferenced by mistake. We will replace all dereferences by macros which do range checking; out of range reads will read 0 and out of range writes will be ignored. Happily most (but not all) of the reads in the code already go through macros which abstract endianness[1] and/or 32/64bitness. [1] Although not all the accesses use endian-aware techniques so in fact the code can''t cope with foreign-endian ELFs. This is a problem for another day. (iv) Look for all loops and check that they are guaranteed to terminate. To enable verification of correctness of these changes I provide them as a series roughly as follows: 1-6: Pre-patches which make a few semantically neutral or semantically correct changes. For human review. 7: Introduces a set of macros to abstract away pointer arithmetic and input and output image memory accesses in libelf. Use these macros everwhere they are applicable. However, define the macros in a way that corresponds to the existing code. That this patch has no functional change can be verified by comparing the before-and-after assembler output from the compiler. 9. Introduce some macros for dealing with nul-terminated strings, defined so as not to have any functional change at this stage. 10. Change the macro definitions, and introduce the new pseudopointer types, pointer range checking, etc. For close human review. Each macro change is justified in the commit message. This patch eliminates most of the potential wild pointer accesses. 8,11-12,14-15: Smaller patches for human review, fixing some leftover bugs, including ensuring that all loops terminate. 13: Eliminate signed integers. Replace every "int", "int*_t", "long" and most "char"s by corresponding unsigned types. This eliminates all integer arithmetic overflows. After this patch, libelf should be safe against hostile input: * All arithmetic operations on values from the input file use unsigned arithmetic which is guaranteed to be defined (although it may of course result in wrong answers); * All pointer accesses based on pointers to locations which depend on the input file go via our range-checking accessors; accesses which are not to the input or output regions are ignored (reads returning 0). * The loops have been checked to ensure that they terminate and are at worst O(image_size). * Whenever an array variable was declared, the code has been manually reviewed looking for possible out-of-bounds accesses. This is XSA-55.
This file is not actually used. It''s not built in Xen''s instance of libelf; in libxc''s it''s built but nothing in it is called. Do not compile it in libxc, and delete it. This reduces the amount of work we need to do in forthcoming patches to libelf (particularly since as libelf-relocate.c is not used it is probably full of bugs). This is part of the fix to a security issue, XSA-55. Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> Acked-by: Ian Campbell <ian.campbell@citrix.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Chuck Anderson <chuck.anderson@oracle.com> --- tools/libxc/Makefile | 2 +- xen/common/libelf/libelf-relocate.c | 372 ----------------------------------- 2 files changed, 1 insertions(+), 373 deletions(-) delete mode 100644 xen/common/libelf/libelf-relocate.c diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile index b200123..4a31282 100644 --- a/tools/libxc/Makefile +++ b/tools/libxc/Makefile @@ -52,7 +52,7 @@ 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 libelf-relocate.c +GUEST_SRCS-y += libelf-dominfo.c # new domain builder GUEST_SRCS-y += xc_dom_core.c xc_dom_boot.c diff --git a/xen/common/libelf/libelf-relocate.c b/xen/common/libelf/libelf-relocate.c deleted file mode 100644 index 2aafc44..0000000 --- a/xen/common/libelf/libelf-relocate.c +++ /dev/null @@ -1,372 +0,0 @@ -/* - * ELF relocation code (not used by xen kernel right now). - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; - * version 2.1 of the License. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library; if not, write to the Free Software - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA - */ - -#include "libelf-private.h" - -/* ------------------------------------------------------------------------ */ - -static const char *rel_names_i386[] = { - "R_386_NONE", - "R_386_32", - "R_386_PC32", - "R_386_GOT32", - "R_386_PLT32", - "R_386_COPY", - "R_386_GLOB_DAT", - "R_386_JMP_SLOT", - "R_386_RELATIVE", - "R_386_GOTOFF", - "R_386_GOTPC", - "R_386_32PLT", - "R_386_TLS_TPOFF", - "R_386_TLS_IE", - "R_386_TLS_GOTIE", - "R_386_TLS_LE", - "R_386_TLS_GD", - "R_386_TLS_LDM", - "R_386_16", - "R_386_PC16", - "R_386_8", - "R_386_PC8", - "R_386_TLS_GD_32", - "R_386_TLS_GD_PUSH", - "R_386_TLS_GD_CALL", - "R_386_TLS_GD_POP", - "R_386_TLS_LDM_32", - "R_386_TLS_LDM_PUSH", - "R_386_TLS_LDM_CALL", - "R_386_TLS_LDM_POP", - "R_386_TLS_LDO_32", - "R_386_TLS_IE_32", - "R_386_TLS_LE_32", - "R_386_TLS_DTPMOD32", - "R_386_TLS_DTPOFF32", - "R_386_TLS_TPOFF32", -}; - -static int elf_reloc_i386(struct elf_binary *elf, int type, - uint64_t addr, uint64_t value) -{ - void *ptr = elf_get_ptr(elf, addr); - uint32_t *u32; - - switch ( type ) - { - case 1 /* R_386_32 */ : - u32 = ptr; - *u32 += elf->reloc_offset; - break; - case 2 /* R_386_PC32 */ : - /* nothing */ - break; - default: - return -1; - } - return 0; -} - -/* ------------------------------------------------------------------------ */ - -static const char *rel_names_x86_64[] = { - "R_X86_64_NONE", - "R_X86_64_64", - "R_X86_64_PC32", - "R_X86_64_GOT32", - "R_X86_64_PLT32", - "R_X86_64_COPY", - "R_X86_64_GLOB_DAT", - "R_X86_64_JUMP_SLOT", - "R_X86_64_RELATIVE", - "R_X86_64_GOTPCREL", - "R_X86_64_32", - "R_X86_64_32S", - "R_X86_64_16", - "R_X86_64_PC16", - "R_X86_64_8", - "R_X86_64_PC8", - "R_X86_64_DTPMOD64", - "R_X86_64_DTPOFF64", - "R_X86_64_TPOFF64", - "R_X86_64_TLSGD", - "R_X86_64_TLSLD", - "R_X86_64_DTPOFF32", - "R_X86_64_GOTTPOFF", - "R_X86_64_TPOFF32", -}; - -static int elf_reloc_x86_64(struct elf_binary *elf, int type, - uint64_t addr, uint64_t value) -{ - void *ptr = elf_get_ptr(elf, addr); - uint64_t *u64; - uint32_t *u32; - int32_t *s32; - - switch ( type ) - { - case 1 /* R_X86_64_64 */ : - u64 = ptr; - value += elf->reloc_offset; - *u64 = value; - break; - case 2 /* R_X86_64_PC32 */ : - u32 = ptr; - *u32 = value - addr; - if ( *u32 != (uint32_t)(value - addr) ) - { - elf_err(elf, "R_X86_64_PC32 overflow: 0x%" PRIx32 - " != 0x%" PRIx32 "\n", - *u32, (uint32_t) (value - addr)); - return -1; - } - break; - case 10 /* R_X86_64_32 */ : - u32 = ptr; - value += elf->reloc_offset; - *u32 = value; - if ( *u32 != value ) - { - elf_err(elf, "R_X86_64_32 overflow: 0x%" PRIx32 - " != 0x%" PRIx64 "\n", - *u32, value); - return -1; - } - break; - case 11 /* R_X86_64_32S */ : - s32 = ptr; - value += elf->reloc_offset; - *s32 = value; - if ( *s32 != (int64_t) value ) - { - elf_err(elf, "R_X86_64_32S overflow: 0x%" PRIx32 - " != 0x%" PRIx64 "\n", - *s32, (int64_t) value); - return -1; - } - break; - default: - return -1; - } - return 0; -} - -/* ------------------------------------------------------------------------ */ - -static struct relocs { - const char **names; - int count; - int (*func) (struct elf_binary * elf, int type, uint64_t addr, - uint64_t value); -} relocs[] -/* *INDENT-OFF* */ -{ - [EM_386] = { - .names = rel_names_i386, - .count = sizeof(rel_names_i386) / sizeof(rel_names_i386[0]), - .func = elf_reloc_i386, - }, - [EM_X86_64] = { - .names = rel_names_x86_64, - .count = sizeof(rel_names_x86_64) / sizeof(rel_names_x86_64[0]), - .func = elf_reloc_x86_64, - } -}; -/* *INDENT-ON* */ - -/* ------------------------------------------------------------------------ */ - -static const char *rela_name(int machine, int type) -{ - if ( machine > sizeof(relocs) / sizeof(relocs[0]) ) - return "unknown mach"; - if ( !relocs[machine].names ) - return "unknown mach"; - if ( type > relocs[machine].count ) - return "unknown rela"; - return relocs[machine].names[type]; -} - -static int elf_reloc_section(struct elf_binary *elf, - const elf_shdr * rels, - const elf_shdr * sect, const elf_shdr * syms) -{ - const void *ptr, *end; - const elf_shdr *shdr; - const elf_rela *rela; - const elf_rel *rel; - const elf_sym *sym; - uint64_t s_type; - uint64_t r_offset; - uint64_t r_info; - uint64_t r_addend; - int r_type, r_sym; - size_t rsize; - uint64_t shndx, sbase, addr, value; - const char *sname; - int machine; - - machine = elf_uval(elf, elf->ehdr, e_machine); - if ( (machine >= (sizeof(relocs) / sizeof(relocs[0]))) || - (relocs[machine].func == NULL) ) - { - elf_err(elf, "%s: can''t handle machine %d\n", - __FUNCTION__, machine); - return -1; - } - if ( elf_swap(elf) ) - { - elf_err(elf, "%s: non-native byte order, relocation not supported\n", - __FUNCTION__); - return -1; - } - - s_type = elf_uval(elf, rels, sh_type); - rsize = (SHT_REL == s_type) ? elf_size(elf, rel) : elf_size(elf, rela); - ptr = elf_section_start(elf, rels); - end = elf_section_end(elf, rels); - - for ( ; ptr < end; ptr += rsize ) - { - switch ( s_type ) - { - case SHT_REL: - rel = ptr; - r_offset = elf_uval(elf, rel, r_offset); - r_info = elf_uval(elf, rel, r_info); - r_addend = 0; - break; - case SHT_RELA: - rela = ptr; - r_offset = elf_uval(elf, rela, r_offset); - r_info = elf_uval(elf, rela, r_info); - r_addend = elf_uval(elf, rela, r_addend); - break; - default: - /* can''t happen */ - return -1; - } - if ( elf_64bit(elf) ) - { - r_type = ELF64_R_TYPE(r_info); - r_sym = ELF64_R_SYM(r_info); - } - else - { - r_type = ELF32_R_TYPE(r_info); - r_sym = ELF32_R_SYM(r_info); - } - - sym = elf_sym_by_index(elf, r_sym); - shndx = elf_uval(elf, sym, st_shndx); - switch ( shndx ) - { - case SHN_UNDEF: - sname = "*UNDEF*"; - sbase = 0; - break; - case SHN_COMMON: - elf_err(elf, "%s: invalid section: %" PRId64 "\n", - __FUNCTION__, shndx); - return -1; - case SHN_ABS: - sname = "*ABS*"; - sbase = 0; - break; - default: - shdr = elf_shdr_by_index(elf, shndx); - if ( shdr == NULL ) - { - elf_err(elf, "%s: invalid section: %" PRId64 "\n", - __FUNCTION__, shndx); - return -1; - } - sname = elf_section_name(elf, shdr); - sbase = elf_uval(elf, shdr, sh_addr); - } - - addr = r_offset; - value = elf_uval(elf, sym, st_value); - value += r_addend; - - if ( elf->log_callback && (elf->verbose > 1) ) - { - uint64_t st_name = elf_uval(elf, sym, st_name); - const char *name = st_name ? elf->sym_strtab + st_name : "*NONE*"; - - elf_msg(elf, - "%s: type %s [%d], off 0x%" PRIx64 ", add 0x%" PRIx64 "," - " sym %s [0x%" PRIx64 "], sec %s [0x%" PRIx64 "]" - " -> addr 0x%" PRIx64 " value 0x%" PRIx64 "\n", - __FUNCTION__, rela_name(machine, r_type), r_type, r_offset, - r_addend, name, elf_uval(elf, sym, st_value), sname, sbase, - addr, value); - } - - if ( relocs[machine].func(elf, r_type, addr, value) == -1 ) - { - elf_err(elf, "%s: unknown/unsupported reloc type %s [%d]\n", - __FUNCTION__, rela_name(machine, r_type), r_type); - return -1; - } - } - return 0; -} - -int elf_reloc(struct elf_binary *elf) -{ - const elf_shdr *rels, *sect, *syms; - uint64_t i, count, type; - - count = elf_shdr_count(elf); - for ( i = 0; i < count; i++ ) - { - rels = elf_shdr_by_index(elf, i); - type = elf_uval(elf, rels, sh_type); - if ( (type != SHT_REL) && (type != SHT_RELA) ) - continue; - - sect = elf_shdr_by_index(elf, elf_uval(elf, rels, sh_info)); - syms = elf_shdr_by_index(elf, elf_uval(elf, rels, sh_link)); - if ( NULL == sect || NULL == syms ) - continue; - - if ( !(elf_uval(elf, sect, sh_flags) & SHF_ALLOC) ) - { - elf_msg(elf, "%s: relocations for %s, skipping\n", - __FUNCTION__, elf_section_name(elf, sect)); - continue; - } - - elf_msg(elf, "%s: relocations for %s @ 0x%" PRIx64 "\n", - __FUNCTION__, elf_section_name(elf, sect), - elf_uval(elf, sect, sh_addr)); - if ( elf_reloc_section(elf, rels, sect, syms) != 0 ) - return -1; - } - return 0; -} - -/* - * Local variables: - * mode: C - * c-file-style: "BSD" - * c-basic-offset: 4 - * tab-width: 4 - * indent-tabs-mode: nil - * End: - */ -- 1.7.2.5
Provide a version of xc_dom_seg_to_ptr which returns the number of guest pages it has actually mapped. This is useful for callers who want to do range checking; we will use this later in this series. This is part of the fix to a security issue, XSA-55. Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> Reviewed-by: Chuck Anderson <chuck.anderson@oracle.com> v7: xc_dom_seg_to_ptr_pages now always expects pages_out!=NULL. (It seems silly to have it tolerate NULL when all the real callers pass non-NULL and there''s a version which doesn''t need pages_out anyway. Fix the call in xc_dom_seg_to_ptr to have a dummy pages for pages_out.) v5: xc_dom_seg_to_ptr_pages sets *pages_out=0 if it returns NULL. v4 was: Acked-by: Ian Campbell <ian.campbell@citrix.com> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> --- tools/libxc/xc_dom.h | 19 ++++++++++++++++--- 1 files changed, 16 insertions(+), 3 deletions(-) diff --git a/tools/libxc/xc_dom.h b/tools/libxc/xc_dom.h index ac36600..316c5cb 100644 --- a/tools/libxc/xc_dom.h +++ b/tools/libxc/xc_dom.h @@ -294,14 +294,27 @@ void *xc_dom_pfn_to_ptr(struct xc_dom_image *dom, xen_pfn_t first, void xc_dom_unmap_one(struct xc_dom_image *dom, xen_pfn_t pfn); void xc_dom_unmap_all(struct xc_dom_image *dom); -static inline void *xc_dom_seg_to_ptr(struct xc_dom_image *dom, - struct xc_dom_seg *seg) +static inline void *xc_dom_seg_to_ptr_pages(struct xc_dom_image *dom, + struct xc_dom_seg *seg, + xen_pfn_t *pages_out) { xen_vaddr_t segsize = seg->vend - seg->vstart; unsigned int page_size = XC_DOM_PAGE_SIZE(dom); xen_pfn_t pages = (segsize + page_size - 1) / page_size; + void *retval; + + retval = xc_dom_pfn_to_ptr(dom, seg->pfn, pages); + + *pages_out = retval ? pages : 0; + return retval; +} + +static inline void *xc_dom_seg_to_ptr(struct xc_dom_image *dom, + struct xc_dom_seg *seg) +{ + xen_pfn_t dummy; - return xc_dom_pfn_to_ptr(dom, seg->pfn, pages); + return xc_dom_seg_to_ptr_pages(dom, seg, &dummy); } static inline void *xc_dom_vaddr_to_ptr(struct xc_dom_image *dom, -- 1.7.2.5
Ian Jackson
2013-Jun-13 18:14 UTC
[PATCH 03/23] libxc: Fix range checking in xc_dom_pfn_to_ptr etc.
* Ensure that xc_dom_pfn_to_ptr (when called with count==0) does not return a previously-allocated block which is entirely before the requested pfn (!) * Provide a version of xc_dom_pfn_to_ptr, xc_dom_pfn_to_ptr_retcount, which provides the length of the mapped region via an out parameter. * Change xc_dom_vaddr_to_ptr to always provide the length of the mapped region and change the call site in xc_dom_binloader.c to check it. The call site in xc_dom_load_elf_symtab will be corrected in a forthcoming patch, and for now ignores the returned length. This is part of the fix to a security issue, XSA-55. Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> v5: This patch is new in v5 of the series. --- tools/libxc/xc_dom.h | 16 +++++++++++++--- tools/libxc/xc_dom_binloader.c | 11 ++++++++++- tools/libxc/xc_dom_core.c | 13 +++++++++++++ tools/libxc/xc_dom_elfloader.c | 3 ++- 4 files changed, 38 insertions(+), 5 deletions(-) diff --git a/tools/libxc/xc_dom.h b/tools/libxc/xc_dom.h index 316c5cb..ad6fdd4 100644 --- a/tools/libxc/xc_dom.h +++ b/tools/libxc/xc_dom.h @@ -291,6 +291,8 @@ int xc_dom_alloc_segment(struct xc_dom_image *dom, void *xc_dom_pfn_to_ptr(struct xc_dom_image *dom, xen_pfn_t first, xen_pfn_t count); +void *xc_dom_pfn_to_ptr_retcount(struct xc_dom_image *dom, xen_pfn_t first, + xen_pfn_t count, xen_pfn_t *count_out); void xc_dom_unmap_one(struct xc_dom_image *dom, xen_pfn_t pfn); void xc_dom_unmap_all(struct xc_dom_image *dom); @@ -318,13 +320,21 @@ static inline void *xc_dom_seg_to_ptr(struct xc_dom_image *dom, } static inline void *xc_dom_vaddr_to_ptr(struct xc_dom_image *dom, - xen_vaddr_t vaddr) + xen_vaddr_t vaddr, + size_t *safe_region_out) { unsigned int page_size = XC_DOM_PAGE_SIZE(dom); xen_pfn_t page = (vaddr - dom->parms.virt_base) / page_size; unsigned int offset = (vaddr - dom->parms.virt_base) % page_size; - void *ptr = xc_dom_pfn_to_ptr(dom, page, 0); - return (ptr ? (ptr + offset) : NULL); + xen_pfn_t safe_region_count; + void *ptr; + + *safe_region_out = 0; + ptr = xc_dom_pfn_to_ptr_retcount(dom, page, 0, &safe_region_count); + if ( ptr == NULL ) + return ptr; + *safe_region_out = (safe_region_count << XC_DOM_PAGE_SHIFT(dom)) - offset; + return ptr; } static inline xen_pfn_t xc_dom_p2m_host(struct xc_dom_image *dom, xen_pfn_t pfn) diff --git a/tools/libxc/xc_dom_binloader.c b/tools/libxc/xc_dom_binloader.c index c14727c..d2de04c 100644 --- a/tools/libxc/xc_dom_binloader.c +++ b/tools/libxc/xc_dom_binloader.c @@ -249,6 +249,7 @@ static int xc_dom_load_bin_kernel(struct xc_dom_image *dom) char *image = dom->kernel_blob; char *dest; size_t image_size = dom->kernel_size; + size_t dest_size; uint32_t start_addr; uint32_t load_end_addr; uint32_t bss_end_addr; @@ -272,7 +273,15 @@ static int xc_dom_load_bin_kernel(struct xc_dom_image *dom) DOMPRINTF(" text_size: 0x%" PRIx32 "", text_size); DOMPRINTF(" bss_size: 0x%" PRIx32 "", bss_size); - dest = xc_dom_vaddr_to_ptr(dom, dom->kernel_seg.vstart); + dest = xc_dom_vaddr_to_ptr(dom, dom->kernel_seg.vstart, &dest_size); + + if ( dest_size < text_size || + dest_size - text_size < bss_size ) + { + DOMPRINTF("%s: mapped region is too small for image", __FUNCTION__); + return -EINVAL; + } + memcpy(dest, image + skip, text_size); memset(dest + text_size, 0, bss_size); diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c index b92e4a9..cf96bfa 100644 --- a/tools/libxc/xc_dom_core.c +++ b/tools/libxc/xc_dom_core.c @@ -351,11 +351,20 @@ int xc_dom_try_gunzip(struct xc_dom_image *dom, void **blob, size_t * size) void *xc_dom_pfn_to_ptr(struct xc_dom_image *dom, xen_pfn_t pfn, xen_pfn_t count) { + xen_pfn_t count_out_dummy; + return xc_dom_pfn_to_ptr_retcount(dom, pfn, count, &count_out_dummy); +} + +void *xc_dom_pfn_to_ptr_retcount(struct xc_dom_image *dom, xen_pfn_t pfn, + xen_pfn_t count, xen_pfn_t *count_out) +{ struct xc_dom_phys *phys; xen_pfn_t offset; unsigned int page_shift = XC_DOM_PAGE_SHIFT(dom); char *mode = "unset"; + *count_out = 0; + offset = pfn - dom->rambase_pfn; if ( offset > dom->total_pages || /* multiple checks to avoid overflows */ count > dom->total_pages || @@ -386,6 +395,7 @@ void *xc_dom_pfn_to_ptr(struct xc_dom_image *dom, xen_pfn_t pfn, phys->count); return NULL; } + *count_out = count; } else { @@ -393,6 +403,9 @@ void *xc_dom_pfn_to_ptr(struct xc_dom_image *dom, xen_pfn_t pfn, just hand out a pointer to it */ if ( pfn < phys->first ) continue; + if ( pfn >= phys->first + phys->count ) + continue; + *count_out = phys->count - (pfn - phys->first); } return phys->ptr + ((pfn - phys->first) << page_shift); } diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c index 6583859..bc92302 100644 --- a/tools/libxc/xc_dom_elfloader.c +++ b/tools/libxc/xc_dom_elfloader.c @@ -128,10 +128,11 @@ 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 */ 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); + hdr = xc_dom_vaddr_to_ptr(dom, dom->bsd_symtab_start, &allow_size); *(int *)hdr = size - sizeof(int); } else -- 1.7.2.5
Ian Jackson
2013-Jun-13 18:14 UTC
[PATCH 04/23] libelf: add `struct elf_binary*'' parameter to elf_load_image
The meat of this function is going to need a copy of the elf pointer, in forthcoming patches. No functional change in this patch. This is part of the fix to a security issue, XSA-55. Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> Acked-by: Ian Campbell <ian.campbell@citrix.com> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Chuck Anderson <chuck.anderson@oracle.com> --- xen/common/libelf/libelf-loader.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-) diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c index 3cf9c59..bce667f 100644 --- a/xen/common/libelf/libelf-loader.c +++ b/xen/common/libelf/libelf-loader.c @@ -108,7 +108,8 @@ void elf_set_log(struct elf_binary *elf, elf_log_callback *log_callback, elf->verbose = verbose; } -static int elf_load_image(void *dst, const void *src, uint64_t filesz, uint64_t memsz) +static int elf_load_image(struct elf_binary *elf, + void *dst, const void *src, uint64_t filesz, uint64_t memsz) { memcpy(dst, src, filesz); memset(dst + filesz, 0, memsz - filesz); @@ -122,7 +123,8 @@ void elf_set_verbose(struct elf_binary *elf) elf->verbose = 1; } -static int elf_load_image(void *dst, const void *src, uint64_t filesz, uint64_t memsz) +static int elf_load_image(struct elf_binary *elf, + void *dst, const void *src, uint64_t filesz, uint64_t memsz) { int rc; if ( filesz > ULONG_MAX || memsz > ULONG_MAX ) @@ -279,7 +281,7 @@ int elf_load_binary(struct elf_binary *elf) dest = elf_get_ptr(elf, paddr); elf_msg(elf, "%s: phdr %" PRIu64 " at 0x%p -> 0x%p\n", __func__, i, dest, dest + filesz); - if ( elf_load_image(dest, elf->image + offset, filesz, memsz) != 0 ) + if ( elf_load_image(elf, dest, elf->image + offset, filesz, memsz) != 0 ) return -1; } -- 1.7.2.5
Ian Jackson
2013-Jun-13 18:14 UTC
[PATCH 05/23] libelf: abolish elf_sval and elf_access_signed
These are not used anywhere. This is part of the fix to a security issue, XSA-55. Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> Acked-by: Ian Campbell <ian.campbell@citrix.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Chuck Anderson <chuck.anderson@oracle.com> --- xen/common/libelf/libelf-tools.c | 28 ---------------------------- xen/include/xen/libelf.h | 11 ----------- 2 files changed, 0 insertions(+), 39 deletions(-) diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c index 8312065..1f08407 100644 --- a/xen/common/libelf/libelf-tools.c +++ b/xen/common/libelf/libelf-tools.c @@ -48,34 +48,6 @@ uint64_t elf_access_unsigned(struct elf_binary * elf, const void *ptr, } } -int64_t elf_access_signed(struct elf_binary *elf, const void *ptr, - uint64_t offset, size_t size) -{ - int need_swap = elf_swap(elf); - const int8_t *s8; - const int16_t *s16; - const int32_t *s32; - const int64_t *s64; - - switch ( size ) - { - case 1: - s8 = ptr + offset; - return *s8; - case 2: - s16 = ptr + offset; - return need_swap ? bswap_16(*s16) : *s16; - case 4: - s32 = ptr + offset; - return need_swap ? bswap_32(*s32) : *s32; - case 8: - s64 = ptr + offset; - return need_swap ? bswap_64(*s64) : *s64; - default: - return 0; - } -} - uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr) { int elf_round = (elf_64bit(elf) ? 8 : 4) - 1; diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h index 218bb18..ae03982 100644 --- a/xen/include/xen/libelf.h +++ b/xen/include/xen/libelf.h @@ -136,23 +136,12 @@ struct elf_binary { offsetof(typeof(*(str)),e32.elem), \ sizeof((str)->e32.elem))) -#define elf_sval(elf, str, elem) \ - ((ELFCLASS64 == (elf)->class) \ - ? elf_access_signed((elf), (str), \ - offsetof(typeof(*(str)),e64.elem), \ - sizeof((str)->e64.elem)) \ - : elf_access_signed((elf), (str), \ - offsetof(typeof(*(str)),e32.elem), \ - sizeof((str)->e32.elem))) - #define elf_size(elf, str) \ ((ELFCLASS64 == (elf)->class) \ ? sizeof((str)->e64) : sizeof((str)->e32)) uint64_t elf_access_unsigned(struct elf_binary *elf, const void *ptr, uint64_t offset, size_t size); -int64_t elf_access_signed(struct elf_binary *elf, const void *ptr, - uint64_t offset, size_t size); uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr); -- 1.7.2.5
Ian Jackson
2013-Jun-13 18:14 UTC
[PATCH 06/23] libelf: move include of <asm/guest_access.h> to top of file
libelf-loader.c #includes <asm/guest_access.h>, when being compiled for Xen. Currently it does this in the middle of the file. Move this #include to the top of the file, before libelf-private.h. This is necessary because in forthcoming patches we will introduce private #defines of memcpy etc. which would interfere with definitions in headers #included from guest_access.h. No semantic or functional change in this patch. This is part of the fix to a security issue, XSA-55. Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> Acked-by: Ian Campbell <ian.campbell@citrix.com> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Chuck Anderson <chuck.anderson@oracle.com> --- xen/common/libelf/libelf-loader.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c index bce667f..94257f6 100644 --- a/xen/common/libelf/libelf-loader.c +++ b/xen/common/libelf/libelf-loader.c @@ -16,6 +16,10 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */ +#ifdef __XEN__ +#include <asm/guest_access.h> +#endif + #include "libelf-private.h" /* ------------------------------------------------------------------------ */ @@ -116,7 +120,6 @@ static int elf_load_image(struct elf_binary *elf, return 0; } #else -#include <asm/guest_access.h> void elf_set_verbose(struct elf_binary *elf) { -- 1.7.2.5
Ian Jackson
2013-Jun-13 18:14 UTC
[PATCH 07/23] libelf/xc_dom_load_elf_symtab: Do not use "syms" uninitialised
xc_dom_load_elf_symtab (with load==0) calls elf_round_up, but it mistakenly used the uninitialised variable "syms" when calculating dom->bsd_symtab_start. This should be a reference to "elf". This change might have the effect of rounding the value differently. Previously if the uninitialised value (a single byte on the stack) was ELFCLASS64 (ie, 2), the alignment would be to 8 bytes, otherwise to 4. However, the value is calculated from dom->kernel_seg.vend so this could only make a difference if that value wasn''t already aligned to 8 bytes. This is part of the fix to a security issue, XSA-55. Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> Acked-by: Ian Campbell <ian.campbell@citrix.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Chuck Anderson <chuck.anderson@oracle.com> v2: Split this change into its own patch for proper review. --- tools/libxc/xc_dom_elfloader.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c index bc92302..7ff51d1 100644 --- a/tools/libxc/xc_dom_elfloader.c +++ b/tools/libxc/xc_dom_elfloader.c @@ -142,7 +142,7 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom, hdr = xc_dom_malloc(dom, size); if ( hdr == NULL ) return 0; - dom->bsd_symtab_start = elf_round_up(&syms, dom->kernel_seg.vend); + dom->bsd_symtab_start = elf_round_up(elf, dom->kernel_seg.vend); } memcpy(hdr + sizeof(int), -- 1.7.2.5
Ian Jackson
2013-Jun-13 18:14 UTC
[PATCH 08/23] libelf: introduce macros for memory access and pointer handling
We introduce a collection of macros which abstract away all the pointer arithmetic and dereferences used for accessing the input ELF and the output area(s). We use the new macros everywhere. For now, these macros are semantically identical to the code they replace, so this patch has no functional change. elf_is_elfbinary is an exception: since it doesn''t take an elf*, we need to handle it differently. In a future patch we will change it to take, and check, a length parameter. For now we just mark it with a fixme. That this patch has no functional change can be verified as follows: 0. Copy the scripts "comparison-generate" and "function-filter" out of this commit message. 1. Check out the tree before this patch. 2. Run the script ../comparison-generate .... ../before 3. Check out the tree after this patch. 4. Run the script ../comparison-generate .... ../after 5. diff --exclude=\*.[soi] -ruN before/ after/ |less Expect these differences: * stubdom/zlib-x86_64/ztest*.s2 The filename of this test file apparently contains the pid. * xen/common/version.s2 The xen build timestamp appears in two diff hunks. Verification that this is all that''s needed: In a completely built xen.git, find * -name .*.d -type f | xargs grep -l libelf\.h Expect results in: xen/arch/x86: Checked above. tools/libxc: Checked above. tools/xcutils/readnotes: Checked above. tools/xenstore: Checked above. xen/common/libelf: This is the build for the hypervisor; checked in B above. stubdom: We have one stubdom which reads ELFs using our libelf, pvgrub, which is checked above. I have not done this verification for ARM. This is part of the fix to a security issue, XSA-55. Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> Acked-by: Ian Campbell <ian.campbell@citrix.com> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> v7: Add uintptr_t cast to ELF_UNSAFE_PTR. Still verifies. Use git foo not git-foo in commit message verification script. v4: Fix elf_load_binary''s phdr message to be correct on 32-bit. Fix ELF_OBSOLETE_VOIDP_CAST to work on 32-bit. Indent scripts in commit message. v3.1: Change elf_store_field to verify correctly on 32-bit. comparison-generate copes with Xen 4.1''s lack of ./configure. v2: Use Xen style for multi-line comments. Postpone changes to readnotes.c:print_l1_mfn_valid_note. Much improved verification instructions with new script. Fixed commit message subject. -8<- comparison-generate -8<- #!/bin/bash # usage: # cd xen.git # .../comparison-generate OUR-CONFIG BUILD-RUNE-PREFIX ../before|../after # eg: # .../comparison-generate ~/work/.config ''schroot -pc64 --'' ../before set -ex test $# = 3 || need-exactly-three-arguments our_config=$1 build_rune_prefix=$2 result_dir=$3 git clean -x -d -f cp "$our_config" . cat <<END >>.config debug_symbols=n CFLAGS += -save-temps END perl -i~ -pe ''s/ -g / -g0 / if m/^CFLAGS/'' xen/Rules.mk if [ -f ./configure ]; then $build_rune_prefix ./configure fi $build_rune_prefix make -C xen $build_rune_prefix make -C tools/include $build_rune_prefix make -C stubdom grub $build_rune_prefix make -C tools/libxc $build_rune_prefix make -C tools/xenstore $build_rune_prefix make -C tools/xcutils rm -rf "$result_dir" mkdir "$result_dir" set +x for f in `find xen tools stubdom -name \*.[soi]`; do mkdir -p "$result_dir"/`dirname $f` cp $f "$result_dir"/${f} case $f in *.s) ../function-filter <$f >"$result_dir"/${f}2 ;; esac done echo ok. -8<- -8<- function-filter -8<- #!/usr/bin/perl -w # function-filter # script for massaging gcc-generated labels to be consistent use strict; our @lines; my $sedderybody = "sub seddery () {\n"; while (<>) { push @lines, $_; if (m/^(__FUNCTION__|__func__)\.(\d+)\:/) { $sedderybody .= " s/\\b$1\\.$2\\b/__XSA55MANGLED__$1.$./g;\n"; } } $sedderybody .= "}\n1;\n"; eval $sedderybody or die $@; foreach (@lines) { seddery(); print or die $!; } -8<- --- tools/libxc/xc_dom_elfloader.c | 30 +++--- tools/libxc/xc_hvm_build_x86.c | 2 +- tools/xcutils/readnotes.c | 26 +++--- xen/common/libelf/libelf-dominfo.c | 51 +++++----- xen/common/libelf/libelf-loader.c | 84 +++++++++-------- xen/common/libelf/libelf-tools.c | 94 +++++++++--------- xen/include/xen/libelf.h | 188 +++++++++++++++++++++++++++++++----- 7 files changed, 312 insertions(+), 163 deletions(-) diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c index 7ff51d1..b8089bc 100644 --- a/tools/libxc/xc_dom_elfloader.c +++ b/tools/libxc/xc_dom_elfloader.c @@ -113,9 +113,9 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom, struct elf_binary *elf, int load) { struct elf_binary syms; - const elf_shdr *shdr, *shdr2; + ELF_HANDLE_DECL_NONCONST(elf_shdr) shdr; ELF_HANDLE_DECL(elf_shdr) shdr2; xen_vaddr_t symtab, maxaddr; - char *hdr; + ELF_PTRVAL_CHAR hdr; size_t size; int h, count, type, i, tables = 0; @@ -145,11 +145,11 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom, dom->bsd_symtab_start = elf_round_up(elf, dom->kernel_seg.vend); } - memcpy(hdr + sizeof(int), - elf->image, + elf_memcpy_safe(elf, hdr + sizeof(int), + ELF_IMAGE_BASE(elf), elf_size(elf, elf->ehdr)); - memcpy(hdr + sizeof(int) + elf_size(elf, elf->ehdr), - elf->image + elf_uval(elf, elf->ehdr, e_shoff), + elf_memcpy_safe(elf, hdr + sizeof(int) + 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) ) { @@ -187,7 +187,7 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom, count = elf_shdr_count(&syms); for ( h = 0; h < count; h++ ) { - shdr = elf_shdr_by_index(&syms, h); + shdr = ELF_OBSOLETE_VOIDP_CAST elf_shdr_by_index(&syms, h); type = elf_uval(&syms, shdr, sh_type); if ( type == SHT_STRTAB ) { @@ -203,9 +203,9 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom, if ( i == count ) { if ( elf_64bit(&syms) ) - *(Elf64_Off*)(&shdr->e64.sh_offset) = 0; + elf_store_field(elf, shdr, e64.sh_offset, 0); else - *(Elf32_Off*)(&shdr->e32.sh_offset) = 0; + elf_store_field(elf, shdr, e32.sh_offset, 0); continue; } } @@ -214,9 +214,9 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom, { /* Mangled to be based on ELF header location. */ if ( elf_64bit(&syms) ) - *(Elf64_Off*)(&shdr->e64.sh_offset) = maxaddr - symtab; + elf_store_field(elf, shdr, e64.sh_offset, maxaddr - symtab); else - *(Elf32_Off*)(&shdr->e32.sh_offset) = maxaddr - symtab; + elf_store_field(elf, shdr, e32.sh_offset, maxaddr - symtab); size = elf_uval(&syms, shdr, sh_size); maxaddr = elf_round_up(&syms, maxaddr + size); tables++; @@ -228,7 +228,7 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom, if ( load ) { shdr2 = elf_shdr_by_index(elf, h); - memcpy((void*)elf_section_start(&syms, shdr), + elf_memcpy_safe(elf, ELF_OBSOLETE_VOIDP_CAST elf_section_start(&syms, shdr), elf_section_start(elf, shdr2), size); } @@ -236,9 +236,9 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom, /* Name is NULL. */ if ( elf_64bit(&syms) ) - *(Elf64_Word*)(&shdr->e64.sh_name) = 0; + elf_store_field(elf, shdr, e64.sh_name, 0); else - *(Elf32_Word*)(&shdr->e32.sh_name) = 0; + elf_store_field(elf, shdr, e32.sh_name, 0); } if ( tables == 0 ) @@ -273,7 +273,7 @@ static int xc_dom_parse_elf_kernel(struct xc_dom_image *dom) } /* Find the section-header strings table. */ - if ( elf->sec_strtab == NULL ) + if ( ELF_PTRVAL_INVALID(elf->sec_strtab) ) { xc_dom_panic(dom->xch, XC_INVALID_KERNEL, "%s: ELF image" " has no shstrtab", __FUNCTION__); diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c index ab33a7f..39f93a3 100644 --- a/tools/libxc/xc_hvm_build_x86.c +++ b/tools/libxc/xc_hvm_build_x86.c @@ -143,7 +143,7 @@ static int loadelfimage(xc_interface *xch, struct elf_binary *elf, if ( elf->dest == NULL ) goto err; - elf->dest += elf->pstart & (PAGE_SIZE - 1); + ELF_ADVANCE_DEST(elf, elf->pstart & (PAGE_SIZE - 1)); /* Load the initial elf image. */ rc = elf_load_binary(elf); diff --git a/tools/xcutils/readnotes.c b/tools/xcutils/readnotes.c index c926186..2af047d 100644 --- a/tools/xcutils/readnotes.c +++ b/tools/xcutils/readnotes.c @@ -61,13 +61,13 @@ struct setup_header { } __attribute__((packed)); static void print_string_note(const char *prefix, struct elf_binary *elf, - const elf_note *note) + ELF_HANDLE_DECL(elf_note) note) { printf("%s: %s\n", prefix, (char*)elf_note_desc(elf, note)); } static void print_numeric_note(const char *prefix, struct elf_binary *elf, - const elf_note *note) + ELF_HANDLE_DECL(elf_note) note) { uint64_t value = elf_note_numeric(elf, note); int descsz = elf_uval(elf, note, descsz); @@ -98,12 +98,12 @@ static void print_l1_mfn_valid_note(const char *prefix, struct elf_binary *elf, } -static int print_notes(struct elf_binary *elf, const elf_note *start, const elf_note *end) +static int print_notes(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) start, ELF_HANDLE_DECL(elf_note) end) { - const elf_note *note; + ELF_HANDLE_DECL(elf_note) note; int notes_found = 0; - for ( note = start; note < end; note = elf_note_next(elf, note) ) + for ( note = start; ELF_HANDLE_PTRVAL(note) < ELF_HANDLE_PTRVAL(end); note = elf_note_next(elf, note) ) { if (0 != strcmp(elf_note_name(elf, note), "Xen")) continue; @@ -170,7 +170,7 @@ int main(int argc, char **argv) void *image,*tmp; struct stat st; struct elf_binary elf; - const elf_shdr *shdr; + ELF_HANDLE_DECL(elf_shdr) shdr; int notes_found = 0; struct setup_header *hdr; @@ -257,7 +257,7 @@ int main(int argc, char **argv) count = elf_phdr_count(&elf); for ( h=0; h < count; h++) { - const elf_phdr *phdr; + ELF_HANDLE_DECL(elf_phdr) phdr; phdr = elf_phdr_by_index(&elf, h); if (elf_uval(&elf, phdr, p_type) != PT_NOTE) continue; @@ -269,8 +269,8 @@ int main(int argc, char **argv) continue; notes_found = print_notes(&elf, - elf_segment_start(&elf, phdr), - elf_segment_end(&elf, phdr)); + ELF_MAKE_HANDLE(elf_note, elf_segment_start(&elf, phdr)), + ELF_MAKE_HANDLE(elf_note, elf_segment_end(&elf, phdr))); } if ( notes_found == 0 ) @@ -278,13 +278,13 @@ int main(int argc, char **argv) count = elf_shdr_count(&elf); for ( h=0; h < count; h++) { - const elf_shdr *shdr; + ELF_HANDLE_DECL(elf_shdr) shdr; shdr = elf_shdr_by_index(&elf, h); if (elf_uval(&elf, shdr, sh_type) != SHT_NOTE) continue; notes_found = print_notes(&elf, - elf_section_start(&elf, shdr), - elf_section_end(&elf, shdr)); + ELF_MAKE_HANDLE(elf_note, elf_section_start(&elf, shdr)), + ELF_MAKE_HANDLE(elf_note, elf_section_end(&elf, shdr))); if ( notes_found ) fprintf(stderr, "using notes from SHT_NOTE section\n"); @@ -292,7 +292,7 @@ int main(int argc, char **argv) } shdr = elf_shdr_by_name(&elf, "__xen_guest"); - if (shdr) + if (ELF_HANDLE_VALID(shdr)) printf("__xen_guest: %s\n", (char*)elf_section_start(&elf, shdr)); return 0; diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c index 3242f54..566f6f9 100644 --- a/xen/common/libelf/libelf-dominfo.c +++ b/xen/common/libelf/libelf-dominfo.c @@ -44,7 +44,7 @@ int elf_xen_parse_features(const char *features, for ( pos = 0; features[pos] != ''\0''; pos += len ) { - memset(feature, 0, sizeof(feature)); + elf_memset_unchecked(feature, 0, sizeof(feature)); for ( len = 0;; len++ ) { if ( len >= sizeof(feature)-1 ) @@ -96,7 +96,7 @@ int elf_xen_parse_features(const char *features, int elf_xen_parse_note(struct elf_binary *elf, struct elf_dom_parms *parms, - const elf_note *note) + ELF_HANDLE_DECL(elf_note) note) { /* *INDENT-OFF* */ static const struct { @@ -215,15 +215,16 @@ int elf_xen_parse_note(struct elf_binary *elf, static int elf_xen_parse_notes(struct elf_binary *elf, struct elf_dom_parms *parms, - const void *start, const void *end) + ELF_PTRVAL_CONST_VOID start, + ELF_PTRVAL_CONST_VOID end) { int xen_elfnotes = 0; - const elf_note *note; + ELF_HANDLE_DECL(elf_note) note; parms->elf_note_start = start; parms->elf_note_end = end; - for ( note = parms->elf_note_start; - (void *)note < parms->elf_note_end; + for ( note = ELF_MAKE_HANDLE(elf_note, parms->elf_note_start); + ELF_HANDLE_PTRVAL(note) < parms->elf_note_end; note = elf_note_next(elf, note) ) { if ( strcmp(elf_note_name(elf, note), "Xen") ) @@ -241,45 +242,46 @@ static int elf_xen_parse_notes(struct elf_binary *elf, int elf_xen_parse_guest_info(struct elf_binary *elf, struct elf_dom_parms *parms) { - const char *h; + ELF_PTRVAL_CONST_CHAR h; char name[32], value[128]; int len; h = parms->guest_info; - while ( *h ) +#define STAR(h) (*(h)) + while ( STAR(h) ) { - memset(name, 0, sizeof(name)); - memset(value, 0, sizeof(value)); + elf_memset_unchecked(name, 0, sizeof(name)); + elf_memset_unchecked(value, 0, sizeof(value)); for ( len = 0;; len++, h++ ) { if ( len >= sizeof(name)-1 ) break; - if ( *h == ''\0'' ) + if ( STAR(h) == ''\0'' ) break; - if ( *h == '','' ) + if ( STAR(h) == '','' ) { h++; break; } - if ( *h == ''='' ) + if ( STAR(h) == ''='' ) { h++; for ( len = 0;; len++, h++ ) { if ( len >= sizeof(value)-1 ) break; - if ( *h == ''\0'' ) + if ( STAR(h) == ''\0'' ) break; - if ( *h == '','' ) + if ( STAR(h) == '','' ) { h++; break; } - value[len] = *h; + value[len] = STAR(h); } break; } - name[len] = *h; + name[len] = STAR(h); } elf_msg(elf, "%s: %s=\"%s\"\n", __FUNCTION__, name, value); @@ -328,7 +330,8 @@ int elf_xen_parse_guest_info(struct elf_binary *elf, static int elf_xen_note_check(struct elf_binary *elf, struct elf_dom_parms *parms) { - if ( (parms->elf_note_start == NULL) && (parms->guest_info == NULL) ) + if ( (ELF_PTRVAL_INVALID(parms->elf_note_start)) && + (ELF_PTRVAL_INVALID(parms->guest_info)) ) { int machine = elf_uval(elf, elf->ehdr, e_machine); if ( (machine == EM_386) || (machine == EM_X86_64) ) @@ -457,12 +460,12 @@ static int elf_xen_addr_calc_check(struct elf_binary *elf, int elf_xen_parse(struct elf_binary *elf, struct elf_dom_parms *parms) { - const elf_shdr *shdr; - const elf_phdr *phdr; + ELF_HANDLE_DECL(elf_shdr) shdr; + ELF_HANDLE_DECL(elf_phdr) phdr; int xen_elfnotes = 0; int i, count, rc; - memset(parms, 0, sizeof(*parms)); + elf_memset_unchecked(parms, 0, sizeof(*parms)); parms->virt_base = UNSET_ADDR; parms->virt_entry = UNSET_ADDR; parms->virt_hypercall = UNSET_ADDR; @@ -532,11 +535,11 @@ int elf_xen_parse(struct elf_binary *elf, for ( i = 0; i < count; i++ ) { shdr = elf_shdr_by_name(elf, "__xen_guest"); - if ( shdr ) + if ( ELF_HANDLE_VALID(shdr) ) { parms->guest_info = elf_section_start(elf, shdr); - parms->elf_note_start = NULL; - parms->elf_note_end = NULL; + parms->elf_note_start = ELF_INVALID_PTRVAL; + parms->elf_note_end = ELF_INVALID_PTRVAL; elf_msg(elf, "%s: __xen_guest: \"%s\"\n", __FUNCTION__, parms->guest_info); elf_xen_parse_guest_info(elf, parms); diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c index 94257f6..f7fe283 100644 --- a/xen/common/libelf/libelf-loader.c +++ b/xen/common/libelf/libelf-loader.c @@ -26,7 +26,7 @@ int elf_init(struct elf_binary *elf, const char *image, size_t size) { - const elf_shdr *shdr; + ELF_HANDLE_DECL(elf_shdr) shdr; uint64_t i, count, section, offset; if ( !elf_is_elfbinary(image) ) @@ -35,7 +35,7 @@ int elf_init(struct elf_binary *elf, const char *image, size_t size) return -1; } - memset(elf, 0, sizeof(*elf)); + elf_memset_unchecked(elf, 0, sizeof(*elf)); elf->image = image; elf->size = size; elf->ehdr = (elf_ehdr *)image; @@ -65,7 +65,7 @@ int elf_init(struct elf_binary *elf, const char *image, size_t size) /* Find section string table. */ section = elf_uval(elf, elf->ehdr, e_shstrndx); shdr = elf_shdr_by_index(elf, section); - if ( shdr != NULL ) + if ( ELF_HANDLE_VALID(shdr) ) elf->sec_strtab = elf_section_start(elf, shdr); /* Find symbol table and symbol string table. */ @@ -77,9 +77,9 @@ int elf_init(struct elf_binary *elf, const char *image, size_t size) continue; elf->sym_tab = shdr; shdr = elf_shdr_by_index(elf, elf_uval(elf, shdr, sh_link)); - if ( shdr == NULL ) + if ( !ELF_HANDLE_VALID(shdr) ) { - elf->sym_tab = NULL; + elf->sym_tab = ELF_INVALID_HANDLE(elf_shdr); continue; } elf->sym_strtab = elf_section_start(elf, shdr); @@ -113,10 +113,11 @@ void elf_set_log(struct elf_binary *elf, elf_log_callback *log_callback, } static int elf_load_image(struct elf_binary *elf, - void *dst, const void *src, uint64_t filesz, uint64_t memsz) + ELF_PTRVAL_VOID dst, ELF_PTRVAL_CONST_VOID src, + uint64_t filesz, uint64_t memsz) { - memcpy(dst, src, filesz); - memset(dst + filesz, 0, memsz - filesz); + elf_memcpy_safe(elf, dst, src, filesz); + elf_memset_safe(elf, dst + filesz, 0, memsz - filesz); return 0; } #else @@ -126,16 +127,17 @@ void elf_set_verbose(struct elf_binary *elf) elf->verbose = 1; } -static int elf_load_image(struct elf_binary *elf, - void *dst, const void *src, uint64_t filesz, uint64_t memsz) +static int elf_load_image(struct elf_binary *elf, ELF_PTRVAL_VOID dst, ELF_PTRVAL_CONST_VOID src, uint64_t filesz, uint64_t memsz) { int rc; if ( filesz > ULONG_MAX || memsz > ULONG_MAX ) return -1; - rc = raw_copy_to_guest(dst, src, filesz); + /* We trust the dom0 kernel image completely, so we don''t care + * about overruns etc. here. */ + rc = raw_copy_to_guest(ELF_UNSAFE_PTR(dst), ELF_UNSAFE_PTR(src), filesz); if ( rc != 0 ) return -1; - rc = raw_clear_guest(dst + filesz, memsz - filesz); + rc = raw_clear_guest(ELF_UNSAFE_PTR(dst + filesz), memsz - filesz); if ( rc != 0 ) return -1; return 0; @@ -146,10 +148,10 @@ static int elf_load_image(struct elf_binary *elf, void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart) { uint64_t sz; - const elf_shdr *shdr; + ELF_HANDLE_DECL(elf_shdr) shdr; int i, type; - if ( !elf->sym_tab ) + if ( !ELF_HANDLE_VALID(elf->sym_tab) ) return; pstart = elf_round_up(elf, pstart); @@ -166,7 +168,7 @@ void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart) for ( i = 0; i < elf_shdr_count(elf); i++ ) { shdr = elf_shdr_by_index(elf, i); - type = elf_uval(elf, (elf_shdr *)shdr, sh_type); + type = elf_uval(elf, shdr, sh_type); if ( (type == SHT_STRTAB) || (type == SHT_SYMTAB) ) sz = elf_round_up(elf, sz + elf_uval(elf, shdr, sh_size)); } @@ -177,10 +179,12 @@ void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart) static void elf_load_bsdsyms(struct elf_binary *elf) { - elf_ehdr *sym_ehdr; + ELF_HANDLE_DECL_NONCONST(elf_ehdr) sym_ehdr; unsigned long sz; - char *maxva, *symbase, *symtab_addr; - elf_shdr *shdr; + ELF_PTRVAL_VOID maxva; + ELF_PTRVAL_VOID symbase; + ELF_PTRVAL_VOID symtab_addr; + ELF_HANDLE_DECL_NONCONST(elf_shdr) shdr; int i, type; if ( !elf->bsd_symtab_pstart ) @@ -189,18 +193,18 @@ static void elf_load_bsdsyms(struct elf_binary *elf) #define elf_hdr_elm(_elf, _hdr, _elm, _val) \ do { \ if ( elf_64bit(_elf) ) \ - (_hdr)->e64._elm = _val; \ + elf_store_field(_elf, _hdr, e64._elm, _val); \ else \ - (_hdr)->e32._elm = _val; \ + elf_store_field(_elf, _hdr, e32._elm, _val); \ } while ( 0 ) symbase = elf_get_ptr(elf, elf->bsd_symtab_pstart); symtab_addr = maxva = symbase + sizeof(uint32_t); /* Set up Elf header. */ - sym_ehdr = (elf_ehdr *)symtab_addr; + sym_ehdr = ELF_MAKE_HANDLE(elf_ehdr, symtab_addr); sz = elf_uval(elf, elf->ehdr, e_ehsize); - memcpy(sym_ehdr, elf->ehdr, sz); + elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(sym_ehdr), ELF_HANDLE_PTRVAL(elf->ehdr), sz); maxva += sz; /* no round up */ elf_hdr_elm(elf, sym_ehdr, e_phoff, 0); @@ -209,37 +213,39 @@ do { \ elf_hdr_elm(elf, sym_ehdr, e_phnum, 0); /* Copy Elf section headers. */ - shdr = (elf_shdr *)maxva; + shdr = ELF_MAKE_HANDLE(elf_shdr, maxva); sz = elf_shdr_count(elf) * elf_uval(elf, elf->ehdr, e_shentsize); - memcpy(shdr, elf->image + elf_uval(elf, elf->ehdr, e_shoff), sz); - maxva = (char *)(long)elf_round_up(elf, (long)maxva + sz); + elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(shdr), + ELF_IMAGE_BASE(elf) + elf_uval(elf, elf->ehdr, e_shoff), + sz); + maxva = ELF_OBSOLETE_VOIDP_CAST elf_round_up(elf, (long)maxva + sz); for ( i = 0; i < elf_shdr_count(elf); i++ ) { type = elf_uval(elf, shdr, sh_type); if ( (type == SHT_STRTAB) || (type == SHT_SYMTAB) ) { - elf_msg(elf, "%s: shdr %i at 0x%p -> 0x%p\n", __func__, i, + elf_msg(elf, "%s: shdr %i at 0x%"ELF_PRPTRVAL" -> 0x%"ELF_PRPTRVAL"\n", __func__, i, elf_section_start(elf, shdr), maxva); sz = elf_uval(elf, shdr, sh_size); - memcpy(maxva, elf_section_start(elf, shdr), sz); + elf_memcpy_safe(elf, maxva, elf_section_start(elf, shdr), sz); /* Mangled to be based on ELF header location. */ elf_hdr_elm(elf, shdr, sh_offset, maxva - symtab_addr); - maxva = (char *)(long)elf_round_up(elf, (long)maxva + sz); + maxva = ELF_OBSOLETE_VOIDP_CAST elf_round_up(elf, (long)maxva + sz); } - shdr = (elf_shdr *)((long)shdr + + shdr = ELF_MAKE_HANDLE(elf_shdr, ELF_HANDLE_PTRVAL(shdr) + (long)elf_uval(elf, elf->ehdr, e_shentsize)); } /* Write down the actual sym size. */ - *(uint32_t *)symbase = maxva - symtab_addr; + elf_store_val(elf, uint32_t, symbase, maxva - symtab_addr); #undef elf_ehdr_elm } void elf_parse_binary(struct elf_binary *elf) { - const elf_phdr *phdr; + ELF_HANDLE_DECL(elf_phdr) phdr; uint64_t low = -1; uint64_t high = 0; uint64_t i, count, paddr, memsz; @@ -267,9 +273,9 @@ void elf_parse_binary(struct elf_binary *elf) int elf_load_binary(struct elf_binary *elf) { - const elf_phdr *phdr; + ELF_HANDLE_DECL(elf_phdr) phdr; uint64_t i, count, paddr, offset, filesz, memsz; - char *dest; + ELF_PTRVAL_VOID dest; count = elf_uval(elf, elf->ehdr, e_phnum); for ( i = 0; i < count; i++ ) @@ -282,9 +288,9 @@ int elf_load_binary(struct elf_binary *elf) filesz = elf_uval(elf, phdr, p_filesz); memsz = elf_uval(elf, phdr, p_memsz); dest = elf_get_ptr(elf, paddr); - elf_msg(elf, "%s: phdr %" PRIu64 " at 0x%p -> 0x%p\n", - __func__, i, dest, dest + filesz); - if ( elf_load_image(elf, dest, elf->image + offset, filesz, memsz) != 0 ) + elf_msg(elf, "%s: phdr %" PRIu64 " at 0x%"ELF_PRPTRVAL" -> 0x%"ELF_PRPTRVAL"\n", + __func__, i, dest, (ELF_PTRVAL_VOID)(dest + filesz)); + if ( elf_load_image(elf, dest, ELF_IMAGE_BASE(elf) + offset, filesz, memsz) != 0 ) return -1; } @@ -292,18 +298,18 @@ int elf_load_binary(struct elf_binary *elf) return 0; } -void *elf_get_ptr(struct elf_binary *elf, unsigned long addr) +ELF_PTRVAL_VOID elf_get_ptr(struct elf_binary *elf, unsigned long addr) { return elf->dest + addr - elf->pstart; } uint64_t elf_lookup_addr(struct elf_binary * elf, const char *symbol) { - const elf_sym *sym; + ELF_HANDLE_DECL(elf_sym) sym; uint64_t value; sym = elf_sym_by_name(elf, symbol); - if ( sym == NULL ) + if ( !ELF_HANDLE_VALID(sym) ) { elf_err(elf, "%s: not found: %s\n", __FUNCTION__, symbol); return -1; diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c index 1f08407..bf68bcd 100644 --- a/xen/common/libelf/libelf-tools.c +++ b/xen/common/libelf/libelf-tools.c @@ -67,10 +67,10 @@ int elf_phdr_count(struct elf_binary *elf) return elf_uval(elf, elf->ehdr, e_phnum); } -const elf_shdr *elf_shdr_by_name(struct elf_binary *elf, const char *name) +ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *name) { uint64_t count = elf_shdr_count(elf); - const elf_shdr *shdr; + ELF_HANDLE_DECL(elf_shdr) shdr; const char *sname; int i; @@ -81,76 +81,80 @@ const elf_shdr *elf_shdr_by_name(struct elf_binary *elf, const char *name) if ( sname && !strcmp(sname, name) ) return shdr; } - return NULL; + return ELF_INVALID_HANDLE(elf_shdr); } -const elf_shdr *elf_shdr_by_index(struct elf_binary *elf, int index) +ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_index(struct elf_binary *elf, int index) { uint64_t count = elf_shdr_count(elf); - const void *ptr; + ELF_PTRVAL_CONST_VOID ptr; if ( index >= count ) - return NULL; + return ELF_INVALID_HANDLE(elf_shdr); - ptr = (elf->image + ptr = (ELF_IMAGE_BASE(elf) + elf_uval(elf, elf->ehdr, e_shoff) + elf_uval(elf, elf->ehdr, e_shentsize) * index); - return ptr; + return ELF_MAKE_HANDLE(elf_shdr, ptr); } -const elf_phdr *elf_phdr_by_index(struct elf_binary *elf, int index) +ELF_HANDLE_DECL(elf_phdr) elf_phdr_by_index(struct elf_binary *elf, int index) { uint64_t count = elf_uval(elf, elf->ehdr, e_phnum); - const void *ptr; + ELF_PTRVAL_CONST_VOID ptr; if ( index >= count ) - return NULL; + return ELF_INVALID_HANDLE(elf_phdr); - ptr = (elf->image + ptr = (ELF_IMAGE_BASE(elf) + elf_uval(elf, elf->ehdr, e_phoff) + elf_uval(elf, elf->ehdr, e_phentsize) * index); - return ptr; + return ELF_MAKE_HANDLE(elf_phdr, ptr); } -const char *elf_section_name(struct elf_binary *elf, const elf_shdr * shdr) + +const char *elf_section_name(struct elf_binary *elf, + ELF_HANDLE_DECL(elf_shdr) shdr) { - if ( elf->sec_strtab == NULL ) + if ( ELF_PTRVAL_INVALID(elf->sec_strtab) ) return "unknown"; + return elf->sec_strtab + elf_uval(elf, shdr, sh_name); } -const void *elf_section_start(struct elf_binary *elf, const elf_shdr * shdr) +ELF_PTRVAL_CONST_VOID elf_section_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr) { - return elf->image + elf_uval(elf, shdr, sh_offset); + return ELF_IMAGE_BASE(elf) + elf_uval(elf, shdr, sh_offset); } -const void *elf_section_end(struct elf_binary *elf, const elf_shdr * shdr) +ELF_PTRVAL_CONST_VOID elf_section_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr) { - return elf->image + return ELF_IMAGE_BASE(elf) + elf_uval(elf, shdr, sh_offset) + elf_uval(elf, shdr, sh_size); } -const void *elf_segment_start(struct elf_binary *elf, const elf_phdr * phdr) +ELF_PTRVAL_CONST_VOID elf_segment_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr) { - return elf->image + elf_uval(elf, phdr, p_offset); + return ELF_IMAGE_BASE(elf) + + elf_uval(elf, phdr, p_offset); } -const void *elf_segment_end(struct elf_binary *elf, const elf_phdr * phdr) +ELF_PTRVAL_CONST_VOID elf_segment_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr) { - return elf->image + return ELF_IMAGE_BASE(elf) + elf_uval(elf, phdr, p_offset) + elf_uval(elf, phdr, p_filesz); } -const elf_sym *elf_sym_by_name(struct elf_binary *elf, const char *symbol) +ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *symbol) { - const void *ptr = elf_section_start(elf, elf->sym_tab); - const void *end = elf_section_end(elf, elf->sym_tab); - const elf_sym *sym; + ELF_PTRVAL_CONST_VOID ptr = elf_section_start(elf, elf->sym_tab); + ELF_PTRVAL_CONST_VOID end = elf_section_end(elf, elf->sym_tab); + ELF_HANDLE_DECL(elf_sym) sym; uint64_t info, name; for ( ; ptr < end; ptr += elf_size(elf, sym) ) { - sym = ptr; + sym = ELF_MAKE_HANDLE(elf_sym, ptr); info = elf_uval(elf, sym, st_info); name = elf_uval(elf, sym, st_name); if ( ELF32_ST_BIND(info) != STB_GLOBAL ) @@ -159,33 +163,33 @@ const elf_sym *elf_sym_by_name(struct elf_binary *elf, const char *symbol) continue; return sym; } - return NULL; + return ELF_INVALID_HANDLE(elf_sym); } -const elf_sym *elf_sym_by_index(struct elf_binary *elf, int index) +ELF_HANDLE_DECL(elf_sym) elf_sym_by_index(struct elf_binary *elf, int index) { - const void *ptr = elf_section_start(elf, elf->sym_tab); - const elf_sym *sym; + ELF_PTRVAL_CONST_VOID ptr = elf_section_start(elf, elf->sym_tab); + ELF_HANDLE_DECL(elf_sym) sym; - sym = ptr + index * elf_size(elf, sym); + sym = ELF_MAKE_HANDLE(elf_sym, ptr + index * elf_size(elf, sym)); return sym; } -const char *elf_note_name(struct elf_binary *elf, const elf_note * note) +const char *elf_note_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note) { - return (void *)note + elf_size(elf, note); + return ELF_HANDLE_PTRVAL(note) + elf_size(elf, note); } -const void *elf_note_desc(struct elf_binary *elf, const elf_note * note) +ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note) { int namesz = (elf_uval(elf, note, namesz) + 3) & ~3; - return (void *)note + elf_size(elf, note) + namesz; + return ELF_HANDLE_PTRVAL(note) + elf_size(elf, note) + namesz; } -uint64_t elf_note_numeric(struct elf_binary *elf, const elf_note * note) +uint64_t elf_note_numeric(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note) { - const void *desc = elf_note_desc(elf, note); + ELF_PTRVAL_CONST_VOID desc = elf_note_desc(elf, note); int descsz = elf_uval(elf, note, descsz); switch (descsz) @@ -200,10 +204,10 @@ uint64_t elf_note_numeric(struct elf_binary *elf, const elf_note * note) } } -uint64_t elf_note_numeric_array(struct elf_binary *elf, const elf_note *note, +uint64_t elf_note_numeric_array(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note, unsigned int unitsz, unsigned int idx) { - const void *desc = elf_note_desc(elf, note); + ELF_PTRVAL_CONST_VOID desc = elf_note_desc(elf, note); int descsz = elf_uval(elf, note, descsz); if ( descsz % unitsz || idx >= descsz / unitsz ) @@ -220,12 +224,12 @@ uint64_t elf_note_numeric_array(struct elf_binary *elf, const elf_note *note, } } -const elf_note *elf_note_next(struct elf_binary *elf, const elf_note * note) +ELF_HANDLE_DECL(elf_note) elf_note_next(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note) { int namesz = (elf_uval(elf, note, namesz) + 3) & ~3; int descsz = (elf_uval(elf, note, descsz) + 3) & ~3; - return (void *)note + elf_size(elf, note) + namesz + descsz; + return ELF_MAKE_HANDLE(elf_note, ELF_HANDLE_PTRVAL(note) + elf_size(elf, note) + namesz + descsz); } /* ------------------------------------------------------------------------ */ @@ -234,10 +238,10 @@ int elf_is_elfbinary(const void *image) { const Elf32_Ehdr *ehdr = image; - return IS_ELF(*ehdr); + return IS_ELF(*ehdr); /* fixme unchecked */ } -int elf_phdr_is_loadable(struct elf_binary *elf, const elf_phdr * phdr) +int elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr) { uint64_t p_type = elf_uval(elf, phdr, p_type); uint64_t p_flags = elf_uval(elf, phdr, p_flags); diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h index ae03982..7bd3bdb 100644 --- a/xen/include/xen/libelf.h +++ b/xen/include/xen/libelf.h @@ -48,6 +48,97 @@ typedef void elf_log_callback(struct elf_binary*, void *caller_data, /* ------------------------------------------------------------------------ */ +/* Macros for accessing the input image and output area. */ + +/* + * We abstract away the pointerness of these pointers, replacing + * various void*, char* and struct* with the following: + * PTRVAL A pointer to a byte; one can do pointer arithmetic + * on this. + * This replaces variables which were char*,void* + * and their const versions, so we provide four + * different declaration macros: + * ELF_PTRVAL_{,CONST}{VOID,CHAR} + * HANDLE A pointer to a struct. There is one of these types + * for each pointer type - that is, for each "structname". + * In the arguments to the various HANDLE macros, structname + * must be a single identifier which is a typedef. + * It is not permitted to do arithmetic on these + * pointers. In the current code attempts to do so will + * compile, but in the next patch this will become a + * compile error. + * We provide two declaration macros for const and + * non-const pointers. + */ + +#define ELF_REALPTR2PTRVAL(realpointer) (realpointer) + /* Converts an actual C pointer into a PTRVAL */ + +#define ELF_HANDLE_DECL_NONCONST(structname) structname * +#define ELF_HANDLE_DECL(structname) const structname * + /* Provides a type declaration for a HANDLE. */ + /* May only be used to declare ONE variable at a time */ + +#define ELF_PTRVAL_VOID void * +#define ELF_PTRVAL_CHAR char * +#define ELF_PTRVAL_CONST_VOID const void * +#define ELF_PTRVAL_CONST_CHAR const char * + /* Provides a type declaration for a PTRVAL. */ + /* May only be used to declare ONE variable at a time */ + +#define ELF_DEFINE_HANDLE(structname) /* empty */ + /* + * This must be invoked for each HANDLE type to define + * the actual C type used for that kind of HANDLE. + */ + +#define ELF_PRPTRVAL "p" + /* printf format a la PRId... for a PTRVAL */ + +#define ELF_MAKE_HANDLE(structname, ptrval) (ptrval) + /* Converts a PTRVAL to a HANDLE */ + +#define ELF_IMAGE_BASE(elf) ((elf)->image) + /* Returns the base of the image as a PTRVAL. */ + +#define ELF_HANDLE_PTRVAL(handleval) ((void*)(handleval)) + /* Converts a HANDLE to a PTRVAL. */ + +#define ELF_OBSOLETE_VOIDP_CAST (void*)(uintptr_t) + /* + * In some places the existing code needs to + * - cast away const (the existing code uses const a fair + * bit but actually sometimes wants to write to its input) + * from a PTRVAL. + * - convert an integer representing a pointer to a PTRVAL + * This macro provides a suitable cast. + */ + +#define ELF_UNSAFE_PTR(ptrval) ((void*)(uintptr_t)(ptrval)) + /* + * Turns a PTRVAL into an actual C pointer. Before this is done + * the caller must have ensured that the PTRVAL does in fact point + * to a permissible location. + */ + +/* PTRVALs can be INVALID (ie, NULL). */ +#define ELF_INVALID_PTRVAL (NULL) /* returns NULL PTRVAL */ +#define ELF_INVALID_HANDLE(structname) /* returns NULL handle */ \ + ELF_MAKE_HANDLE(structname, ELF_INVALID_PTRVAL) +#define ELF_PTRVAL_VALID(ptrval) (ptrval) /* } */ +#define ELF_HANDLE_VALID(handleval) (handleval) /* } predicates */ +#define ELF_PTRVAL_INVALID(ptrval) ((ptrval) == NULL) /* } */ + +/* For internal use by other macros here */ +#define ELF__HANDLE_FIELD_TYPE(handleval, elm) \ + typeof((handleval)->elm) +#define ELF__HANDLE_FIELD_OFFSET(handleval, elm) \ + offsetof(typeof(*(handleval)),elm) + + +/* ------------------------------------------------------------------------ */ + + typedef union { Elf32_Ehdr e32; Elf64_Ehdr e64; @@ -83,6 +174,12 @@ typedef union { Elf64_Note e64; } elf_note; +ELF_DEFINE_HANDLE(elf_ehdr) +ELF_DEFINE_HANDLE(elf_shdr) +ELF_DEFINE_HANDLE(elf_phdr) +ELF_DEFINE_HANDLE(elf_sym) +ELF_DEFINE_HANDLE(elf_note) + struct elf_binary { /* elf binary */ const char *image; @@ -90,10 +187,10 @@ struct elf_binary { char class; char data; - const elf_ehdr *ehdr; - const char *sec_strtab; - const elf_shdr *sym_tab; - const char *sym_strtab; + ELF_HANDLE_DECL(elf_ehdr) ehdr; + ELF_PTRVAL_CONST_CHAR sec_strtab; + ELF_HANDLE_DECL(elf_shdr) sym_tab; + ELF_PTRVAL_CONST_CHAR sym_strtab; /* loaded to */ char *dest; @@ -135,45 +232,72 @@ struct elf_binary { : elf_access_unsigned((elf), (str), \ offsetof(typeof(*(str)),e32.elem), \ sizeof((str)->e32.elem))) + /* + * Reads an unsigned field in a header structure in the ELF. + * str is a HANDLE, and elem is the field name in it. + */ #define elf_size(elf, str) \ ((ELFCLASS64 == (elf)->class) \ ? sizeof((str)->e64) : sizeof((str)->e32)) + /* + * Returns the size of the substructure for the appropriate 32/64-bitness. + * str should be a HANDLE. + */ -uint64_t elf_access_unsigned(struct elf_binary *elf, const void *ptr, +uint64_t elf_access_unsigned(struct elf_binary *elf, ELF_PTRVAL_CONST_VOID ptr, uint64_t offset, size_t size); + /* Reads a field at arbitrary offset and alignemnt */ uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr); + +#define elf_memcpy_safe(elf, dst, src, sz) memcpy((dst),(src),(sz)) +#define elf_memset_safe(elf, dst, c, sz) memset((dst),(c),(sz)) + /* + * Versions of memcpy and memset which will (in the next patch) + * arrange never to write outside permitted areas. + */ + +#define elf_store_val(elf, type, ptr, val) (*(type*)(ptr) = (val)) + /* Stores a value at a particular PTRVAL. */ + +#define elf_store_field(elf, hdr, elm, val) \ + (elf_store_val((elf), ELF__HANDLE_FIELD_TYPE(hdr, elm), \ + &((hdr)->elm), \ + (val))) + /* Stores a 32/64-bit field. hdr is a HANDLE and elm is the field name. */ + + /* ------------------------------------------------------------------------ */ /* xc_libelf_tools.c */ int elf_shdr_count(struct elf_binary *elf); int elf_phdr_count(struct elf_binary *elf); -const elf_shdr *elf_shdr_by_name(struct elf_binary *elf, const char *name); -const elf_shdr *elf_shdr_by_index(struct elf_binary *elf, int index); -const elf_phdr *elf_phdr_by_index(struct elf_binary *elf, int index); +ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *name); +ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_index(struct elf_binary *elf, int index); +ELF_HANDLE_DECL(elf_phdr) elf_phdr_by_index(struct elf_binary *elf, int index); -const char *elf_section_name(struct elf_binary *elf, const elf_shdr * shdr); -const void *elf_section_start(struct elf_binary *elf, const elf_shdr * shdr); -const void *elf_section_end(struct elf_binary *elf, const elf_shdr * shdr); +const char *elf_section_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr); +ELF_PTRVAL_CONST_VOID elf_section_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr); +ELF_PTRVAL_CONST_VOID elf_section_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr); -const void *elf_segment_start(struct elf_binary *elf, const elf_phdr * phdr); -const void *elf_segment_end(struct elf_binary *elf, const elf_phdr * phdr); +ELF_PTRVAL_CONST_VOID elf_segment_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr); +ELF_PTRVAL_CONST_VOID elf_segment_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr); -const elf_sym *elf_sym_by_name(struct elf_binary *elf, const char *symbol); -const elf_sym *elf_sym_by_index(struct elf_binary *elf, int index); +ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *symbol); +ELF_HANDLE_DECL(elf_sym) elf_sym_by_index(struct elf_binary *elf, int index); -const char *elf_note_name(struct elf_binary *elf, const elf_note * note); -const void *elf_note_desc(struct elf_binary *elf, const elf_note * note); -uint64_t elf_note_numeric(struct elf_binary *elf, const elf_note * note); -uint64_t elf_note_numeric_array(struct elf_binary *, const elf_note *, +const char *elf_note_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note); +ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note); +uint64_t elf_note_numeric(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note); +uint64_t elf_note_numeric_array(struct elf_binary *, ELF_HANDLE_DECL(elf_note), unsigned int unitsz, unsigned int idx); -const elf_note *elf_note_next(struct elf_binary *elf, const elf_note * note); +ELF_HANDLE_DECL(elf_note) elf_note_next(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note); int elf_is_elfbinary(const void *image); -int elf_phdr_is_loadable(struct elf_binary *elf, const elf_phdr * phdr); +int elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr); /* ------------------------------------------------------------------------ */ /* xc_libelf_loader.c */ @@ -189,7 +313,7 @@ void elf_set_log(struct elf_binary *elf, elf_log_callback*, void elf_parse_binary(struct elf_binary *elf); int elf_load_binary(struct elf_binary *elf); -void *elf_get_ptr(struct elf_binary *elf, unsigned long addr); +ELF_PTRVAL_VOID elf_get_ptr(struct elf_binary *elf, unsigned long addr); uint64_t elf_lookup_addr(struct elf_binary *elf, const char *symbol); void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart); /* private */ @@ -221,9 +345,9 @@ struct xen_elfnote { struct elf_dom_parms { /* raw */ - const char *guest_info; - const void *elf_note_start; - const void *elf_note_end; + ELF_PTRVAL_CONST_CHAR guest_info; + ELF_PTRVAL_CONST_VOID elf_note_start; + ELF_PTRVAL_CONST_VOID elf_note_end; struct xen_elfnote elf_notes[XEN_ELFNOTE_MAX + 1]; /* parsed */ @@ -262,10 +386,22 @@ int elf_xen_parse_features(const char *features, uint32_t *required); int elf_xen_parse_note(struct elf_binary *elf, struct elf_dom_parms *parms, - const elf_note *note); + ELF_HANDLE_DECL(elf_note) note); int elf_xen_parse_guest_info(struct elf_binary *elf, struct elf_dom_parms *parms); int elf_xen_parse(struct elf_binary *elf, struct elf_dom_parms *parms); +#define elf_memcpy_unchecked memcpy +#define elf_memset_unchecked memset + /* + * Unsafe versions of memcpy and memset which take actual C + * pointers. These are just like real memcpy and memset. + */ + + +#define ELF_ADVANCE_DEST(elf, amount) elf->dest += (amount) + /* Advances past amount bytes of the current destination area. */ + + #endif /* __XEN_LIBELF_H__ */ -- 1.7.2.5
Ian Jackson
2013-Jun-13 18:14 UTC
[PATCH 09/23] tools/xcutils/readnotes: adjust print_l1_mfn_valid_note
Use the new PTRVAL macros and elf_access_unsigned in print_l1_mfn_valid_note. No functional change unless the input is wrong, or we are reading a file for a different endianness. Separated out from the previous patch because this change does produce a difference in the generated code. This is part of the fix to a security issue, XSA-55. Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> Acked-by: Ian Campbell <ian.campbell@citrix.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Chuck Anderson <chuck.anderson@oracle.com> v2: Split out into its own patch. --- tools/xcutils/readnotes.c | 11 ++++++----- 1 files changed, 6 insertions(+), 5 deletions(-) diff --git a/tools/xcutils/readnotes.c b/tools/xcutils/readnotes.c index 2af047d..7ff2530 100644 --- a/tools/xcutils/readnotes.c +++ b/tools/xcutils/readnotes.c @@ -77,22 +77,23 @@ 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, - const elf_note *note) + ELF_HANDLE_DECL(elf_note) note) { int descsz = elf_uval(elf, note, descsz); - const uint32_t *desc32 = elf_note_desc(elf, note); - const uint64_t *desc64 = elf_note_desc(elf, note); + ELF_PTRVAL_CONST_VOID desc = elf_note_desc(elf, note); /* XXX should be able to cope with a list of values. */ switch ( descsz / 2 ) { case 8: printf("%s: mask=%#"PRIx64" value=%#"PRIx64"\n", prefix, - desc64[0], desc64[1]); + elf_access_unsigned(elf, desc, 0, 8), + elf_access_unsigned(elf, desc, 8, 8)); break; case 4: printf("%s: mask=%#"PRIx32" value=%#"PRIx32"\n", prefix, - desc32[0],desc32[1]); + (uint32_t)elf_access_unsigned(elf, desc, 0, 4), + (uint32_t)elf_access_unsigned(elf, desc, 4, 4)); break; } -- 1.7.2.5
Ian Jackson
2013-Jun-13 18:14 UTC
[PATCH 10/23] libelf: check nul-terminated strings properly
It is not safe to simply take pointers into the ELF and use them as C pointers. They might not be properly nul-terminated (and the pointers might be wild). So we are going to introduce a new function elf_strval for safely getting strings. This will check that the addresses are in range and that there is a proper nul-terminated string. Of course it might discover that there isn''t. In that case, it will be made to fail. This means that elf_note_name might fail, too. For the benefit of call sites which are just going to pass the value to a printf-like function, we provide elf_strfmt which returns "(invalid)" on failure rather than NULL. In this patch we introduce dummy definitions of these functions. We introduce calls to elf_strval and elf_strfmt everywhere, and update all the call sites with appropriate error checking. There is not yet any semantic change, since before this patch all the places where we introduce elf_strval dereferenced the value anyway, so it mustn''t have been NULL. In future patches, when elf_strval is made able return NULL, when it does so it will mark the elf "broken" so that an appropriate diagnostic can be printed. This is part of the fix to a security issue, XSA-55. Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> Acked-by: Ian Campbell <ian.campbell@citrix.com> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Reviewed-by: Chuck Anderson <chuck.anderson@oracle.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> v7: Change readnotes.c check to use two if statements rather than ||. v2: Fix coding style, in one "if" statement. --- tools/xcutils/readnotes.c | 11 ++++++++--- xen/common/libelf/libelf-dominfo.c | 13 ++++++++++--- xen/common/libelf/libelf-tools.c | 10 +++++++--- xen/include/xen/libelf.h | 7 +++++-- 4 files changed, 30 insertions(+), 11 deletions(-) diff --git a/tools/xcutils/readnotes.c b/tools/xcutils/readnotes.c index 7ff2530..cfae994 100644 --- a/tools/xcutils/readnotes.c +++ b/tools/xcutils/readnotes.c @@ -63,7 +63,7 @@ struct setup_header { static void print_string_note(const char *prefix, struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note) { - printf("%s: %s\n", prefix, (char*)elf_note_desc(elf, note)); + printf("%s: %s\n", prefix, elf_strfmt(elf, elf_note_desc(elf, note))); } static void print_numeric_note(const char *prefix, struct elf_binary *elf, @@ -103,10 +103,14 @@ static int print_notes(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) start, { ELF_HANDLE_DECL(elf_note) note; int 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) ) { - if (0 != strcmp(elf_note_name(elf, note), "Xen")) + this_note_name = elf_note_name(elf, note); + if (NULL == this_note_name) + continue; + if (0 != strcmp(this_note_name, "Xen")) continue; notes_found++; @@ -294,7 +298,8 @@ int main(int argc, char **argv) shdr = elf_shdr_by_name(&elf, "__xen_guest"); if (ELF_HANDLE_VALID(shdr)) - printf("__xen_guest: %s\n", (char*)elf_section_start(&elf, shdr)); + printf("__xen_guest: %s\n", + elf_strfmt(&elf, elf_section_start(&elf, shdr))); return 0; } diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c index 566f6f9..ba0dc83 100644 --- a/xen/common/libelf/libelf-dominfo.c +++ b/xen/common/libelf/libelf-dominfo.c @@ -137,7 +137,10 @@ int elf_xen_parse_note(struct elf_binary *elf, if ( note_desc[type].str ) { - str = elf_note_desc(elf, note); + str = elf_strval(elf, elf_note_desc(elf, note)); + if (str == NULL) + /* elf_strval will mark elf broken if it fails so no need to log */ + return 0; elf_msg(elf, "%s: %s = \"%s\"\n", __FUNCTION__, note_desc[type].name, str); parms->elf_notes[type].type = XEN_ENT_STR; @@ -220,6 +223,7 @@ static int elf_xen_parse_notes(struct elf_binary *elf, { int xen_elfnotes = 0; ELF_HANDLE_DECL(elf_note) note; + const char *note_name; parms->elf_note_start = start; parms->elf_note_end = end; @@ -227,7 +231,10 @@ static int elf_xen_parse_notes(struct elf_binary *elf, ELF_HANDLE_PTRVAL(note) < parms->elf_note_end; note = elf_note_next(elf, note) ) { - if ( strcmp(elf_note_name(elf, note), "Xen") ) + note_name = elf_note_name(elf, note); + if ( note_name == NULL ) + continue; + if ( strcmp(note_name, "Xen") ) continue; if ( elf_xen_parse_note(elf, parms, note) ) return -1; @@ -541,7 +548,7 @@ int elf_xen_parse(struct elf_binary *elf, parms->elf_note_start = ELF_INVALID_PTRVAL; parms->elf_note_end = ELF_INVALID_PTRVAL; elf_msg(elf, "%s: __xen_guest: \"%s\"\n", __FUNCTION__, - parms->guest_info); + elf_strfmt(elf, parms->guest_info)); elf_xen_parse_guest_info(elf, parms); break; } diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c index bf68bcd..fa7dedd 100644 --- a/xen/common/libelf/libelf-tools.c +++ b/xen/common/libelf/libelf-tools.c @@ -119,7 +119,7 @@ const char *elf_section_name(struct elf_binary *elf, if ( ELF_PTRVAL_INVALID(elf->sec_strtab) ) return "unknown"; - return elf->sec_strtab + elf_uval(elf, shdr, sh_name); + return elf_strval(elf, elf->sec_strtab + elf_uval(elf, shdr, sh_name)); } ELF_PTRVAL_CONST_VOID elf_section_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr) @@ -151,6 +151,7 @@ ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *sym ELF_PTRVAL_CONST_VOID end = elf_section_end(elf, elf->sym_tab); ELF_HANDLE_DECL(elf_sym) sym; uint64_t info, name; + const char *sym_name; for ( ; ptr < end; ptr += elf_size(elf, sym) ) { @@ -159,7 +160,10 @@ ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *sym name = elf_uval(elf, sym, st_name); if ( ELF32_ST_BIND(info) != STB_GLOBAL ) continue; - if ( strcmp(elf->sym_strtab + name, symbol) ) + sym_name = elf_strval(elf, elf->sym_strtab + name); + if ( sym_name == NULL ) /* out of range, oops */ + return ELF_INVALID_HANDLE(elf_sym); + if ( strcmp(sym_name, symbol) ) continue; return sym; } @@ -177,7 +181,7 @@ ELF_HANDLE_DECL(elf_sym) elf_sym_by_index(struct elf_binary *elf, int index) const char *elf_note_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note) { - return ELF_HANDLE_PTRVAL(note) + elf_size(elf, note); + return elf_strval(elf, ELF_HANDLE_PTRVAL(note) + elf_size(elf, note)); } ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note) diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h index 7bd3bdb..28c7b11 100644 --- a/xen/include/xen/libelf.h +++ b/xen/include/xen/libelf.h @@ -252,6 +252,9 @@ uint64_t elf_access_unsigned(struct elf_binary *elf, ELF_PTRVAL_CONST_VOID ptr, uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr); +#define elf_strval(elf,x) ((const char*)(x)) /* may return NULL in the future */ +#define elf_strfmt(elf,x) ((const char*)(x)) /* will return (invalid) instead */ + #define elf_memcpy_safe(elf, dst, src, sz) memcpy((dst),(src),(sz)) #define elf_memset_safe(elf, dst, c, sz) memset((dst),(c),(sz)) /* @@ -279,7 +282,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *n ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_index(struct elf_binary *elf, int index); ELF_HANDLE_DECL(elf_phdr) elf_phdr_by_index(struct elf_binary *elf, int index); -const char *elf_section_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr); +const char *elf_section_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr); /* might return NULL if inputs are invalid */ ELF_PTRVAL_CONST_VOID elf_section_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr); ELF_PTRVAL_CONST_VOID elf_section_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr); @@ -289,7 +292,7 @@ ELF_PTRVAL_CONST_VOID elf_segment_end(struct elf_binary *elf, ELF_HANDLE_DECL(el ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *symbol); ELF_HANDLE_DECL(elf_sym) elf_sym_by_index(struct elf_binary *elf, int index); -const char *elf_note_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note); +const char *elf_note_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note); /* may return NULL */ ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note); uint64_t elf_note_numeric(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note); uint64_t elf_note_numeric_array(struct elf_binary *, ELF_HANDLE_DECL(elf_note), -- 1.7.2.5
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. Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> v7: Remove a spurious whitespace change. v5: Use allow_size value from xc_dom_vaddr_to_ptr to set xdest_size correctly. If ELF_ADVANCE_DEST advances past the end, mark the elf broken. Always regard NULL allowable region pointers (e.g. dest_base) as invalid (since NULL pointers don''t point anywhere). v4: Fix ELF_UNSAFE_PTR to work on 32-bit even when provided 64-bit values. Fix xc_dom_load_elf_symtab not to call XC_DOM_PAGE_SIZE unnecessarily if load is false. This was a regression. v3.1: Introduce a change to elf_store_field to undo the effects of the v3.1 change to the previous patch (the definition there is not compatible with the new types). v3: Fix a whitespace error. v2 was Acked-by: Ian Campbell <ian.campbell@citrix.com> v2: BUGFIX: elf_strval: Fix loop termination condition to actually work. BUGFIX: elf_strval: Fix return value to not always be totally wild. BUGFIX: xc_dom_load_elf_symtab: do proper check for small header size. xc_dom_load_elf_symtab: narrow scope of `hdr_ptr''. xc_dom_load_elf_symtab: split out uninit''d symtab.class ref fix. More comments on the lifetime/validity of elf-> dest ptrs etc. libelf.h: write "obsolete" out in full libelf.h: rename "dontuse" to "typeonly" and add doc comment elf_ptrval_in_range: Document trustedness of arguments. Style and commit message fixes. --- tools/libxc/xc_dom_elfloader.c | 49 ++++++++-- tools/libxc/xc_hvm_build_x86.c | 10 +- xen/arch/x86/domain_build.c | 3 +- xen/common/libelf/libelf-dominfo.c | 2 +- xen/common/libelf/libelf-loader.c | 16 ++-- xen/common/libelf/libelf-private.h | 13 +++ xen/common/libelf/libelf-tools.c | 106 ++++++++++++++++++- xen/include/xen/libelf.h | 198 +++++++++++++++++++++++++----------- 8 files changed, 312 insertions(+), 85 deletions(-) diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c index b8089bc..c038d1c 100644 --- a/tools/libxc/xc_dom_elfloader.c +++ b/tools/libxc/xc_dom_elfloader.c @@ -128,20 +128,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); } @@ -169,9 +179,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); @@ -310,8 +343,10 @@ static int xc_dom_load_elf_kernel(struct xc_dom_image *dom) { struct elf_binary *elf = dom->private_loader; int rc; + 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); rc = elf_load_binary(elf); if ( rc < 0 ) { diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c index 39f93a3..eff55a4 100644 --- a/tools/libxc/xc_hvm_build_x86.c +++ b/tools/libxc/xc_hvm_build_x86.c @@ -137,11 +137,12 @@ static int loadelfimage(xc_interface *xch, struct elf_binary *elf, 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; ELF_ADVANCE_DEST(elf, elf->pstart & (PAGE_SIZE - 1)); @@ -150,8 +151,9 @@ static int loadelfimage(xc_interface *xch, struct elf_binary *elf, if ( rc < 0 ) PERROR("Failed to load elf binary\n"); - 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); diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c index 9980ea2..db31a91 100644 --- a/xen/arch/x86/domain_build.c +++ b/xen/arch/x86/domain_build.c @@ -765,7 +765,8 @@ int __init construct_dom0( mapcache_override_current(v); /* Copy the OS image and free temporary buffer. */ - elf.dest = (void*)vkern_start; + elf.dest_base = (void*)vkern_start; + elf.dest_size = vkern_end - vkern_start; rc = elf_load_binary(&elf); if ( rc < 0 ) { diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c index ba0dc83..b9a4e25 100644 --- a/xen/common/libelf/libelf-dominfo.c +++ b/xen/common/libelf/libelf-dominfo.c @@ -254,7 +254,7 @@ int elf_xen_parse_guest_info(struct elf_binary *elf, int len; h = parms->guest_info; -#define STAR(h) (*(h)) +#define STAR(h) (elf_access_unsigned(elf, (h), 0, 1)) while ( STAR(h) ) { elf_memset_unchecked(name, 0, sizeof(name)); diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c index f7fe283..878552e 100644 --- a/xen/common/libelf/libelf-loader.c +++ b/xen/common/libelf/libelf-loader.c @@ -24,23 +24,25 @@ /* ------------------------------------------------------------------------ */ -int elf_init(struct elf_binary *elf, const char *image, size_t size) +int elf_init(struct elf_binary *elf, const char *image_input, size_t size) { ELF_HANDLE_DECL(elf_shdr) shdr; uint64_t i, count, section, offset; - if ( !elf_is_elfbinary(image) ) + if ( !elf_is_elfbinary(image_input) ) { elf_err(elf, "%s: not an ELF binary\n", __FUNCTION__); return -1; } elf_memset_unchecked(elf, 0, sizeof(*elf)); - elf->image = image; + elf->image_base = image_input; elf->size = size; - elf->ehdr = (elf_ehdr *)image; - elf->class = elf->ehdr->e32.e_ident[EI_CLASS]; - elf->data = elf->ehdr->e32.e_ident[EI_DATA]; + elf->ehdr = ELF_MAKE_HANDLE(elf_ehdr, (elf_ptrval)image_input); + elf->class = elf_uval_3264(elf, elf->ehdr, e32.e_ident[EI_CLASS]); + elf->data = elf_uval_3264(elf, elf->ehdr, e32.e_ident[EI_DATA]); + elf->caller_xdest_base = NULL; + elf->caller_xdest_size = 0; /* Sanity check phdr. */ offset = elf_uval(elf, elf->ehdr, e_phoff) + @@ -300,7 +302,7 @@ int elf_load_binary(struct elf_binary *elf) ELF_PTRVAL_VOID elf_get_ptr(struct elf_binary *elf, unsigned long addr) { - return elf->dest + addr - elf->pstart; + return ELF_REALPTR2PTRVAL(elf->dest_base) + addr - elf->pstart; } uint64_t elf_lookup_addr(struct elf_binary * elf, const char *symbol) diff --git a/xen/common/libelf/libelf-private.h b/xen/common/libelf/libelf-private.h index 0d4dcf6..0bd9e66 100644 --- a/xen/common/libelf/libelf-private.h +++ b/xen/common/libelf/libelf-private.h @@ -86,6 +86,19 @@ do { strncpy((d),(s),sizeof((d))-1); \ #endif +#undef memcpy +#undef memset +#undef memmove +#undef strcpy + +#define memcpy MISTAKE_unspecified_memcpy +#define memset MISTAKE_unspecified_memset +#define memmove MISTAKE_unspecified_memmove +#define strcpy MISTAKE_unspecified_strcpy + /* This prevents libelf from using these undecorated versions + * of memcpy, memset, memmove and strcpy. Every call site + * must either use elf_mem*_unchecked, or elf_mem*_safe. */ + #endif /* __LIBELF_PRIVATE_H_ */ /* diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c index fa7dedd..08ab027 100644 --- a/xen/common/libelf/libelf-tools.c +++ b/xen/common/libelf/libelf-tools.c @@ -20,28 +20,100 @@ /* ------------------------------------------------------------------------ */ -uint64_t elf_access_unsigned(struct elf_binary * elf, const void *ptr, - uint64_t offset, size_t size) +void elf_mark_broken(struct elf_binary *elf, const char *msg) { + if ( elf->broken == NULL ) + elf->broken = msg; +} + +const char *elf_check_broken(const struct elf_binary *elf) +{ + return elf->broken; +} + +static int elf_ptrval_in_range(elf_ptrval ptrval, uint64_t size, + const void *region, uint64_t regionsize) + /* + * Returns true if the putative memory area [ptrval,ptrval+size> + * is completely inside the region [region,region+regionsize>. + * + * ptrval and size are the untrusted inputs to be checked. + * region and regionsize are trusted and must be correct and valid, + * although it is OK for region to perhaps be maliciously NULL + * (but not some other malicious value). + */ +{ + elf_ptrval regionp = (elf_ptrval)region; + + if ( (region == NULL) || + (ptrval < regionp) || /* start is before region */ + (ptrval > regionp + regionsize) || /* start is after region */ + (size > regionsize - (ptrval - regionp)) ) /* too big */ + return 0; + return 1; +} + +int elf_access_ok(struct elf_binary * elf, + uint64_t ptrval, size_t size) +{ + if ( elf_ptrval_in_range(ptrval, size, elf->image_base, elf->size) ) + return 1; + if ( elf_ptrval_in_range(ptrval, size, elf->dest_base, elf->dest_size) ) + return 1; + if ( elf_ptrval_in_range(ptrval, size, + elf->caller_xdest_base, elf->caller_xdest_size) ) + return 1; + elf_mark_broken(elf, "out of range access"); + return 0; +} + +void elf_memcpy_safe(struct elf_binary *elf, elf_ptrval dst, + elf_ptrval src, size_t size) +{ + if ( elf_access_ok(elf, dst, size) && + elf_access_ok(elf, src, size) ) + { + /* use memmove because these checks do not prove that the + * regions don''t overlap and overlapping regions grant + * permission for compiler malice */ + elf_memmove_unchecked(ELF_UNSAFE_PTR(dst), ELF_UNSAFE_PTR(src), size); + } +} + +void elf_memset_safe(struct elf_binary *elf, elf_ptrval dst, int c, size_t size) +{ + if ( elf_access_ok(elf, dst, size) ) + { + elf_memset_unchecked(ELF_UNSAFE_PTR(dst), c, size); + } +} + +uint64_t elf_access_unsigned(struct elf_binary * elf, elf_ptrval base, + uint64_t moreoffset, size_t size) +{ + elf_ptrval ptrval = base + moreoffset; int need_swap = elf_swap(elf); const uint8_t *u8; const uint16_t *u16; const uint32_t *u32; const uint64_t *u64; + if ( !elf_access_ok(elf, ptrval, size) ) + return 0; + switch ( size ) { case 1: - u8 = ptr + offset; + u8 = (const void*)ptrval; return *u8; case 2: - u16 = ptr + offset; + u16 = (const void*)ptrval; return need_swap ? bswap_16(*u16) : *u16; case 4: - u32 = ptr + offset; + u32 = (const void*)ptrval; return need_swap ? bswap_32(*u32) : *u32; case 8: - u64 = ptr + offset; + u64 = (const void*)ptrval; return need_swap ? bswap_64(*u64) : *u64; default: return 0; @@ -122,6 +194,28 @@ const char *elf_section_name(struct elf_binary *elf, return elf_strval(elf, elf->sec_strtab + elf_uval(elf, shdr, sh_name)); } +const char *elf_strval(struct elf_binary *elf, elf_ptrval start) +{ + uint64_t length; + + for ( length = 0; ; length++ ) { + if ( !elf_access_ok(elf, start + length, 1) ) + return NULL; + if ( !elf_access_unsigned(elf, start, length, 1) ) + /* ok */ + return ELF_UNSAFE_PTR(start); + } +} + +const char *elf_strfmt(struct elf_binary *elf, elf_ptrval start) +{ + const char *str = elf_strval(elf, start); + + if ( str == NULL ) + return "(invalid)"; + return str; +} + ELF_PTRVAL_CONST_VOID elf_section_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr) { return ELF_IMAGE_BASE(elf) + elf_uval(elf, shdr, sh_offset); diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h index 28c7b11..f3f18da 100644 --- a/xen/include/xen/libelf.h +++ b/xen/include/xen/libelf.h @@ -57,8 +57,9 @@ typedef void elf_log_callback(struct elf_binary*, void *caller_data, * on this. * This replaces variables which were char*,void* * and their const versions, so we provide four - * different declaration macros: + * different obsolete declaration macros: * ELF_PTRVAL_{,CONST}{VOID,CHAR} + * New code can simply use the elf_ptrval typedef. * HANDLE A pointer to a struct. There is one of these types * for each pointer type - that is, for each "structname". * In the arguments to the various HANDLE macros, structname @@ -67,54 +68,66 @@ typedef void elf_log_callback(struct elf_binary*, void *caller_data, * pointers. In the current code attempts to do so will * compile, but in the next patch this will become a * compile error. - * We provide two declaration macros for const and - * non-const pointers. + * We also provide a second declaration macro for + * pointers which were to const; this is obsolete. */ -#define ELF_REALPTR2PTRVAL(realpointer) (realpointer) +typedef uintptr_t elf_ptrval; + +#define ELF_REALPTR2PTRVAL(realpointer) ((elf_ptrval)(realpointer)) /* Converts an actual C pointer into a PTRVAL */ -#define ELF_HANDLE_DECL_NONCONST(structname) structname * -#define ELF_HANDLE_DECL(structname) const structname * +#define ELF_HANDLE_DECL_NONCONST(structname) structname##_handle /*obsolete*/ +#define ELF_HANDLE_DECL(structname) structname##_handle /* Provides a type declaration for a HANDLE. */ - /* May only be used to declare ONE variable at a time */ -#define ELF_PTRVAL_VOID void * -#define ELF_PTRVAL_CHAR char * -#define ELF_PTRVAL_CONST_VOID const void * -#define ELF_PTRVAL_CONST_CHAR const char * - /* Provides a type declaration for a PTRVAL. */ - /* May only be used to declare ONE variable at a time */ +#define ELF_PTRVAL_VOID elf_ptrval /*obsolete*/ +#define ELF_PTRVAL_CHAR elf_ptrval /*obsolete*/ +#define ELF_PTRVAL_CONST_VOID elf_ptrval /*obsolete*/ +#define ELF_PTRVAL_CONST_CHAR elf_ptrval /*obsolete*/ + +#ifdef __XEN__ +# define ELF_PRPTRVAL "lu" + /* + * PRIuPTR is misdefined in xen/include/xen/inttypes.h, on 32-bit, + * to "u", when in fact uintptr_t is an unsigned long. + */ +#else +# define ELF_PRPTRVAL PRIuPTR +#endif + /* printf format a la PRId... for a PTRVAL */ -#define ELF_DEFINE_HANDLE(structname) /* empty */ +#define ELF_DEFINE_HANDLE(structname) \ + typedef union { \ + elf_ptrval ptrval; \ + const structname *typeonly; /* for sizeof, offsetof, &c only */ \ + } structname##_handle; /* * This must be invoked for each HANDLE type to define * the actual C type used for that kind of HANDLE. */ -#define ELF_PRPTRVAL "p" - /* printf format a la PRId... for a PTRVAL */ - -#define ELF_MAKE_HANDLE(structname, ptrval) (ptrval) +#define ELF_MAKE_HANDLE(structname, ptrval) ((structname##_handle){ ptrval }) /* Converts a PTRVAL to a HANDLE */ -#define ELF_IMAGE_BASE(elf) ((elf)->image) +#define ELF_IMAGE_BASE(elf) ((elf_ptrval)(elf)->image_base) /* Returns the base of the image as a PTRVAL. */ -#define ELF_HANDLE_PTRVAL(handleval) ((void*)(handleval)) +#define ELF_HANDLE_PTRVAL(handleval) ((handleval).ptrval) /* Converts a HANDLE to a PTRVAL. */ -#define ELF_OBSOLETE_VOIDP_CAST (void*)(uintptr_t) +#define ELF_OBSOLETE_VOIDP_CAST /*empty*/ /* - * In some places the existing code needs to + * In some places the old code used to need to * - cast away const (the existing code uses const a fair * bit but actually sometimes wants to write to its input) * from a PTRVAL. * - convert an integer representing a pointer to a PTRVAL - * This macro provides a suitable cast. + * Nowadays all of these re uintptr_ts so there is no const problem + * and no need for any casting. */ -#define ELF_UNSAFE_PTR(ptrval) ((void*)(uintptr_t)(ptrval)) +#define ELF_UNSAFE_PTR(ptrval) ((void*)(elf_ptrval)(ptrval)) /* * Turns a PTRVAL into an actual C pointer. Before this is done * the caller must have ensured that the PTRVAL does in fact point @@ -122,18 +135,21 @@ typedef void elf_log_callback(struct elf_binary*, void *caller_data, */ /* PTRVALs can be INVALID (ie, NULL). */ -#define ELF_INVALID_PTRVAL (NULL) /* returns NULL PTRVAL */ +#define ELF_INVALID_PTRVAL ((elf_ptrval)0) /* returns NULL PTRVAL */ #define ELF_INVALID_HANDLE(structname) /* returns NULL handle */ \ ELF_MAKE_HANDLE(structname, ELF_INVALID_PTRVAL) -#define ELF_PTRVAL_VALID(ptrval) (ptrval) /* } */ -#define ELF_HANDLE_VALID(handleval) (handleval) /* } predicates */ -#define ELF_PTRVAL_INVALID(ptrval) ((ptrval) == NULL) /* } */ +#define ELF_PTRVAL_VALID(ptrval) (!!(ptrval)) /* } */ +#define ELF_HANDLE_VALID(handleval) (!!(handleval).ptrval) /* } predicates */ +#define ELF_PTRVAL_INVALID(ptrval) (!ELF_PTRVAL_VALID((ptrval))) /* } */ + +#define ELF_MAX_PTRVAL (~(elf_ptrval)0) + /* PTRVAL value guaranteed to compare > to any valid PTRVAL */ /* For internal use by other macros here */ #define ELF__HANDLE_FIELD_TYPE(handleval, elm) \ - typeof((handleval)->elm) + typeof((handleval).typeonly->elm) #define ELF__HANDLE_FIELD_OFFSET(handleval, elm) \ - offsetof(typeof(*(handleval)),elm) + offsetof(typeof(*(handleval).typeonly),elm) /* ------------------------------------------------------------------------ */ @@ -182,7 +198,7 @@ ELF_DEFINE_HANDLE(elf_note) struct elf_binary { /* elf binary */ - const char *image; + const void *image_base; size_t size; char class; char data; @@ -190,10 +206,16 @@ struct elf_binary { ELF_HANDLE_DECL(elf_ehdr) ehdr; ELF_PTRVAL_CONST_CHAR sec_strtab; ELF_HANDLE_DECL(elf_shdr) sym_tab; - ELF_PTRVAL_CONST_CHAR sym_strtab; + uint64_t sym_strtab; /* loaded to */ - char *dest; + /* + * dest_base and dest_size are trusted and must be correct; + * whenever dest_size is not 0, both of these must be valid + * so long as the struct elf_binary is in use. + */ + char *dest_base; + size_t dest_size; uint64_t pstart; uint64_t pend; uint64_t reloc_offset; @@ -201,12 +223,22 @@ struct elf_binary { uint64_t bsd_symtab_pstart; uint64_t bsd_symtab_pend; + /* + * caller''s other acceptable destination + * + * Again, these are trusted and must be valid (or 0) so long + * as the struct elf_binary is in use. + */ + void *caller_xdest_base; + uint64_t caller_xdest_size; + #ifndef __XEN__ /* misc */ elf_log_callback *log_callback; void *log_caller_data; #endif int verbose; + const char *broken; }; /* ------------------------------------------------------------------------ */ @@ -224,22 +256,27 @@ struct elf_binary { #define elf_lsb(elf) (ELFDATA2LSB == (elf)->data) #define elf_swap(elf) (NATIVE_ELFDATA != (elf)->data) -#define elf_uval(elf, str, elem) \ - ((ELFCLASS64 == (elf)->class) \ - ? elf_access_unsigned((elf), (str), \ - offsetof(typeof(*(str)),e64.elem), \ - sizeof((str)->e64.elem)) \ - : elf_access_unsigned((elf), (str), \ - offsetof(typeof(*(str)),e32.elem), \ - sizeof((str)->e32.elem))) +#define elf_uval_3264(elf, handle, elem) \ + elf_access_unsigned((elf), (handle).ptrval, \ + offsetof(typeof(*(handle).typeonly),elem), \ + sizeof((handle).typeonly->elem)) + +#define elf_uval(elf, handle, elem) \ + ((ELFCLASS64 == (elf)->class) \ + ? elf_uval_3264(elf, handle, e64.elem) \ + : elf_uval_3264(elf, handle, e32.elem)) /* * Reads an unsigned field in a header structure in the ELF. * str is a HANDLE, and elem is the field name in it. */ -#define elf_size(elf, str) \ + +#define elf_size(elf, handle_or_handletype) ({ \ + typeof(handle_or_handletype) elf_size__dummy; \ ((ELFCLASS64 == (elf)->class) \ - ? sizeof((str)->e64) : sizeof((str)->e32)) + ? sizeof(elf_size__dummy.typeonly->e64) \ + : sizeof(elf_size__dummy.typeonly->e32)); \ +}) /* * Returns the size of the substructure for the appropriate 32/64-bitness. * str should be a HANDLE. @@ -251,23 +288,37 @@ uint64_t elf_access_unsigned(struct elf_binary *elf, ELF_PTRVAL_CONST_VOID ptr, uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr); +const char *elf_strval(struct elf_binary *elf, elf_ptrval start); + /* may return NULL if the string is out of range etc. */ -#define elf_strval(elf,x) ((const char*)(x)) /* may return NULL in the future */ -#define elf_strfmt(elf,x) ((const char*)(x)) /* will return (invalid) instead */ +const char *elf_strfmt(struct elf_binary *elf, elf_ptrval start); + /* like elf_strval but returns "(invalid)" instead of NULL */ -#define elf_memcpy_safe(elf, dst, src, sz) memcpy((dst),(src),(sz)) -#define elf_memset_safe(elf, dst, c, sz) memset((dst),(c),(sz)) +void elf_memcpy_safe(struct elf_binary*, elf_ptrval dst, elf_ptrval src, size_t); +void elf_memset_safe(struct elf_binary*, elf_ptrval dst, int c, size_t); /* - * Versions of memcpy and memset which will (in the next patch) - * arrange never to write outside permitted areas. + * Versions of memcpy and memset which arrange never to write + * outside permitted areas. */ -#define elf_store_val(elf, type, ptr, val) (*(type*)(ptr) = (val)) +int elf_access_ok(struct elf_binary * elf, + uint64_t ptrval, size_t size); + +#define elf_store_val(elf, type, ptr, val) \ + ({ \ + typeof(type) elf_store__val = (val); \ + elf_ptrval elf_store__targ = ptr; \ + if (elf_access_ok((elf), elf_store__targ, \ + sizeof(elf_store__val))) { \ + elf_memcpy_unchecked((void*)elf_store__targ, &elf_store__val, \ + sizeof(elf_store__val)); \ + } \ + }) \ /* Stores a value at a particular PTRVAL. */ -#define elf_store_field(elf, hdr, elm, val) \ - (elf_store_val((elf), ELF__HANDLE_FIELD_TYPE(hdr, elm), \ - &((hdr)->elm), \ +#define elf_store_field(elf, hdr, elm, val) \ + (elf_store_val((elf), ELF__HANDLE_FIELD_TYPE(hdr, elm), \ + ELF_HANDLE_PTRVAL(hdr) + ELF__HANDLE_FIELD_OFFSET(hdr, elm), \ (val))) /* Stores a 32/64-bit field. hdr is a HANDLE and elm is the field name. */ @@ -306,6 +357,10 @@ int elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr) /* xc_libelf_loader.c */ int elf_init(struct elf_binary *elf, const char *image, size_t size); + /* + * image and size must be correct. They will be recorded in + * *elf, and must remain valid while the elf is in use. + */ #ifdef __XEN__ void elf_set_verbose(struct elf_binary *elf); #else @@ -321,6 +376,9 @@ uint64_t elf_lookup_addr(struct elf_binary *elf, const char *symbol); void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart); /* private */ +void elf_mark_broken(struct elf_binary *elf, const char *msg); +const char *elf_check_broken(const struct elf_binary *elf); /* NULL means OK */ + /* ------------------------------------------------------------------------ */ /* xc_libelf_relocate.c */ @@ -395,16 +453,38 @@ int elf_xen_parse_guest_info(struct elf_binary *elf, int elf_xen_parse(struct elf_binary *elf, struct elf_dom_parms *parms); -#define elf_memcpy_unchecked memcpy -#define elf_memset_unchecked memset +static inline void *elf_memcpy_unchecked(void *dest, const void *src, size_t n) + { return memcpy(dest, src, n); } +static inline void *elf_memmove_unchecked(void *dest, const void *src, size_t n) + { return memmove(dest, src, n); } +static inline void *elf_memset_unchecked(void *s, int c, size_t n) + { return memset(s, c, n); } /* - * Unsafe versions of memcpy and memset which take actual C - * pointers. These are just like real memcpy and memset. + * Unsafe versions of memcpy, memmove memset which take actual C + * pointers. These are just like the real functions. + * We provide these so that in libelf-private.h we can #define + * memcpy, memset and memmove to undefined MISTAKE things. */ -#define ELF_ADVANCE_DEST(elf, amount) elf->dest += (amount) - /* Advances past amount bytes of the current destination area. */ +/* Advances past amount bytes of the current destination area. */ +static inline void ELF_ADVANCE_DEST(struct elf_binary *elf, uint64_t amount) +{ + if ( elf->dest_base == NULL ) + { + elf_mark_broken(elf, "advancing in null image"); + } + else if ( elf->dest_size >= amount ) + { + elf->dest_base += amount; + elf->dest_size -= amount; + } + else + { + elf->dest_size = 0; + elf_mark_broken(elf, "advancing past end (image very short?)"); + } +} #endif /* __XEN_LIBELF_H__ */ -- 1.7.2.5
Ian Jackson
2013-Jun-13 18:14 UTC
[PATCH 12/23] libelf: Check pointer references in elf_is_elfbinary
elf_is_elfbinary didn''t take a length parameter and could potentially access out of range when provided with a very short image. We only need to check the size is enough for the actual dereference in elf_is_elfbinary; callers are just using it to check the magic number and do their own checks (usually via the new elf_ptrval system) before dereferencing other parts of the header. This is part of the fix to a security issue, XSA-55. Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> Acked-by: Ian Campbell <ian.campbell@citrix.com> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> v7: Add a comment about the limited function of elf_is_elfbinary. v2: Style fix. Fix commit message subject. --- tools/libxc/xc_dom_elfloader.c | 2 +- xen/arch/x86/bzimage.c | 4 ++-- xen/common/libelf/libelf-loader.c | 2 +- xen/common/libelf/libelf-tools.c | 9 ++++++--- xen/include/xen/libelf.h | 4 +++- 5 files changed, 13 insertions(+), 8 deletions(-) diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c index c038d1c..f14b053 100644 --- a/tools/libxc/xc_dom_elfloader.c +++ b/tools/libxc/xc_dom_elfloader.c @@ -93,7 +93,7 @@ static int check_elf_kernel(struct xc_dom_image *dom, int verbose) return -EINVAL; } - if ( !elf_is_elfbinary(dom->kernel_blob) ) + if ( !elf_is_elfbinary(dom->kernel_blob, dom->kernel_size) ) { if ( verbose ) xc_dom_panic(dom->xch, diff --git a/xen/arch/x86/bzimage.c b/xen/arch/x86/bzimage.c index c5519d8..58fda16 100644 --- a/xen/arch/x86/bzimage.c +++ b/xen/arch/x86/bzimage.c @@ -220,7 +220,7 @@ unsigned long __init bzimage_headroom(char *image_start, image_length = hdr->payload_length; } - if ( elf_is_elfbinary(image_start) ) + if ( elf_is_elfbinary(image_start, image_length) ) return 0; orig_image_len = image_length; @@ -251,7 +251,7 @@ int __init bzimage_parse(char *image_base, char **image_start, unsigned long *im *image_len = hdr->payload_length; } - if ( elf_is_elfbinary(*image_start) ) + if ( elf_is_elfbinary(*image_start, *image_len) ) return 0; BUG_ON(!(image_base < *image_start)); diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c index 878552e..6c43c34 100644 --- a/xen/common/libelf/libelf-loader.c +++ b/xen/common/libelf/libelf-loader.c @@ -29,7 +29,7 @@ int elf_init(struct elf_binary *elf, const char *image_input, size_t size) ELF_HANDLE_DECL(elf_shdr) shdr; uint64_t i, count, section, offset; - if ( !elf_is_elfbinary(image_input) ) + if ( !elf_is_elfbinary(image_input, size) ) { elf_err(elf, "%s: not an ELF binary\n", __FUNCTION__); return -1; diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c index 08ab027..b613593 100644 --- a/xen/common/libelf/libelf-tools.c +++ b/xen/common/libelf/libelf-tools.c @@ -332,11 +332,14 @@ ELF_HANDLE_DECL(elf_note) elf_note_next(struct elf_binary *elf, ELF_HANDLE_DECL( /* ------------------------------------------------------------------------ */ -int elf_is_elfbinary(const void *image) +int elf_is_elfbinary(const void *image_start, size_t image_size) { - const Elf32_Ehdr *ehdr = image; + const Elf32_Ehdr *ehdr = image_start; - return IS_ELF(*ehdr); /* fixme unchecked */ + if ( image_size < sizeof(*ehdr) ) + return 0; + + return IS_ELF(*ehdr); } int elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr) diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h index f3f18da..df93f2c 100644 --- a/xen/include/xen/libelf.h +++ b/xen/include/xen/libelf.h @@ -350,7 +350,9 @@ uint64_t elf_note_numeric_array(struct elf_binary *, ELF_HANDLE_DECL(elf_note), unsigned int unitsz, unsigned int idx); ELF_HANDLE_DECL(elf_note) elf_note_next(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note); -int elf_is_elfbinary(const void *image); +/* (Only) checks that the image has the right magic number. */ +int elf_is_elfbinary(const void *image_start, size_t image_size); + int elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr); /* ------------------------------------------------------------------------ */ -- 1.7.2.5
Ian Jackson
2013-Jun-13 18:14 UTC
[PATCH 13/23] libelf: Make all callers call elf_check_broken
This arranges that if the new pointer reference error checking tripped, we actually get a message about it. In this patch these messages do not change the actual return values from the various functions: so pointer reference errors do not prevent loading. This is for fear that some existing kernels might cause the code to make these wild references, which would then break, which is not a good thing in a security patch. In xen/arch/x86/domain_build.c we have to introduce an "out" label and change all of the "return rc" beyond the relevant point into "goto out". This is part of the fix to a security issue, XSA-55. 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> v5: Fix two whitespace errors. v3.1: Add error check to xc_dom_parse_elf_kernel. Move check in xc_hvm_build_x86.c:setup_guest to right place. v2 was Acked-by: Ian Campbell <ian.campbell@citrix.com> v2 was Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> v2: Style fixes. --- tools/libxc/xc_dom_elfloader.c | 25 +++++++++++++++++++++---- tools/libxc/xc_hvm_build_x86.c | 3 +++ tools/xcutils/readnotes.c | 3 +++ xen/arch/arm/kernel.c | 10 ++++++++++ xen/arch/x86/domain_build.c | 28 +++++++++++++++++++++------- 5 files changed, 58 insertions(+), 11 deletions(-) diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c index f14b053..a0d39b3 100644 --- a/tools/libxc/xc_dom_elfloader.c +++ b/tools/libxc/xc_dom_elfloader.c @@ -274,6 +274,13 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom, elf_store_field(elf, shdr, e32.sh_name, 0); } + if ( elf_check_broken(&syms) ) + DOMPRINTF("%s: symbols ELF broken: %s", __FUNCTION__, + elf_check_broken(&syms)); + if ( elf_check_broken(elf) ) + DOMPRINTF("%s: ELF broken: %s", __FUNCTION__, + elf_check_broken(elf)); + if ( tables == 0 ) { DOMPRINTF("%s: no symbol table present", __FUNCTION__); @@ -310,19 +317,23 @@ static int xc_dom_parse_elf_kernel(struct xc_dom_image *dom) { xc_dom_panic(dom->xch, XC_INVALID_KERNEL, "%s: ELF image" " has no shstrtab", __FUNCTION__); - return -EINVAL; + rc = -EINVAL; + goto out; } /* parse binary and get xen meta info */ elf_parse_binary(elf); if ( (rc = elf_xen_parse(elf, &dom->parms)) != 0 ) - return rc; + { + goto out; + } if ( elf_xen_feature_get(XENFEAT_dom0, dom->parms.f_required) ) { xc_dom_panic(dom->xch, XC_INVALID_KERNEL, "%s: Kernel does not" " support unprivileged (DomU) operation", __FUNCTION__); - return -EINVAL; + rc = -EINVAL; + goto out; } /* find kernel segment */ @@ -336,7 +347,13 @@ static int xc_dom_parse_elf_kernel(struct xc_dom_image *dom) DOMPRINTF("%s: %s: 0x%" PRIx64 " -> 0x%" PRIx64 "", __FUNCTION__, dom->guest_type, dom->kernel_seg.vstart, dom->kernel_seg.vend); - return 0; + rc = 0; +out: + if ( elf_check_broken(elf) ) + DOMPRINTF("%s: ELF broken: %s", __FUNCTION__, + elf_check_broken(elf)); + + return rc; } static int xc_dom_load_elf_kernel(struct xc_dom_image *dom) diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c index eff55a4..8bb0178 100644 --- a/tools/libxc/xc_hvm_build_x86.c +++ b/tools/libxc/xc_hvm_build_x86.c @@ -524,6 +524,9 @@ static int setup_guest(xc_interface *xch, error_out: rc = -1; out: + if ( elf_check_broken(&elf) ) + ERROR("HVM ELF broken: %s", elf_check_broken(&elf)); + /* ensure no unclaimed pages are left unused */ xc_domain_claim_pages(xch, dom, 0 /* cancels the claim */); diff --git a/tools/xcutils/readnotes.c b/tools/xcutils/readnotes.c index cfae994..d1f7a30 100644 --- a/tools/xcutils/readnotes.c +++ b/tools/xcutils/readnotes.c @@ -301,6 +301,9 @@ int main(int argc, char **argv) printf("__xen_guest: %s\n", elf_strfmt(&elf, elf_section_start(&elf, shdr))); + if (elf_check_broken(&elf)) + printf("warning: broken ELF: %s\n", elf_check_broken(&elf)); + return 0; } diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c index 8f4a60d..43cf2ab 100644 --- a/xen/arch/arm/kernel.c +++ b/xen/arch/arm/kernel.c @@ -171,6 +171,8 @@ static int kernel_try_elf_prepare(struct kernel_info *info, { int rc; + memset(&info->elf.elf, 0, sizeof(info->elf.elf)); + info->kernel_order = get_order_from_bytes(size); info->kernel_img = alloc_xenheap_pages(info->kernel_order, 0); if ( info->kernel_img == NULL ) @@ -194,8 +196,16 @@ static int kernel_try_elf_prepare(struct kernel_info *info, info->entry = info->elf.parms.virt_entry; info->load = kernel_elf_load; + if ( elf_check_broken(&info->elf.elf) ) + printk("Xen: warning: ELF kernel broken: %s\n", + elf_check_broken(&info->elf.elf)); + return 0; err: + if ( elf_check_broken(&info->elf.elf) ) + printk("Xen: ELF kernel broken: %s\n", + elf_check_broken(&info->elf.elf)); + free_xenheap_pages(info->kernel_img, info->kernel_order); return rc; } diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c index db31a91..03fe845 100644 --- a/xen/arch/x86/domain_build.c +++ b/xen/arch/x86/domain_build.c @@ -380,7 +380,7 @@ int __init construct_dom0( #endif elf_parse_binary(&elf); if ( (rc = elf_xen_parse(&elf, &parms)) != 0 ) - return rc; + goto out; /* compatibility check */ compatible = 0; @@ -408,14 +408,16 @@ int __init construct_dom0( if ( !compatible ) { printk("Mismatch between Xen and DOM0 kernel\n"); - return -EINVAL; + rc = -EINVAL; + goto out; } if ( parms.elf_notes[XEN_ELFNOTE_SUPPORTED_FEATURES].type != XEN_ENT_NONE && !test_bit(XENFEAT_dom0, parms.f_supported) ) { printk("Kernel does not support Dom0 operation\n"); - return -EINVAL; + rc = -EINVAL; + goto out; } if ( compat32 ) @@ -596,7 +598,8 @@ int __init construct_dom0( (v_end > HYPERVISOR_COMPAT_VIRT_START(d)) ) { printk("DOM0 image overlaps with Xen private area.\n"); - return -EINVAL; + rc = -EINVAL; + goto out; } if ( is_pv_32on64_domain(d) ) @@ -771,7 +774,7 @@ int __init construct_dom0( if ( rc < 0 ) { printk("Failed to load the kernel binary\n"); - return rc; + goto out; } bootstrap_map(NULL); @@ -783,7 +786,8 @@ int __init construct_dom0( mapcache_override_current(NULL); write_ptbase(current); printk("Invalid HYPERCALL_PAGE field in ELF notes.\n"); - return -1; + rc = -1; + goto out; } hypercall_page_initialise( d, (void *)(unsigned long)parms.virt_hypercall); @@ -1133,9 +1137,19 @@ int __init construct_dom0( BUG_ON(rc != 0); - iommu_dom0_init(dom0); + if ( elf_check_broken(&elf) ) + printk(" Xen warning: dom0 kernel broken ELF: %s\n", + elf_check_broken(&elf)); + iommu_dom0_init(dom0); return 0; + +out: + if ( elf_check_broken(&elf) ) + printk(" Xen dom0 kernel broken ELF: %s\n", + elf_check_broken(&elf)); + + return rc; } /* -- 1.7.2.5
We want to remove uses of "int" because signed integers have undesirable undefined behaviours on overflow. Malicious compilers can turn apparently-correct code into code with security vulnerabilities etc. In this patch we change all the booleans in libelf to C99 bool, from <stdbool.h>. For the one visible libelf boolean in libxc''s public interface we retain the use of int to avoid changing the ABI; libxc converts it to a bool for consumption by libelf. It is OK to change all values only ever used as booleans to _Bool (bool) because conversion from any scalar type to a _Bool works the same as the boolean test in if() or ?: and is always defined (C99 6.3.1.2). But we do need to check that all these variables really are only ever used that way. (It is theoretically possible that the old code truncated some 64-bit values to 32-bit ints which might become zero depending on the value, which would mean a behavioural change in this patch, but it seems implausible that treating 0x????????00000000 as false could have been intended.) This is part of the fix to a security issue, XSA-55. Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> Acked-by: George Dunlap <george.dunlap@eu.citrix.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> v3: Use <stdbool.h>''s bool (or _Bool) instead of defining elf_bool. Split this into a separate patch. --- tools/libxc/xc_dom_elfloader.c | 8 ++++---- xen/common/libelf/libelf-dominfo.c | 2 +- xen/common/libelf/libelf-loader.c | 4 ++-- xen/common/libelf/libelf-private.h | 2 +- xen/common/libelf/libelf-tools.c | 10 +++++----- xen/include/xen/libelf.h | 18 ++++++++++-------- 6 files changed, 23 insertions(+), 21 deletions(-) diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c index a0d39b3..8f9c2fb 100644 --- a/tools/libxc/xc_dom_elfloader.c +++ b/tools/libxc/xc_dom_elfloader.c @@ -34,7 +34,7 @@ /* ------------------------------------------------------------------------ */ static void log_callback(struct elf_binary *elf, void *caller_data, - int iserr, const char *fmt, va_list al) { + bool iserr, const char *fmt, va_list al) { xc_interface *xch = caller_data; xc_reportv(xch, @@ -46,7 +46,7 @@ static void log_callback(struct elf_binary *elf, void *caller_data, void xc_elf_set_logfile(xc_interface *xch, struct elf_binary *elf, int verbose) { - elf_set_log(elf, log_callback, xch, verbose); + elf_set_log(elf, log_callback, xch, verbose /* convert to bool */); } /* ------------------------------------------------------------------------ */ @@ -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, int verbose) +static int check_elf_kernel(struct xc_dom_image *dom, bool verbose) { if ( dom->kernel_blob == NULL ) { @@ -110,7 +110,7 @@ static int xc_dom_probe_elf_kernel(struct xc_dom_image *dom) } static int xc_dom_load_elf_symtab(struct xc_dom_image *dom, - struct elf_binary *elf, int load) + struct elf_binary *elf, bool load) { struct elf_binary syms; ELF_HANDLE_DECL_NONCONST(elf_shdr) shdr; ELF_HANDLE_DECL(elf_shdr) shdr2; diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c index b9a4e25..c4ced67 100644 --- a/xen/common/libelf/libelf-dominfo.c +++ b/xen/common/libelf/libelf-dominfo.c @@ -101,7 +101,7 @@ int elf_xen_parse_note(struct elf_binary *elf, /* *INDENT-OFF* */ static const struct { char *name; - int str; + bool str; } note_desc[] = { [XEN_ELFNOTE_ENTRY] = { "ENTRY", 0}, [XEN_ELFNOTE_HYPERCALL_PAGE] = { "HYPERCALL_PAGE", 0}, diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c index 6c43c34..798f88b 100644 --- a/xen/common/libelf/libelf-loader.c +++ b/xen/common/libelf/libelf-loader.c @@ -92,7 +92,7 @@ int elf_init(struct elf_binary *elf, const char *image_input, size_t size) } #ifndef __XEN__ -void elf_call_log_callback(struct elf_binary *elf, int iserr, +void elf_call_log_callback(struct elf_binary *elf, bool iserr, const char *fmt,...) { va_list al; @@ -107,7 +107,7 @@ void elf_call_log_callback(struct elf_binary *elf, int iserr, } void elf_set_log(struct elf_binary *elf, elf_log_callback *log_callback, - void *log_caller_data, int verbose) + void *log_caller_data, bool verbose) { elf->log_callback = log_callback; elf->log_caller_data = log_caller_data; diff --git a/xen/common/libelf/libelf-private.h b/xen/common/libelf/libelf-private.h index 0bd9e66..ea7e197 100644 --- a/xen/common/libelf/libelf-private.h +++ b/xen/common/libelf/libelf-private.h @@ -77,7 +77,7 @@ #define elf_err(elf, fmt, args ... ) \ elf_call_log_callback(elf, 1, fmt , ## args ); -void elf_call_log_callback(struct elf_binary*, int iserr, const char *fmt,...); +void elf_call_log_callback(struct elf_binary*, bool iserr, const char *fmt,...); #define safe_strcpy(d,s) \ do { strncpy((d),(s),sizeof((d))-1); \ diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c index b613593..0b7b2b6 100644 --- a/xen/common/libelf/libelf-tools.c +++ b/xen/common/libelf/libelf-tools.c @@ -31,7 +31,7 @@ const char *elf_check_broken(const struct elf_binary *elf) return elf->broken; } -static int elf_ptrval_in_range(elf_ptrval ptrval, uint64_t size, +static bool elf_ptrval_in_range(elf_ptrval ptrval, uint64_t size, const void *region, uint64_t regionsize) /* * Returns true if the putative memory area [ptrval,ptrval+size> @@ -53,7 +53,7 @@ static int elf_ptrval_in_range(elf_ptrval ptrval, uint64_t size, return 1; } -int elf_access_ok(struct elf_binary * elf, +bool elf_access_ok(struct elf_binary * elf, uint64_t ptrval, size_t size) { if ( elf_ptrval_in_range(ptrval, size, elf->image_base, elf->size) ) @@ -92,7 +92,7 @@ uint64_t elf_access_unsigned(struct elf_binary * elf, elf_ptrval base, uint64_t moreoffset, size_t size) { elf_ptrval ptrval = base + moreoffset; - int need_swap = elf_swap(elf); + bool need_swap = elf_swap(elf); const uint8_t *u8; const uint16_t *u16; const uint32_t *u32; @@ -332,7 +332,7 @@ ELF_HANDLE_DECL(elf_note) elf_note_next(struct elf_binary *elf, ELF_HANDLE_DECL( /* ------------------------------------------------------------------------ */ -int elf_is_elfbinary(const void *image_start, size_t image_size) +bool elf_is_elfbinary(const void *image_start, size_t image_size) { const Elf32_Ehdr *ehdr = image_start; @@ -342,7 +342,7 @@ int elf_is_elfbinary(const void *image_start, size_t image_size) return IS_ELF(*ehdr); } -int elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr) +bool elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr) { uint64_t p_type = elf_uval(elf, phdr, p_type); uint64_t p_flags = elf_uval(elf, phdr, p_flags); diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h index df93f2c..32b3ce2 100644 --- a/xen/include/xen/libelf.h +++ b/xen/include/xen/libelf.h @@ -29,6 +29,8 @@ #error define architectural endianness #endif +#include <stdbool.h> + #undef ELFSIZE #include "elfstructs.h" #ifdef __XEN__ @@ -42,7 +44,7 @@ struct elf_binary; typedef void elf_log_callback(struct elf_binary*, void *caller_data, - int iserr, const char *fmt, va_list al); + bool iserr, const char *fmt, va_list al); #endif @@ -237,7 +239,7 @@ struct elf_binary { elf_log_callback *log_callback; void *log_caller_data; #endif - int verbose; + bool verbose; const char *broken; }; @@ -301,8 +303,8 @@ void elf_memset_safe(struct elf_binary*, elf_ptrval dst, int c, size_t); * outside permitted areas. */ -int elf_access_ok(struct elf_binary * elf, - uint64_t ptrval, size_t size); +bool elf_access_ok(struct elf_binary * elf, + uint64_t ptrval, size_t size); #define elf_store_val(elf, type, ptr, val) \ ({ \ @@ -351,9 +353,9 @@ uint64_t elf_note_numeric_array(struct elf_binary *, ELF_HANDLE_DECL(elf_note), ELF_HANDLE_DECL(elf_note) elf_note_next(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note); /* (Only) checks that the image has the right magic number. */ -int elf_is_elfbinary(const void *image_start, size_t image_size); +bool elf_is_elfbinary(const void *image_start, size_t image_size); -int elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr); +bool elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr); /* ------------------------------------------------------------------------ */ /* xc_libelf_loader.c */ @@ -367,7 +369,7 @@ int elf_init(struct elf_binary *elf, const char *image, size_t size); void elf_set_verbose(struct elf_binary *elf); #else void elf_set_log(struct elf_binary *elf, elf_log_callback*, - void *log_caller_pointer, int verbose); + void *log_caller_pointer, bool verbose); #endif void elf_parse_binary(struct elf_binary *elf); @@ -419,7 +421,7 @@ struct elf_dom_parms { char xen_ver[16]; char loader[16]; int pae; - int bsd_symtab; + bool bsd_symtab; uint64_t virt_base; uint64_t virt_entry; uint64_t virt_hypercall; -- 1.7.2.5
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. --- tools/libxc/Makefile | 9 +++++- tools/libxc/xc_dom.h | 7 +++-- tools/libxc/xc_dom_elfloader.c | 42 ++++++++++++++++------------- tools/xcutils/readnotes.c | 15 +++++----- xen/common/libelf/Makefile | 2 + xen/common/libelf/libelf-dominfo.c | 52 ++++++++++++++++++----------------- xen/common/libelf/libelf-loader.c | 20 +++++++------- xen/common/libelf/libelf-tools.c | 24 ++++++++-------- xen/include/xen/libelf.h | 21 ++++++++------ 9 files changed, 105 insertions(+), 87 deletions(-) diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile index 4a31282..512a994 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 ad6fdd4..5968e7b 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 8f9c2fb..75e469a 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 d1f7a30..2ca7732 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; diff --git a/xen/common/libelf/Makefile b/xen/common/libelf/Makefile index 18dc8e2..5bf8f76 100644 --- a/xen/common/libelf/Makefile +++ b/xen/common/libelf/Makefile @@ -2,6 +2,8 @@ obj-bin-y := libelf.o SECTIONS := text data $(SPECIAL_DATA_SECTIONS) +CFLAGS += -Wno-pointer-sign + libelf.o: libelf-temp.o Makefile $(OBJCOPY) $(foreach s,$(SECTIONS),--rename-section .$(s)=.init.$(s)) $< $@ diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c index c4ced67..0b07002 100644 --- a/xen/common/libelf/libelf-dominfo.c +++ b/xen/common/libelf/libelf-dominfo.c @@ -29,15 +29,15 @@ static const char *const elf_xen_feature_names[] = { [XENFEAT_pae_pgdir_above_4gb] = "pae_pgdir_above_4gb", [XENFEAT_dom0] = "dom0" }; -static const int elf_xen_features +static const unsigned elf_xen_features sizeof(elf_xen_feature_names) / sizeof(elf_xen_feature_names[0]); -int elf_xen_parse_features(const char *features, +elf_errorstatus elf_xen_parse_features(const char *features, uint32_t *supported, uint32_t *required) { - char feature[64]; - int pos, len, i; + unsigned char feature[64]; + unsigned pos, len, i; if ( features == NULL ) return 0; @@ -94,7 +94,7 @@ int elf_xen_parse_features(const char *features, /* ------------------------------------------------------------------------ */ /* xen elf notes */ -int elf_xen_parse_note(struct elf_binary *elf, +elf_errorstatus elf_xen_parse_note(struct elf_binary *elf, struct elf_dom_parms *parms, ELF_HANDLE_DECL(elf_note) note) { @@ -125,7 +125,7 @@ int elf_xen_parse_note(struct elf_binary *elf, const char *str = NULL; uint64_t val = 0; unsigned int i; - int type = elf_uval(elf, note, type); + unsigned type = elf_uval(elf, note, type); if ( (type >= sizeof(note_desc) / sizeof(note_desc[0])) || (note_desc[type].name == NULL) ) @@ -216,12 +216,14 @@ int elf_xen_parse_note(struct elf_binary *elf, return 0; } -static int elf_xen_parse_notes(struct elf_binary *elf, +#define ELF_NOTE_INVALID (~0U) + +static unsigned elf_xen_parse_notes(struct elf_binary *elf, struct elf_dom_parms *parms, ELF_PTRVAL_CONST_VOID start, ELF_PTRVAL_CONST_VOID end) { - int xen_elfnotes = 0; + unsigned xen_elfnotes = 0; ELF_HANDLE_DECL(elf_note) note; const char *note_name; @@ -237,7 +239,7 @@ static int elf_xen_parse_notes(struct elf_binary *elf, if ( strcmp(note_name, "Xen") ) continue; if ( elf_xen_parse_note(elf, parms, note) ) - return -1; + return ELF_NOTE_INVALID; xen_elfnotes++; } return xen_elfnotes; @@ -246,12 +248,12 @@ static int elf_xen_parse_notes(struct elf_binary *elf, /* ------------------------------------------------------------------------ */ /* __xen_guest section */ -int elf_xen_parse_guest_info(struct elf_binary *elf, +elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf, struct elf_dom_parms *parms) { ELF_PTRVAL_CONST_CHAR h; - char name[32], value[128]; - int len; + unsigned char name[32], value[128]; + unsigned len; h = parms->guest_info; #define STAR(h) (elf_access_unsigned(elf, (h), 0, 1)) @@ -334,13 +336,13 @@ int elf_xen_parse_guest_info(struct elf_binary *elf, /* ------------------------------------------------------------------------ */ /* sanity checks */ -static int elf_xen_note_check(struct elf_binary *elf, +static elf_errorstatus elf_xen_note_check(struct elf_binary *elf, struct elf_dom_parms *parms) { if ( (ELF_PTRVAL_INVALID(parms->elf_note_start)) && (ELF_PTRVAL_INVALID(parms->guest_info)) ) { - int machine = elf_uval(elf, elf->ehdr, e_machine); + unsigned machine = elf_uval(elf, elf->ehdr, e_machine); if ( (machine == EM_386) || (machine == EM_X86_64) ) { elf_err(elf, "%s: ERROR: Not a Xen-ELF image: " @@ -378,7 +380,7 @@ static int elf_xen_note_check(struct elf_binary *elf, return 0; } -static int elf_xen_addr_calc_check(struct elf_binary *elf, +static elf_errorstatus elf_xen_addr_calc_check(struct elf_binary *elf, struct elf_dom_parms *parms) { if ( (parms->elf_paddr_offset != UNSET_ADDR) && @@ -464,13 +466,13 @@ static int elf_xen_addr_calc_check(struct elf_binary *elf, /* ------------------------------------------------------------------------ */ /* glue it all together ... */ -int elf_xen_parse(struct elf_binary *elf, +elf_errorstatus elf_xen_parse(struct elf_binary *elf, struct elf_dom_parms *parms) { ELF_HANDLE_DECL(elf_shdr) shdr; ELF_HANDLE_DECL(elf_phdr) phdr; - int xen_elfnotes = 0; - int i, count, rc; + unsigned xen_elfnotes = 0; + unsigned i, count, more_notes; elf_memset_unchecked(parms, 0, sizeof(*parms)); parms->virt_base = UNSET_ADDR; @@ -495,13 +497,13 @@ int elf_xen_parse(struct elf_binary *elf, if (elf_uval(elf, phdr, p_offset) == 0) continue; - rc = elf_xen_parse_notes(elf, parms, + more_notes = elf_xen_parse_notes(elf, parms, elf_segment_start(elf, phdr), elf_segment_end(elf, phdr)); - if ( rc == -1 ) + if ( more_notes == ELF_NOTE_INVALID ) return -1; - xen_elfnotes += rc; + xen_elfnotes += more_notes; } /* @@ -518,17 +520,17 @@ int elf_xen_parse(struct elf_binary *elf, if ( elf_uval(elf, shdr, sh_type) != SHT_NOTE ) continue; - rc = elf_xen_parse_notes(elf, parms, + more_notes = elf_xen_parse_notes(elf, parms, elf_section_start(elf, shdr), elf_section_end(elf, shdr)); - if ( rc == -1 ) + if ( more_notes == ELF_NOTE_INVALID ) return -1; - if ( xen_elfnotes == 0 && rc > 0 ) + if ( xen_elfnotes == 0 && more_notes > 0 ) elf_msg(elf, "%s: using notes from SHT_NOTE section\n", __FUNCTION__); - xen_elfnotes += rc; + xen_elfnotes += more_notes; } } diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c index 798f88b..937c99b 100644 --- a/xen/common/libelf/libelf-loader.c +++ b/xen/common/libelf/libelf-loader.c @@ -24,7 +24,7 @@ /* ------------------------------------------------------------------------ */ -int elf_init(struct elf_binary *elf, const char *image_input, size_t size) +elf_errorstatus elf_init(struct elf_binary *elf, const char *image_input, size_t size) { ELF_HANDLE_DECL(elf_shdr) shdr; uint64_t i, count, section, offset; @@ -114,7 +114,7 @@ void elf_set_log(struct elf_binary *elf, elf_log_callback *log_callback, elf->verbose = verbose; } -static int elf_load_image(struct elf_binary *elf, +static elf_errorstatus elf_load_image(struct elf_binary *elf, ELF_PTRVAL_VOID dst, ELF_PTRVAL_CONST_VOID src, uint64_t filesz, uint64_t memsz) { @@ -129,9 +129,9 @@ void elf_set_verbose(struct elf_binary *elf) elf->verbose = 1; } -static int elf_load_image(struct elf_binary *elf, ELF_PTRVAL_VOID dst, ELF_PTRVAL_CONST_VOID src, uint64_t filesz, uint64_t memsz) +static elf_errorstatus elf_load_image(struct elf_binary *elf, ELF_PTRVAL_VOID dst, ELF_PTRVAL_CONST_VOID src, uint64_t filesz, uint64_t memsz) { - int rc; + elf_errorstatus rc; if ( filesz > ULONG_MAX || memsz > ULONG_MAX ) return -1; /* We trust the dom0 kernel image completely, so we don''t care @@ -151,7 +151,7 @@ void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart) { uint64_t sz; ELF_HANDLE_DECL(elf_shdr) shdr; - int i, type; + unsigned i, type; if ( !ELF_HANDLE_VALID(elf->sym_tab) ) return; @@ -187,7 +187,7 @@ static void elf_load_bsdsyms(struct elf_binary *elf) ELF_PTRVAL_VOID symbase; ELF_PTRVAL_VOID symtab_addr; ELF_HANDLE_DECL_NONCONST(elf_shdr) shdr; - int i, type; + unsigned i, type; if ( !elf->bsd_symtab_pstart ) return; @@ -220,7 +220,7 @@ do { \ elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(shdr), ELF_IMAGE_BASE(elf) + elf_uval(elf, elf->ehdr, e_shoff), sz); - maxva = ELF_OBSOLETE_VOIDP_CAST elf_round_up(elf, (long)maxva + sz); + maxva = ELF_OBSOLETE_VOIDP_CAST elf_round_up(elf, (unsigned long)maxva + sz); for ( i = 0; i < elf_shdr_count(elf); i++ ) { @@ -233,10 +233,10 @@ do { \ elf_memcpy_safe(elf, maxva, elf_section_start(elf, shdr), sz); /* Mangled to be based on ELF header location. */ elf_hdr_elm(elf, shdr, sh_offset, maxva - symtab_addr); - maxva = ELF_OBSOLETE_VOIDP_CAST elf_round_up(elf, (long)maxva + sz); + maxva = ELF_OBSOLETE_VOIDP_CAST elf_round_up(elf, (unsigned long)maxva + sz); } shdr = ELF_MAKE_HANDLE(elf_shdr, ELF_HANDLE_PTRVAL(shdr) + - (long)elf_uval(elf, elf->ehdr, e_shentsize)); + (unsigned long)elf_uval(elf, elf->ehdr, e_shentsize)); } /* Write down the actual sym size. */ @@ -273,7 +273,7 @@ void elf_parse_binary(struct elf_binary *elf) __FUNCTION__, elf->pstart, elf->pend); } -int elf_load_binary(struct elf_binary *elf) +elf_errorstatus elf_load_binary(struct elf_binary *elf) { ELF_HANDLE_DECL(elf_phdr) phdr; uint64_t i, count, paddr, offset, filesz, memsz; diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c index 0b7b2b6..6543f33 100644 --- a/xen/common/libelf/libelf-tools.c +++ b/xen/common/libelf/libelf-tools.c @@ -122,19 +122,19 @@ uint64_t elf_access_unsigned(struct elf_binary * elf, elf_ptrval base, uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr) { - int elf_round = (elf_64bit(elf) ? 8 : 4) - 1; + uint64_t elf_round = (elf_64bit(elf) ? 8 : 4) - 1; return (addr + elf_round) & ~elf_round; } /* ------------------------------------------------------------------------ */ -int elf_shdr_count(struct elf_binary *elf) +unsigned elf_shdr_count(struct elf_binary *elf) { return elf_uval(elf, elf->ehdr, e_shnum); } -int elf_phdr_count(struct elf_binary *elf) +unsigned elf_phdr_count(struct elf_binary *elf) { return elf_uval(elf, elf->ehdr, e_phnum); } @@ -144,7 +144,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *n uint64_t count = elf_shdr_count(elf); ELF_HANDLE_DECL(elf_shdr) shdr; const char *sname; - int i; + unsigned i; for ( i = 0; i < count; i++ ) { @@ -156,7 +156,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *n return ELF_INVALID_HANDLE(elf_shdr); } -ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_index(struct elf_binary *elf, int index) +ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_index(struct elf_binary *elf, unsigned index) { uint64_t count = elf_shdr_count(elf); ELF_PTRVAL_CONST_VOID ptr; @@ -170,7 +170,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_index(struct elf_binary *elf, int index) return ELF_MAKE_HANDLE(elf_shdr, ptr); } -ELF_HANDLE_DECL(elf_phdr) elf_phdr_by_index(struct elf_binary *elf, int index) +ELF_HANDLE_DECL(elf_phdr) elf_phdr_by_index(struct elf_binary *elf, unsigned index) { uint64_t count = elf_uval(elf, elf->ehdr, e_phnum); ELF_PTRVAL_CONST_VOID ptr; @@ -264,7 +264,7 @@ ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *sym return ELF_INVALID_HANDLE(elf_sym); } -ELF_HANDLE_DECL(elf_sym) elf_sym_by_index(struct elf_binary *elf, int index) +ELF_HANDLE_DECL(elf_sym) elf_sym_by_index(struct elf_binary *elf, unsigned index) { ELF_PTRVAL_CONST_VOID ptr = elf_section_start(elf, elf->sym_tab); ELF_HANDLE_DECL(elf_sym) sym; @@ -280,7 +280,7 @@ const char *elf_note_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note) { - int namesz = (elf_uval(elf, note, namesz) + 3) & ~3; + unsigned namesz = (elf_uval(elf, note, namesz) + 3) & ~3; return ELF_HANDLE_PTRVAL(note) + elf_size(elf, note) + namesz; } @@ -288,7 +288,7 @@ ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_ uint64_t elf_note_numeric(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note) { ELF_PTRVAL_CONST_VOID desc = elf_note_desc(elf, note); - int descsz = elf_uval(elf, note, descsz); + unsigned descsz = elf_uval(elf, note, descsz); switch (descsz) { @@ -306,7 +306,7 @@ uint64_t elf_note_numeric_array(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note unsigned int unitsz, unsigned int idx) { ELF_PTRVAL_CONST_VOID desc = elf_note_desc(elf, note); - int descsz = elf_uval(elf, note, descsz); + unsigned descsz = elf_uval(elf, note, descsz); if ( descsz % unitsz || idx >= descsz / unitsz ) return 0; @@ -324,8 +324,8 @@ uint64_t elf_note_numeric_array(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note ELF_HANDLE_DECL(elf_note) elf_note_next(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note) { - int namesz = (elf_uval(elf, note, namesz) + 3) & ~3; - int descsz = (elf_uval(elf, note, descsz) + 3) & ~3; + unsigned namesz = (elf_uval(elf, note, namesz) + 3) & ~3; + unsigned descsz = (elf_uval(elf, note, descsz) + 3) & ~3; return ELF_MAKE_HANDLE(elf_note, ELF_HANDLE_PTRVAL(note) + elf_size(elf, note) + namesz + descsz); } diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h index 32b3ce2..87e6f40 100644 --- a/xen/include/xen/libelf.h +++ b/xen/include/xen/libelf.h @@ -31,6 +31,9 @@ #include <stdbool.h> +typedef int elf_errorstatus; /* 0: ok; -ve (normally -1): error */ +typedef int elf_negerrnoval; /* 0: ok; -EFOO: error */ + #undef ELFSIZE #include "elfstructs.h" #ifdef __XEN__ @@ -328,12 +331,12 @@ bool elf_access_ok(struct elf_binary * elf, /* ------------------------------------------------------------------------ */ /* xc_libelf_tools.c */ -int elf_shdr_count(struct elf_binary *elf); -int elf_phdr_count(struct elf_binary *elf); +unsigned elf_shdr_count(struct elf_binary *elf); +unsigned elf_phdr_count(struct elf_binary *elf); ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *name); -ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_index(struct elf_binary *elf, int index); -ELF_HANDLE_DECL(elf_phdr) elf_phdr_by_index(struct elf_binary *elf, int index); +ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_index(struct elf_binary *elf, unsigned index); +ELF_HANDLE_DECL(elf_phdr) elf_phdr_by_index(struct elf_binary *elf, unsigned index); const char *elf_section_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr); /* might return NULL if inputs are invalid */ ELF_PTRVAL_CONST_VOID elf_section_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr); @@ -343,7 +346,7 @@ ELF_PTRVAL_CONST_VOID elf_segment_start(struct elf_binary *elf, ELF_HANDLE_DECL( ELF_PTRVAL_CONST_VOID elf_segment_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr); ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *symbol); -ELF_HANDLE_DECL(elf_sym) elf_sym_by_index(struct elf_binary *elf, int index); +ELF_HANDLE_DECL(elf_sym) elf_sym_by_index(struct elf_binary *elf, unsigned index); const char *elf_note_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note); /* may return NULL */ ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note); @@ -360,7 +363,7 @@ bool elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr /* ------------------------------------------------------------------------ */ /* xc_libelf_loader.c */ -int elf_init(struct elf_binary *elf, const char *image, size_t size); +elf_errorstatus elf_init(struct elf_binary *elf, const char *image, size_t size); /* * image and size must be correct. They will be recorded in * *elf, and must remain valid while the elf is in use. @@ -373,7 +376,7 @@ void elf_set_log(struct elf_binary *elf, elf_log_callback*, #endif void elf_parse_binary(struct elf_binary *elf); -int elf_load_binary(struct elf_binary *elf); +elf_errorstatus elf_load_binary(struct elf_binary *elf); ELF_PTRVAL_VOID elf_get_ptr(struct elf_binary *elf, unsigned long addr); uint64_t elf_lookup_addr(struct elf_binary *elf, const char *symbol); @@ -386,7 +389,7 @@ const char *elf_check_broken(const struct elf_binary *elf); /* NULL means OK */ /* ------------------------------------------------------------------------ */ /* xc_libelf_relocate.c */ -int elf_reloc(struct elf_binary *elf); +elf_errorstatus elf_reloc(struct elf_binary *elf); /* ------------------------------------------------------------------------ */ /* xc_libelf_dominfo.c */ @@ -420,7 +423,7 @@ struct elf_dom_parms { char guest_ver[16]; char xen_ver[16]; char loader[16]; - int pae; + int pae; /* some kind of enum apparently */ bool bsd_symtab; uint64_t virt_base; uint64_t virt_entry; -- 1.7.2.5
Ensure that libelf does not have any loops which can run away indefinitely even if the input is bogus. (Grepped for \bfor, \bwhile and \bgoto in libelf and xc_dom_*loader*.c.) Changes needed: * elf_note_next uses the note''s unchecked alleged length, which might wrap round. If it does, return ELF_MAX_PTRVAL (0xfff..fff) instead, which will be beyond the end of the section and so terminate the caller''s loop. Also check that the returned psuedopointer is sane. * In various loops over section and program headers, check that the calculated header pointer is still within the image, and quit the loop if it isn''t. * Some fixed limits to avoid potentially O(image_size^2) loops: - maximum length of strings: 4K (longer ones ignored totally) - maximum total number of ELF notes: 65536 (any more are ignored) * Check that the total program contents (text, data) we copy or initialise doesn''t exceed twice the output image area size. * Remove an entirely useless loop from elf_xen_parse (!) * Replace a nested search loop in in xc_dom_load_elf_symtab in xc_dom_elfloader.c by a precomputation of a bitmap of referenced symtabs. We have not changed loops which might, in principle, iterate over the whole image - even if they might do so one byte at a time with a nontrivial access check function in the middle. This is part of the fix to a security issue, XSA-55. Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> v8: Fix the two loops in libelf-dominfo.c; the comment about PT_NOTE and SHT_NOTE wasn''t true because the checks did "continue", not "break". Add a comment about elf_note_next''s expectations of the caller''s loop conditions (which most plausible callers will follow anyway). v5: Fix regression due to wrong image size loop limit calculation. Check return value from xc_dom_malloc. v4: Fix regression due to misplacement of test in elf_shdr_by_name (uninitialised variable). Introduce fixed limits. Avoid O(size^2) loops. Check returned psuedopointer from elf_note_next is correct. A few style fixes. v3: Fix a whitespace error. v2: BUGFIX: elf_shdr_by_name, elf_note_next: Reject new <= old, not just <. elf_shdr_by_name: Change order of checks to be a bit clearer. elf_load_bsdsyms: shdr loop check, improve chance of brokenness detection. Style fixes. --- tools/libxc/xc_dom_elfloader.c | 33 ++++++++++++++++++------- xen/common/libelf/libelf-dominfo.c | 43 ++++++++++++++++++++------------ xen/common/libelf/libelf-loader.c | 47 ++++++++++++++++++++++++++++++++++- xen/common/libelf/libelf-tools.c | 28 ++++++++++++++++++++- xen/include/xen/libelf.h | 13 ++++++++++ 5 files changed, 135 insertions(+), 29 deletions(-) diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c index 75e469a..3b835ee 100644 --- a/tools/libxc/xc_dom_elfloader.c +++ b/tools/libxc/xc_dom_elfloader.c @@ -28,6 +28,7 @@ #include "xg_private.h" #include "xc_dom.h" +#include "xc_bitops.h" #define XEN_VER "xen-3.0" @@ -118,6 +119,7 @@ static elf_errorstatus xc_dom_load_elf_symtab(struct xc_dom_image *dom, ELF_PTRVAL_CHAR hdr; size_t size; unsigned h, count, type, i, tables = 0; + unsigned long *strtab_referenced = NULL; if ( elf_swap(elf) ) { @@ -218,22 +220,35 @@ static elf_errorstatus xc_dom_load_elf_symtab(struct xc_dom_image *dom, symtab, maxaddr); count = elf_shdr_count(&syms); + /* elf_shdr_count guarantees that count is reasonable */ + + strtab_referenced = xc_dom_malloc(dom, bitmap_size(count)); + if ( strtab_referenced == NULL ) + return -1; + bitmap_clear(strtab_referenced, count); + /* Note the symtabs @h linked to by any strtab @i. */ + for ( i = 0; i < count; i++ ) + { + shdr2 = elf_shdr_by_index(&syms, i); + if ( elf_uval(&syms, shdr2, sh_type) == SHT_SYMTAB ) + { + h = elf_uval(&syms, shdr2, sh_link); + if (h < count) + set_bit(h, strtab_referenced); + } + } + for ( h = 0; h < count; h++ ) { shdr = ELF_OBSOLETE_VOIDP_CAST elf_shdr_by_index(&syms, h); + if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) ) + /* input has an insane section header count field */ + break; type = elf_uval(&syms, shdr, sh_type); if ( type == SHT_STRTAB ) { - /* Look for a strtab @i linked to symtab @h. */ - for ( i = 0; i < count; i++ ) - { - shdr2 = elf_shdr_by_index(&syms, i); - if ( (elf_uval(&syms, shdr2, sh_type) == SHT_SYMTAB) && - (elf_uval(&syms, shdr2, sh_link) == h) ) - break; - } /* Skip symtab @h if we found no corresponding strtab @i. */ - if ( i == count ) + if ( !test_bit(h, strtab_referenced) ) { if ( elf_64bit(&syms) ) elf_store_field(elf, shdr, e64.sh_offset, 0); diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c index 0b07002..8ca2a33 100644 --- a/xen/common/libelf/libelf-dominfo.c +++ b/xen/common/libelf/libelf-dominfo.c @@ -221,7 +221,8 @@ elf_errorstatus elf_xen_parse_note(struct elf_binary *elf, static unsigned elf_xen_parse_notes(struct elf_binary *elf, struct elf_dom_parms *parms, ELF_PTRVAL_CONST_VOID start, - ELF_PTRVAL_CONST_VOID end) + ELF_PTRVAL_CONST_VOID end, + unsigned *total_note_count) { unsigned xen_elfnotes = 0; ELF_HANDLE_DECL(elf_note) note; @@ -233,6 +234,12 @@ static unsigned elf_xen_parse_notes(struct elf_binary *elf, ELF_HANDLE_PTRVAL(note) < parms->elf_note_end; note = elf_note_next(elf, note) ) { + if ( *total_note_count >= ELF_MAX_TOTAL_NOTE_COUNT ) + { + elf_mark_broken(elf, "too many ELF notes"); + break; + } + (*total_note_count)++; note_name = elf_note_name(elf, note); if ( note_name == NULL ) continue; @@ -473,6 +480,7 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr; unsigned xen_elfnotes = 0; unsigned i, count, more_notes; + unsigned total_note_count = 0; elf_memset_unchecked(parms, 0, sizeof(*parms)); parms->virt_base = UNSET_ADDR; @@ -487,6 +495,9 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf, for ( i = 0; i < count; i++ ) { phdr = elf_phdr_by_index(elf, i); + if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(phdr), 1) ) + /* input has an insane program header count field */ + break; if ( elf_uval(elf, phdr, p_type) != PT_NOTE ) continue; @@ -499,7 +510,8 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf, more_notes = elf_xen_parse_notes(elf, parms, elf_segment_start(elf, phdr), - elf_segment_end(elf, phdr)); + elf_segment_end(elf, phdr), + &total_note_count); if ( more_notes == ELF_NOTE_INVALID ) return -1; @@ -516,13 +528,17 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf, for ( i = 0; i < count; i++ ) { shdr = elf_shdr_by_index(elf, i); + if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) ) + /* input has an insane section header count field */ + break; if ( elf_uval(elf, shdr, sh_type) != SHT_NOTE ) continue; more_notes = elf_xen_parse_notes(elf, parms, elf_section_start(elf, shdr), - elf_section_end(elf, shdr)); + elf_section_end(elf, shdr), + &total_note_count); if ( more_notes == ELF_NOTE_INVALID ) return -1; @@ -540,20 +556,15 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf, */ if ( xen_elfnotes == 0 ) { - count = elf_shdr_count(elf); - for ( i = 0; i < count; i++ ) + shdr = elf_shdr_by_name(elf, "__xen_guest"); + if ( ELF_HANDLE_VALID(shdr) ) { - shdr = elf_shdr_by_name(elf, "__xen_guest"); - if ( ELF_HANDLE_VALID(shdr) ) - { - parms->guest_info = elf_section_start(elf, shdr); - parms->elf_note_start = ELF_INVALID_PTRVAL; - parms->elf_note_end = ELF_INVALID_PTRVAL; - elf_msg(elf, "%s: __xen_guest: \"%s\"\n", __FUNCTION__, - elf_strfmt(elf, parms->guest_info)); - elf_xen_parse_guest_info(elf, parms); - break; - } + parms->guest_info = elf_section_start(elf, shdr); + parms->elf_note_start = ELF_INVALID_PTRVAL; + parms->elf_note_end = ELF_INVALID_PTRVAL; + elf_msg(elf, "%s: __xen_guest: \"%s\"\n", __FUNCTION__, + elf_strfmt(elf, parms->guest_info)); + elf_xen_parse_guest_info(elf, parms); } } diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c index 937c99b..47957aa 100644 --- a/xen/common/libelf/libelf-loader.c +++ b/xen/common/libelf/libelf-loader.c @@ -75,6 +75,9 @@ elf_errorstatus elf_init(struct elf_binary *elf, const char *image_input, size_t for ( i = 0; i < count; i++ ) { shdr = elf_shdr_by_index(elf, i); + if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) ) + /* input has an insane section header count field */ + break; if ( elf_uval(elf, shdr, sh_type) != SHT_SYMTAB ) continue; elf->sym_tab = shdr; @@ -170,6 +173,9 @@ void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart) for ( i = 0; i < elf_shdr_count(elf); i++ ) { shdr = elf_shdr_by_index(elf, i); + if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) ) + /* input has an insane section header count field */ + break; type = elf_uval(elf, shdr, sh_type); if ( (type == SHT_STRTAB) || (type == SHT_SYMTAB) ) sz = elf_round_up(elf, sz + elf_uval(elf, shdr, sh_size)); @@ -224,6 +230,9 @@ do { \ for ( i = 0; i < elf_shdr_count(elf); i++ ) { + elf_ptrval old_shdr_p; + elf_ptrval new_shdr_p; + type = elf_uval(elf, shdr, sh_type); if ( (type == SHT_STRTAB) || (type == SHT_SYMTAB) ) { @@ -235,8 +244,16 @@ do { \ elf_hdr_elm(elf, shdr, sh_offset, maxva - symtab_addr); maxva = ELF_OBSOLETE_VOIDP_CAST elf_round_up(elf, (unsigned long)maxva + sz); } - shdr = ELF_MAKE_HANDLE(elf_shdr, ELF_HANDLE_PTRVAL(shdr) + - (unsigned long)elf_uval(elf, elf->ehdr, e_shentsize)); + old_shdr_p = ELF_HANDLE_PTRVAL(shdr); + new_shdr_p = old_shdr_p + elf_uval(elf, elf->ehdr, e_shentsize); + if ( new_shdr_p <= old_shdr_p ) /* wrapped or stuck */ + { + elf_mark_broken(elf, "bad section header length"); + break; + } + if ( !elf_access_ok(elf, new_shdr_p, 1) ) /* outside image */ + break; + shdr = ELF_MAKE_HANDLE(elf_shdr, new_shdr_p); } /* Write down the actual sym size. */ @@ -256,6 +273,9 @@ void elf_parse_binary(struct elf_binary *elf) for ( i = 0; i < count; i++ ) { phdr = elf_phdr_by_index(elf, i); + if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(phdr), 1) ) + /* input has an insane program header count field */ + break; if ( !elf_phdr_is_loadable(elf, phdr) ) continue; paddr = elf_uval(elf, phdr, p_paddr); @@ -278,11 +298,20 @@ elf_errorstatus elf_load_binary(struct elf_binary *elf) ELF_HANDLE_DECL(elf_phdr) phdr; uint64_t i, count, paddr, offset, filesz, memsz; ELF_PTRVAL_VOID dest; + /* + * Let bizarre ELFs write the output image up to twice; this + * calculation is just to ensure our copying loop is no worse than + * O(domain_size). + */ + uint64_t remain_allow_copy = (uint64_t)elf->dest_size * 2; count = elf_uval(elf, elf->ehdr, e_phnum); for ( i = 0; i < count; i++ ) { phdr = elf_phdr_by_index(elf, i); + if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(phdr), 1) ) + /* input has an insane program header count field */ + break; if ( !elf_phdr_is_loadable(elf, phdr) ) continue; paddr = elf_uval(elf, phdr, p_paddr); @@ -290,6 +319,20 @@ elf_errorstatus elf_load_binary(struct elf_binary *elf) filesz = elf_uval(elf, phdr, p_filesz); memsz = elf_uval(elf, phdr, p_memsz); dest = elf_get_ptr(elf, paddr); + + /* + * We need to check that the input image doesn''t have us copy + * the whole image zillions of times, as that could lead to + * O(n^2) time behaviour and possible DoS by a malicous ELF. + */ + if ( remain_allow_copy < memsz ) + { + elf_mark_broken(elf, "program segments total to more" + " than the input image size"); + break; + } + remain_allow_copy -= memsz; + elf_msg(elf, "%s: phdr %" PRIu64 " at 0x%"ELF_PRPTRVAL" -> 0x%"ELF_PRPTRVAL"\n", __func__, i, dest, (ELF_PTRVAL_VOID)(dest + filesz)); if ( elf_load_image(elf, dest, ELF_IMAGE_BASE(elf) + offset, filesz, memsz) != 0 ) diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c index 6543f33..ef13b0d 100644 --- a/xen/common/libelf/libelf-tools.c +++ b/xen/common/libelf/libelf-tools.c @@ -131,7 +131,16 @@ uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr) unsigned elf_shdr_count(struct elf_binary *elf) { - return elf_uval(elf, elf->ehdr, e_shnum); + unsigned count = elf_uval(elf, elf->ehdr, e_shnum); + uint64_t max = elf->size / sizeof(Elf32_Shdr); + if (max > ~(unsigned)0) + max = ~(unsigned)0; /* Xen doesn''t have limits.h :-/ */ + if (count > max) + { + elf_mark_broken(elf, "far too many section headers"); + count = max; + } + return count; } unsigned elf_phdr_count(struct elf_binary *elf) @@ -149,6 +158,9 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *n for ( i = 0; i < count; i++ ) { shdr = elf_shdr_by_index(elf, i); + if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) ) + /* input has an insane section header count field */ + break; sname = elf_section_name(elf, shdr); if ( sname && !strcmp(sname, name) ) return shdr; @@ -204,6 +216,11 @@ const char *elf_strval(struct elf_binary *elf, elf_ptrval start) if ( !elf_access_unsigned(elf, start, length, 1) ) /* ok */ return ELF_UNSAFE_PTR(start); + if ( length >= ELF_MAX_STRING_LENGTH ) + { + elf_mark_broken(elf, "excessively long string"); + return NULL; + } } } @@ -327,7 +344,14 @@ ELF_HANDLE_DECL(elf_note) elf_note_next(struct elf_binary *elf, ELF_HANDLE_DECL( unsigned namesz = (elf_uval(elf, note, namesz) + 3) & ~3; unsigned descsz = (elf_uval(elf, note, descsz) + 3) & ~3; - return ELF_MAKE_HANDLE(elf_note, ELF_HANDLE_PTRVAL(note) + elf_size(elf, note) + namesz + descsz); + elf_ptrval ptrval = ELF_HANDLE_PTRVAL(note) + + elf_size(elf, note) + namesz + descsz; + + if ( ( ptrval <= ELF_HANDLE_PTRVAL(note) || /* wrapped or stuck */ + !elf_access_ok(elf, ELF_HANDLE_PTRVAL(note), 1) ) ) + ptrval = ELF_MAX_PTRVAL; /* terminate caller''s loop */ + + return ELF_MAKE_HANDLE(elf_note, ptrval); } /* ------------------------------------------------------------------------ */ diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h index 87e6f40..63d056d 100644 --- a/xen/include/xen/libelf.h +++ b/xen/include/xen/libelf.h @@ -51,6 +51,9 @@ typedef void elf_log_callback(struct elf_binary*, void *caller_data, #endif +#define ELF_MAX_STRING_LENGTH 4096 +#define ELF_MAX_TOTAL_NOTE_COUNT 65536 + /* ------------------------------------------------------------------------ */ /* Macros for accessing the input image and output area. */ @@ -353,6 +356,16 @@ ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_ uint64_t elf_note_numeric(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note); uint64_t elf_note_numeric_array(struct elf_binary *, ELF_HANDLE_DECL(elf_note), unsigned int unitsz, unsigned int idx); + +/* + * If you use elf_note_next in a loop, you must put a nontrivial upper + * bound on the returned value as part of your loop condition. In + * some cases elf_note_next will substitute ELF_PTRVAL_MAX as return + * value to indicate that the iteration isn''t going well (for example, + * the putative "next" value would be earlier in memory). In this + * case the caller''s loop must terminate. Checking against the + * end of the notes segment with a strict inequality is sufficient. + */ ELF_HANDLE_DECL(elf_note) elf_note_next(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note); /* (Only) checks that the image has the right magic number. */ -- 1.7.2.5
Abolish ELF_PTRVAL_[CONST_]{CHAR,VOID}; change uses to elf_ptrval. Abolish ELF_HANDLE_DECL_NONCONST; change uses to ELF_HANDLE_DECL. Abolish ELF_OBSOLETE_VOIDP_CAST; simply remove all uses. No functional change. (Verified by diffing assembler output.) This is part of the fix to a security issue, XSA-55. Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> Acked-by: Ian Campbell <ian.campbell@citrix.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> v2: New patch. --- tools/libxc/xc_dom_elfloader.c | 8 +++--- tools/xcutils/readnotes.c | 2 +- xen/common/libelf/libelf-dominfo.c | 6 ++-- xen/common/libelf/libelf-loader.c | 24 +++++++++--------- xen/common/libelf/libelf-tools.c | 24 +++++++++--------- xen/include/xen/libelf.h | 48 +++++++++--------------------------- 6 files changed, 44 insertions(+), 68 deletions(-) diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c index 3b835ee..f2bc2f5 100644 --- a/tools/libxc/xc_dom_elfloader.c +++ b/tools/libxc/xc_dom_elfloader.c @@ -114,9 +114,9 @@ static elf_errorstatus xc_dom_load_elf_symtab(struct xc_dom_image *dom, struct elf_binary *elf, bool load) { struct elf_binary syms; - ELF_HANDLE_DECL_NONCONST(elf_shdr) shdr; ELF_HANDLE_DECL(elf_shdr) shdr2; + ELF_HANDLE_DECL(elf_shdr) shdr; ELF_HANDLE_DECL(elf_shdr) shdr2; xen_vaddr_t symtab, maxaddr; - ELF_PTRVAL_CHAR hdr; + elf_ptrval hdr; size_t size; unsigned h, count, type, i, tables = 0; unsigned long *strtab_referenced = NULL; @@ -240,7 +240,7 @@ static elf_errorstatus xc_dom_load_elf_symtab(struct xc_dom_image *dom, for ( h = 0; h < count; h++ ) { - shdr = ELF_OBSOLETE_VOIDP_CAST elf_shdr_by_index(&syms, h); + shdr = elf_shdr_by_index(&syms, h); if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) ) /* input has an insane section header count field */ break; @@ -276,7 +276,7 @@ static elf_errorstatus xc_dom_load_elf_symtab(struct xc_dom_image *dom, if ( load ) { shdr2 = elf_shdr_by_index(elf, h); - elf_memcpy_safe(elf, ELF_OBSOLETE_VOIDP_CAST elf_section_start(&syms, shdr), + elf_memcpy_safe(elf, elf_section_start(&syms, shdr), elf_section_start(elf, shdr2), size); } diff --git a/tools/xcutils/readnotes.c b/tools/xcutils/readnotes.c index 2ca7732..5fa445e 100644 --- a/tools/xcutils/readnotes.c +++ b/tools/xcutils/readnotes.c @@ -80,7 +80,7 @@ static void print_l1_mfn_valid_note(const char *prefix, struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note) { unsigned descsz = elf_uval(elf, note, descsz); - ELF_PTRVAL_CONST_VOID desc = elf_note_desc(elf, note); + elf_ptrval desc = elf_note_desc(elf, note); /* XXX should be able to cope with a list of values. */ switch ( descsz / 2 ) diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c index 8ca2a33..e79b339 100644 --- a/xen/common/libelf/libelf-dominfo.c +++ b/xen/common/libelf/libelf-dominfo.c @@ -220,8 +220,8 @@ elf_errorstatus elf_xen_parse_note(struct elf_binary *elf, static unsigned elf_xen_parse_notes(struct elf_binary *elf, struct elf_dom_parms *parms, - ELF_PTRVAL_CONST_VOID start, - ELF_PTRVAL_CONST_VOID end, + elf_ptrval start, + elf_ptrval end, unsigned *total_note_count) { unsigned xen_elfnotes = 0; @@ -258,7 +258,7 @@ static unsigned elf_xen_parse_notes(struct elf_binary *elf, elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf, struct elf_dom_parms *parms) { - ELF_PTRVAL_CONST_CHAR h; + elf_ptrval h; unsigned char name[32], value[128]; unsigned len; diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c index 47957aa..c5e9141 100644 --- a/xen/common/libelf/libelf-loader.c +++ b/xen/common/libelf/libelf-loader.c @@ -118,7 +118,7 @@ void elf_set_log(struct elf_binary *elf, elf_log_callback *log_callback, } static elf_errorstatus elf_load_image(struct elf_binary *elf, - ELF_PTRVAL_VOID dst, ELF_PTRVAL_CONST_VOID src, + elf_ptrval dst, elf_ptrval src, uint64_t filesz, uint64_t memsz) { elf_memcpy_safe(elf, dst, src, filesz); @@ -132,7 +132,7 @@ void elf_set_verbose(struct elf_binary *elf) elf->verbose = 1; } -static elf_errorstatus elf_load_image(struct elf_binary *elf, ELF_PTRVAL_VOID dst, ELF_PTRVAL_CONST_VOID src, uint64_t filesz, uint64_t memsz) +static elf_errorstatus elf_load_image(struct elf_binary *elf, elf_ptrval dst, elf_ptrval src, uint64_t filesz, uint64_t memsz) { elf_errorstatus rc; if ( filesz > ULONG_MAX || memsz > ULONG_MAX ) @@ -187,12 +187,12 @@ void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart) static void elf_load_bsdsyms(struct elf_binary *elf) { - ELF_HANDLE_DECL_NONCONST(elf_ehdr) sym_ehdr; + ELF_HANDLE_DECL(elf_ehdr) sym_ehdr; unsigned long sz; - ELF_PTRVAL_VOID maxva; - ELF_PTRVAL_VOID symbase; - ELF_PTRVAL_VOID symtab_addr; - ELF_HANDLE_DECL_NONCONST(elf_shdr) shdr; + elf_ptrval maxva; + elf_ptrval symbase; + elf_ptrval symtab_addr; + ELF_HANDLE_DECL(elf_shdr) shdr; unsigned i, type; if ( !elf->bsd_symtab_pstart ) @@ -226,7 +226,7 @@ do { \ elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(shdr), ELF_IMAGE_BASE(elf) + elf_uval(elf, elf->ehdr, e_shoff), sz); - maxva = ELF_OBSOLETE_VOIDP_CAST elf_round_up(elf, (unsigned long)maxva + sz); + maxva = elf_round_up(elf, (unsigned long)maxva + sz); for ( i = 0; i < elf_shdr_count(elf); i++ ) { @@ -242,7 +242,7 @@ do { \ elf_memcpy_safe(elf, maxva, elf_section_start(elf, shdr), sz); /* Mangled to be based on ELF header location. */ elf_hdr_elm(elf, shdr, sh_offset, maxva - symtab_addr); - maxva = ELF_OBSOLETE_VOIDP_CAST elf_round_up(elf, (unsigned long)maxva + sz); + maxva = elf_round_up(elf, (unsigned long)maxva + sz); } old_shdr_p = ELF_HANDLE_PTRVAL(shdr); new_shdr_p = old_shdr_p + elf_uval(elf, elf->ehdr, e_shentsize); @@ -297,7 +297,7 @@ elf_errorstatus elf_load_binary(struct elf_binary *elf) { ELF_HANDLE_DECL(elf_phdr) phdr; uint64_t i, count, paddr, offset, filesz, memsz; - ELF_PTRVAL_VOID dest; + elf_ptrval dest; /* * Let bizarre ELFs write the output image up to twice; this * calculation is just to ensure our copying loop is no worse than @@ -334,7 +334,7 @@ elf_errorstatus elf_load_binary(struct elf_binary *elf) remain_allow_copy -= memsz; elf_msg(elf, "%s: phdr %" PRIu64 " at 0x%"ELF_PRPTRVAL" -> 0x%"ELF_PRPTRVAL"\n", - __func__, i, dest, (ELF_PTRVAL_VOID)(dest + filesz)); + __func__, i, dest, (elf_ptrval)(dest + filesz)); if ( elf_load_image(elf, dest, ELF_IMAGE_BASE(elf) + offset, filesz, memsz) != 0 ) return -1; } @@ -343,7 +343,7 @@ elf_errorstatus elf_load_binary(struct elf_binary *elf) return 0; } -ELF_PTRVAL_VOID elf_get_ptr(struct elf_binary *elf, unsigned long addr) +elf_ptrval elf_get_ptr(struct elf_binary *elf, unsigned long addr) { return ELF_REALPTR2PTRVAL(elf->dest_base) + addr - elf->pstart; } diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c index ef13b0d..dae210e 100644 --- a/xen/common/libelf/libelf-tools.c +++ b/xen/common/libelf/libelf-tools.c @@ -171,7 +171,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *n ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_index(struct elf_binary *elf, unsigned index) { uint64_t count = elf_shdr_count(elf); - ELF_PTRVAL_CONST_VOID ptr; + elf_ptrval ptr; if ( index >= count ) return ELF_INVALID_HANDLE(elf_shdr); @@ -185,7 +185,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_index(struct elf_binary *elf, unsigned ind ELF_HANDLE_DECL(elf_phdr) elf_phdr_by_index(struct elf_binary *elf, unsigned index) { uint64_t count = elf_uval(elf, elf->ehdr, e_phnum); - ELF_PTRVAL_CONST_VOID ptr; + elf_ptrval ptr; if ( index >= count ) return ELF_INVALID_HANDLE(elf_phdr); @@ -233,24 +233,24 @@ const char *elf_strfmt(struct elf_binary *elf, elf_ptrval start) return str; } -ELF_PTRVAL_CONST_VOID elf_section_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr) +elf_ptrval elf_section_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr) { return ELF_IMAGE_BASE(elf) + elf_uval(elf, shdr, sh_offset); } -ELF_PTRVAL_CONST_VOID elf_section_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr) +elf_ptrval elf_section_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr) { return ELF_IMAGE_BASE(elf) + elf_uval(elf, shdr, sh_offset) + elf_uval(elf, shdr, sh_size); } -ELF_PTRVAL_CONST_VOID elf_segment_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr) +elf_ptrval elf_segment_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr) { return ELF_IMAGE_BASE(elf) + elf_uval(elf, phdr, p_offset); } -ELF_PTRVAL_CONST_VOID elf_segment_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr) +elf_ptrval elf_segment_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr) { return ELF_IMAGE_BASE(elf) + elf_uval(elf, phdr, p_offset) + elf_uval(elf, phdr, p_filesz); @@ -258,8 +258,8 @@ ELF_PTRVAL_CONST_VOID elf_segment_end(struct elf_binary *elf, ELF_HANDLE_DECL(el ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *symbol) { - ELF_PTRVAL_CONST_VOID ptr = elf_section_start(elf, elf->sym_tab); - ELF_PTRVAL_CONST_VOID end = elf_section_end(elf, elf->sym_tab); + elf_ptrval ptr = elf_section_start(elf, elf->sym_tab); + elf_ptrval end = elf_section_end(elf, elf->sym_tab); ELF_HANDLE_DECL(elf_sym) sym; uint64_t info, name; const char *sym_name; @@ -283,7 +283,7 @@ ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *sym ELF_HANDLE_DECL(elf_sym) elf_sym_by_index(struct elf_binary *elf, unsigned index) { - ELF_PTRVAL_CONST_VOID ptr = elf_section_start(elf, elf->sym_tab); + elf_ptrval ptr = elf_section_start(elf, elf->sym_tab); ELF_HANDLE_DECL(elf_sym) sym; sym = ELF_MAKE_HANDLE(elf_sym, ptr + index * elf_size(elf, sym)); @@ -295,7 +295,7 @@ const char *elf_note_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note return elf_strval(elf, ELF_HANDLE_PTRVAL(note) + elf_size(elf, note)); } -ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note) +elf_ptrval elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note) { unsigned namesz = (elf_uval(elf, note, namesz) + 3) & ~3; @@ -304,7 +304,7 @@ ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_ uint64_t elf_note_numeric(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note) { - ELF_PTRVAL_CONST_VOID desc = elf_note_desc(elf, note); + elf_ptrval desc = elf_note_desc(elf, note); unsigned descsz = elf_uval(elf, note, descsz); switch (descsz) @@ -322,7 +322,7 @@ uint64_t elf_note_numeric(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note uint64_t elf_note_numeric_array(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note, unsigned int unitsz, unsigned int idx) { - ELF_PTRVAL_CONST_VOID desc = elf_note_desc(elf, note); + elf_ptrval desc = elf_note_desc(elf, note); unsigned descsz = elf_uval(elf, note, descsz); if ( descsz % unitsz || idx >= descsz / unitsz ) diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h index 63d056d..7c04ac3 100644 --- a/xen/include/xen/libelf.h +++ b/xen/include/xen/libelf.h @@ -61,13 +61,8 @@ typedef void elf_log_callback(struct elf_binary*, void *caller_data, /* * We abstract away the pointerness of these pointers, replacing * various void*, char* and struct* with the following: - * PTRVAL A pointer to a byte; one can do pointer arithmetic + * elf_ptrval A pointer to a byte; one can do pointer arithmetic * on this. - * This replaces variables which were char*,void* - * and their const versions, so we provide four - * different obsolete declaration macros: - * ELF_PTRVAL_{,CONST}{VOID,CHAR} - * New code can simply use the elf_ptrval typedef. * HANDLE A pointer to a struct. There is one of these types * for each pointer type - that is, for each "structname". * In the arguments to the various HANDLE macros, structname @@ -76,8 +71,6 @@ typedef void elf_log_callback(struct elf_binary*, void *caller_data, * pointers. In the current code attempts to do so will * compile, but in the next patch this will become a * compile error. - * We also provide a second declaration macro for - * pointers which were to const; this is obsolete. */ typedef uintptr_t elf_ptrval; @@ -85,15 +78,9 @@ typedef uintptr_t elf_ptrval; #define ELF_REALPTR2PTRVAL(realpointer) ((elf_ptrval)(realpointer)) /* Converts an actual C pointer into a PTRVAL */ -#define ELF_HANDLE_DECL_NONCONST(structname) structname##_handle /*obsolete*/ #define ELF_HANDLE_DECL(structname) structname##_handle /* Provides a type declaration for a HANDLE. */ -#define ELF_PTRVAL_VOID elf_ptrval /*obsolete*/ -#define ELF_PTRVAL_CHAR elf_ptrval /*obsolete*/ -#define ELF_PTRVAL_CONST_VOID elf_ptrval /*obsolete*/ -#define ELF_PTRVAL_CONST_CHAR elf_ptrval /*obsolete*/ - #ifdef __XEN__ # define ELF_PRPTRVAL "lu" /* @@ -124,17 +111,6 @@ typedef uintptr_t elf_ptrval; #define ELF_HANDLE_PTRVAL(handleval) ((handleval).ptrval) /* Converts a HANDLE to a PTRVAL. */ -#define ELF_OBSOLETE_VOIDP_CAST /*empty*/ - /* - * In some places the old code used to need to - * - cast away const (the existing code uses const a fair - * bit but actually sometimes wants to write to its input) - * from a PTRVAL. - * - convert an integer representing a pointer to a PTRVAL - * Nowadays all of these re uintptr_ts so there is no const problem - * and no need for any casting. - */ - #define ELF_UNSAFE_PTR(ptrval) ((void*)(elf_ptrval)(ptrval)) /* * Turns a PTRVAL into an actual C pointer. Before this is done @@ -212,7 +188,7 @@ struct elf_binary { char data; ELF_HANDLE_DECL(elf_ehdr) ehdr; - ELF_PTRVAL_CONST_CHAR sec_strtab; + elf_ptrval sec_strtab; ELF_HANDLE_DECL(elf_shdr) sym_tab; uint64_t sym_strtab; @@ -290,7 +266,7 @@ struct elf_binary { * str should be a HANDLE. */ -uint64_t elf_access_unsigned(struct elf_binary *elf, ELF_PTRVAL_CONST_VOID ptr, +uint64_t elf_access_unsigned(struct elf_binary *elf, elf_ptrval ptr, uint64_t offset, size_t size); /* Reads a field at arbitrary offset and alignemnt */ @@ -342,17 +318,17 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_index(struct elf_binary *elf, unsigned ind ELF_HANDLE_DECL(elf_phdr) elf_phdr_by_index(struct elf_binary *elf, unsigned index); const char *elf_section_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr); /* might return NULL if inputs are invalid */ -ELF_PTRVAL_CONST_VOID elf_section_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr); -ELF_PTRVAL_CONST_VOID elf_section_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr); +elf_ptrval elf_section_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr); +elf_ptrval elf_section_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_shdr) shdr); -ELF_PTRVAL_CONST_VOID elf_segment_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr); -ELF_PTRVAL_CONST_VOID elf_segment_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr); +elf_ptrval elf_segment_start(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr); +elf_ptrval elf_segment_end(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr); ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char *symbol); ELF_HANDLE_DECL(elf_sym) elf_sym_by_index(struct elf_binary *elf, unsigned index); const char *elf_note_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note); /* may return NULL */ -ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note); +elf_ptrval elf_note_desc(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note); uint64_t elf_note_numeric(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note); uint64_t elf_note_numeric_array(struct elf_binary *, ELF_HANDLE_DECL(elf_note), unsigned int unitsz, unsigned int idx); @@ -391,7 +367,7 @@ void elf_set_log(struct elf_binary *elf, elf_log_callback*, void elf_parse_binary(struct elf_binary *elf); elf_errorstatus elf_load_binary(struct elf_binary *elf); -ELF_PTRVAL_VOID elf_get_ptr(struct elf_binary *elf, unsigned long addr); +elf_ptrval elf_get_ptr(struct elf_binary *elf, unsigned long addr); uint64_t elf_lookup_addr(struct elf_binary *elf, const char *symbol); void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart); /* private */ @@ -426,9 +402,9 @@ struct xen_elfnote { struct elf_dom_parms { /* raw */ - ELF_PTRVAL_CONST_CHAR guest_info; - ELF_PTRVAL_CONST_VOID elf_note_start; - ELF_PTRVAL_CONST_VOID elf_note_end; + elf_ptrval guest_info; + elf_ptrval elf_note_start; + elf_ptrval elf_note_end; struct xen_elfnote elf_notes[XEN_ELFNOTE_MAX + 1]; /* parsed */ -- 1.7.2.5
Ian Jackson
2013-Jun-13 18:14 UTC
[PATCH 18/23] libxc: Add range checking to xc_dom_binloader
This is a simple binary image loader with its own metadata format. However, it is too careless with image-supplied values. Add the following checks: * That the image is bigger than the metadata table; otherwise the pointer arithmetic to calculate the metadata table location may yield undefined and dangerous values. * When clamping the end of the region to search, that we do not calculate pointers beyond the end of the image. The C specification does not permit this and compilers are becoming ever more determined to miscompile code when they can "prove" various falsehoods based on assertions from the C spec. * That the supplied image is big enough for the text we are allegedly copying from it. Otherwise we might have a read overrun and copy the results (perhaps a lot of secret data) into the guest. This is part of the fix to a security issue, XSA-55. Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> v6: Add a missing `return -EINVAL'' (Matthew Daley). Fix an error in the commit message (Matthew Daley). v5: This patch is new in this version of the series. --- tools/libxc/xc_dom_binloader.c | 13 ++++++++++++- 1 files changed, 12 insertions(+), 1 deletions(-) diff --git a/tools/libxc/xc_dom_binloader.c b/tools/libxc/xc_dom_binloader.c index d2de04c..7eaf071 100644 --- a/tools/libxc/xc_dom_binloader.c +++ b/tools/libxc/xc_dom_binloader.c @@ -123,9 +123,12 @@ static struct xen_bin_image_table *find_table(struct xc_dom_image *dom) uint32_t *probe_ptr; uint32_t *probe_end; + if ( dom->kernel_size < sizeof(*table) ) + return NULL; probe_ptr = dom->kernel_blob; probe_end = dom->kernel_blob + dom->kernel_size - sizeof(*table); - if ( (void*)probe_end > (dom->kernel_blob + 8192) ) + if ( dom->kernel_size >= 8192 && + (void*)probe_end > (dom->kernel_blob + 8192) ) probe_end = dom->kernel_blob + 8192; for ( table = NULL; probe_ptr < probe_end; probe_ptr++ ) @@ -282,6 +285,14 @@ static int xc_dom_load_bin_kernel(struct xc_dom_image *dom) return -EINVAL; } + if ( image_size < skip || + image_size - skip < text_size ) + { + DOMPRINTF("%s: image is too small for declared text size", + __FUNCTION__); + return -EINVAL; + } + memcpy(dest, image + skip, text_size); memset(dest + text_size, 0, bss_size); -- 1.7.2.5
Ian Jackson
2013-Jun-13 18:14 UTC
[PATCH 19/23] libxc: check failure of xc_dom_*_to_ptr, xc_map_foreign_range
The return values from xc_dom_*_to_ptr and xc_map_foreign_range are sometimes dereferenced, or subjected to pointer arithmetic, without checking whether the relevant function failed and returned NULL. Add an appropriate error check at every call site. This is part of the fix to a security issue, XSA-55. Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> v8: Add a missing check in xc_offline_page.c:xc_exchange_page, which was in the next patch in v7 of the series. Also improve the message. I think in this particular error case it may be that the results are a broken guest, but turning this from a possible host tools crash into a guest problem seems to solve the potential security problem. v7: Simplify an error DOMPRINTF to not use "load ? : ". Make DOMPRINTF allocation error messages consistent. Do not set elf->dest_pages in xc_dom_load_elf_kernel if xc_dom_seg_to_ptr_pages fails. v5: This patch is new in this version of the series. --- tools/libxc/xc_dom_armzimageloader.c | 6 ++++ tools/libxc/xc_dom_binloader.c | 6 ++++ tools/libxc/xc_dom_core.c | 6 ++++ tools/libxc/xc_dom_elfloader.c | 13 ++++++++++ tools/libxc/xc_dom_x86.c | 45 ++++++++++++++++++++++++++++++++++ tools/libxc/xc_domain_restore.c | 27 ++++++++++++++++++++ tools/libxc/xc_offline_page.c | 5 ++++ 7 files changed, 108 insertions(+), 0 deletions(-) diff --git a/tools/libxc/xc_dom_armzimageloader.c b/tools/libxc/xc_dom_armzimageloader.c index 74027db..4cbbbab 100644 --- a/tools/libxc/xc_dom_armzimageloader.c +++ b/tools/libxc/xc_dom_armzimageloader.c @@ -140,6 +140,12 @@ static int xc_dom_load_zimage_kernel(struct xc_dom_image *dom) DOMPRINTF_CALLED(dom->xch); dst = xc_dom_seg_to_ptr(dom, &dom->kernel_seg); + if ( dst == NULL ) + { + DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, &dom->kernel_seg) => NULL", + __func__); + return -1; + } DOMPRINTF("%s: kernel sed %#"PRIx64"-%#"PRIx64, __func__, dom->kernel_seg.vstart, dom->kernel_seg.vend); diff --git a/tools/libxc/xc_dom_binloader.c b/tools/libxc/xc_dom_binloader.c index 7eaf071..891bcea 100644 --- a/tools/libxc/xc_dom_binloader.c +++ b/tools/libxc/xc_dom_binloader.c @@ -277,6 +277,12 @@ static int xc_dom_load_bin_kernel(struct xc_dom_image *dom) DOMPRINTF(" bss_size: 0x%" PRIx32 "", bss_size); dest = xc_dom_vaddr_to_ptr(dom, dom->kernel_seg.vstart, &dest_size); + if ( dest == NULL ) + { + DOMPRINTF("%s: xc_dom_vaddr_to_ptr(dom, dom->kernel_seg.vstart)" + " => NULL", __FUNCTION__); + return -EINVAL; + } if ( dest_size < text_size || dest_size - text_size < bss_size ) diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c index cf96bfa..21a8e0d 100644 --- a/tools/libxc/xc_dom_core.c +++ b/tools/libxc/xc_dom_core.c @@ -870,6 +870,12 @@ int xc_dom_build_image(struct xc_dom_image *dom) ramdisklen) != 0 ) goto err; ramdiskmap = xc_dom_seg_to_ptr(dom, &dom->ramdisk_seg); + if ( ramdiskmap == NULL ) + { + DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, &dom->ramdisk_seg) => NULL", + __FUNCTION__); + goto err; + } if ( unziplen ) { if ( xc_dom_do_gunzip(dom->xch, diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c index f2bc2f5..8d0a09f 100644 --- a/tools/libxc/xc_dom_elfloader.c +++ b/tools/libxc/xc_dom_elfloader.c @@ -137,6 +137,12 @@ static elf_errorstatus xc_dom_load_elf_symtab(struct xc_dom_image *dom, return 0; size = dom->kernel_seg.vend - dom->bsd_symtab_start; hdr_ptr = xc_dom_vaddr_to_ptr(dom, dom->bsd_symtab_start, &allow_size); + if ( hdr_ptr == NULL ) + { + DOMPRINTF("%s/load: xc_dom_vaddr_to_ptr(dom,dom->bsd_symtab_start" + " => NULL", __FUNCTION__); + return -1; + } elf->caller_xdest_base = hdr_ptr; elf->caller_xdest_size = allow_size; hdr = ELF_REALPTR2PTRVAL(hdr_ptr); @@ -382,7 +388,14 @@ static elf_errorstatus xc_dom_load_elf_kernel(struct xc_dom_image *dom) xen_pfn_t pages; elf->dest_base = xc_dom_seg_to_ptr_pages(dom, &dom->kernel_seg, &pages); + if ( elf->dest_base == NULL ) + { + DOMPRINTF("%s: xc_dom_vaddr_to_ptr(dom,dom->kernel_seg)" + " => NULL", __FUNCTION__); + return -1; + } elf->dest_size = pages * XC_DOM_PAGE_SIZE(dom); + rc = elf_load_binary(elf); if ( rc < 0 ) { diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c index f1be43b..8b6191d 100644 --- a/tools/libxc/xc_dom_x86.c +++ b/tools/libxc/xc_dom_x86.c @@ -223,6 +223,12 @@ static xen_pfn_t move_l3_below_4G(struct xc_dom_image *dom, goto out; l3tab = xc_dom_pfn_to_ptr(dom, l3pfn, 1); + if ( l3tab == NULL ) + { + DOMPRINTF("%s: xc_dom_pfn_to_ptr(dom, l3pfn, 1) => NULL", + __FUNCTION__); + return l3mfn; /* our one call site will call xc_dom_panic and fail */ + } memset(l3tab, 0, XC_DOM_PAGE_SIZE(dom)); DOMPRINTF("%s: successfully relocated L3 below 4G. " @@ -266,6 +272,8 @@ static int setup_pgtables_x86_32_pae(struct xc_dom_image *dom) } l3tab = xc_dom_pfn_to_ptr(dom, l3pfn, 1); + if ( l3tab == NULL ) + goto pfn_error; for ( addr = dom->parms.virt_base; addr < dom->virt_pgtab_end; addr += PAGE_SIZE_X86 ) @@ -274,6 +282,8 @@ static int setup_pgtables_x86_32_pae(struct xc_dom_image *dom) { /* get L2 tab, make L3 entry */ l2tab = xc_dom_pfn_to_ptr(dom, l2pfn, 1); + if ( l2tab == NULL ) + goto pfn_error; l3off = l3_table_offset_pae(addr); l3tab[l3off] pfn_to_paddr(xc_dom_p2m_guest(dom, l2pfn)) | L3_PROT; @@ -284,6 +294,8 @@ static int setup_pgtables_x86_32_pae(struct xc_dom_image *dom) { /* get L1 tab, make L2 entry */ l1tab = xc_dom_pfn_to_ptr(dom, l1pfn, 1); + if ( l1tab == NULL ) + goto pfn_error; l2off = l2_table_offset_pae(addr); l2tab[l2off] pfn_to_paddr(xc_dom_p2m_guest(dom, l1pfn)) | L2_PROT; @@ -310,6 +322,11 @@ static int setup_pgtables_x86_32_pae(struct xc_dom_image *dom) l3tab[3] = pfn_to_paddr(xc_dom_p2m_guest(dom, l2pfn)) | L3_PROT; } return 0; + +pfn_error: + xc_dom_panic(dom->xch, XC_INTERNAL_ERROR, + "%s: xc_dom_pfn_to_ptr failed", __FUNCTION__); + return -EINVAL; } #undef L1_PROT @@ -347,6 +364,9 @@ static int setup_pgtables_x86_64(struct xc_dom_image *dom) uint64_t addr; xen_pfn_t pgpfn; + if ( l4tab == NULL ) + goto pfn_error; + for ( addr = dom->parms.virt_base; addr < dom->virt_pgtab_end; addr += PAGE_SIZE_X86 ) { @@ -354,6 +374,8 @@ static int setup_pgtables_x86_64(struct xc_dom_image *dom) { /* get L3 tab, make L4 entry */ l3tab = xc_dom_pfn_to_ptr(dom, l3pfn, 1); + if ( l3tab == NULL ) + goto pfn_error; l4off = l4_table_offset_x86_64(addr); l4tab[l4off] pfn_to_paddr(xc_dom_p2m_guest(dom, l3pfn)) | L4_PROT; @@ -364,6 +386,8 @@ static int setup_pgtables_x86_64(struct xc_dom_image *dom) { /* get L2 tab, make L3 entry */ l2tab = xc_dom_pfn_to_ptr(dom, l2pfn, 1); + if ( l2tab == NULL ) + goto pfn_error; l3off = l3_table_offset_x86_64(addr); l3tab[l3off] pfn_to_paddr(xc_dom_p2m_guest(dom, l2pfn)) | L3_PROT; @@ -376,6 +400,8 @@ static int setup_pgtables_x86_64(struct xc_dom_image *dom) { /* get L1 tab, make L2 entry */ l1tab = xc_dom_pfn_to_ptr(dom, l1pfn, 1); + if ( l1tab == NULL ) + goto pfn_error; l2off = l2_table_offset_x86_64(addr); l2tab[l2off] pfn_to_paddr(xc_dom_p2m_guest(dom, l1pfn)) | L2_PROT; @@ -396,6 +422,11 @@ static int setup_pgtables_x86_64(struct xc_dom_image *dom) l1tab = NULL; } return 0; + +pfn_error: + xc_dom_panic(dom->xch, XC_INTERNAL_ERROR, + "%s: xc_dom_pfn_to_ptr failed", __FUNCTION__); + return -EINVAL; } #undef L1_PROT @@ -413,6 +444,8 @@ static int alloc_magic_pages(struct xc_dom_image *dom) if ( xc_dom_alloc_segment(dom, &dom->p2m_seg, "phys2mach", 0, p2m_size) ) return -1; dom->p2m_guest = xc_dom_seg_to_ptr(dom, &dom->p2m_seg); + if ( dom->p2m_guest == NULL ) + return -1; /* allocate special pages */ dom->start_info_pfn = xc_dom_alloc_page(dom, "start info"); @@ -437,6 +470,12 @@ static int start_info_x86_32(struct xc_dom_image *dom) DOMPRINTF_CALLED(dom->xch); + if ( start_info == NULL ) + { + DOMPRINTF("%s: xc_dom_pfn_to_ptr failed on start_info", __FUNCTION__); + return -1; /* our caller throws away our return value :-/ */ + } + memset(start_info, 0, sizeof(*start_info)); strncpy(start_info->magic, dom->guest_type, sizeof(start_info->magic)); start_info->magic[sizeof(start_info->magic) - 1] = ''\0''; @@ -477,6 +516,12 @@ static int start_info_x86_64(struct xc_dom_image *dom) DOMPRINTF_CALLED(dom->xch); + if ( start_info == NULL ) + { + DOMPRINTF("%s: xc_dom_pfn_to_ptr failed on start_info", __FUNCTION__); + return -1; /* our caller throws away our return value :-/ */ + } + memset(start_info, 0, sizeof(*start_info)); strncpy(start_info->magic, dom->guest_type, sizeof(start_info->magic)); start_info->magic[sizeof(start_info->magic) - 1] = ''\0''; diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c index a15f86a..c7835ff 100644 --- a/tools/libxc/xc_domain_restore.c +++ b/tools/libxc/xc_domain_restore.c @@ -1638,6 +1638,12 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom, mfn = ctx->p2m[pfn]; buf = xc_map_foreign_range(xch, dom, PAGE_SIZE, PROT_READ | PROT_WRITE, mfn); + if ( buf == NULL ) + { + ERROR("xc_map_foreign_range for generation id" + " buffer failed"); + goto out; + } generationid = *(unsigned long long *)(buf + offset); *(unsigned long long *)(buf + offset) = generationid + 1; @@ -1794,6 +1800,11 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom, l3tab = (uint64_t *) xc_map_foreign_range(xch, dom, PAGE_SIZE, PROT_READ, ctx->p2m[i]); + if ( l3tab == NULL ) + { + PERROR("xc_map_foreign_range failed (for l3tab)"); + goto out; + } for ( j = 0; j < 4; j++ ) l3ptes[j] = l3tab[j]; @@ -1820,6 +1831,11 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom, l3tab = (uint64_t *) xc_map_foreign_range(xch, dom, PAGE_SIZE, PROT_READ | PROT_WRITE, ctx->p2m[i]); + if ( l3tab == NULL ) + { + PERROR("xc_map_foreign_range failed (for l3tab, 2nd)"); + goto out; + } for ( j = 0; j < 4; j++ ) l3tab[j] = l3ptes[j]; @@ -1996,6 +2012,12 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom, SET_FIELD(ctxt, user_regs.edx, mfn); start_info = xc_map_foreign_range( xch, dom, PAGE_SIZE, PROT_READ | PROT_WRITE, mfn); + if ( start_info == NULL ) + { + PERROR("xc_map_foreign_range failed (for start_info)"); + goto out; + } + SET_FIELD(start_info, nr_pages, dinfo->p2m_size); SET_FIELD(start_info, shared_info, shared_info_frame<<PAGE_SHIFT); SET_FIELD(start_info, flags, 0); @@ -2143,6 +2165,11 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom, /* Restore contents of shared-info page. No checking needed. */ new_shared_info = xc_map_foreign_range( xch, dom, PAGE_SIZE, PROT_WRITE, shared_info_frame); + if ( new_shared_info == NULL ) + { + PERROR("xc_map_foreign_range failed (for new_shared_info)"); + goto out; + } /* restore saved vcpu_info and arch specific info */ MEMCPY_FIELD(new_shared_info, old_shared_info, vcpu_info); diff --git a/tools/libxc/xc_offline_page.c b/tools/libxc/xc_offline_page.c index 089a361..36b9812 100644 --- a/tools/libxc/xc_offline_page.c +++ b/tools/libxc/xc_offline_page.c @@ -714,6 +714,11 @@ int xc_exchange_page(xc_interface *xch, int domid, xen_pfn_t mfn) new_p = xc_map_foreign_range(xch, domid, PAGE_SIZE, PROT_READ|PROT_WRITE, new_mfn); + if ( new_p == NULL ) + { + ERROR("failed to map new_p for copy, guest may be broken?"); + goto failed; + } memcpy(new_p, backup, PAGE_SIZE); munmap(new_p, PAGE_SIZE); mops.arg1.mfn = new_mfn; -- 1.7.2.5
A sufficiently malformed input to libxc (such as a malformed input ELF or other guest-controlled data) might cause one of libxc''s malloc() to fail. In this case we need to make sure we don''t dereference or do pointer arithmetic on the result. Search for all occurrences of \b(m|c|re)alloc in libxc, and all functions which call them, and add appropriate error checking where missing. This includes the functions xc_dom_malloc*, which now print a message when they fail so that callers don''t have to do so. The function xc_cpuid_to_str wasn''t provided with a sane return value and has a pretty strange API, which now becomes a little stranger. There are no in-tree callers. This is part of the fix to a security issue, XSA-55. Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> v8: Move a check in xc_exchange_page to the previous patch (ie, remove it from this patch). v7: Add a missing check for a call to alloc_str. Add arithmetic overflow check in xc_dom_malloc. Coding style fix. v6: Fix a missed call `pfn_err = calloc...'' in xc_domain_restore.c. Fix a missed call `new_pfn = xc_map_foreign_range...'' in xc_offline_page.c v5: This patch is new in this version of the series. --- tools/libxc/xc_cpuid_x86.c | 20 ++++++++++++++++++-- tools/libxc/xc_dom_arm.c | 2 ++ tools/libxc/xc_dom_core.c | 13 +++++++++++++ tools/libxc/xc_dom_elfloader.c | 2 ++ tools/libxc/xc_dom_x86.c | 3 +++ tools/libxc/xc_domain_restore.c | 13 +++++++++++++ tools/libxc/xc_linux_osdep.c | 4 ++++ tools/libxc/xc_private.c | 2 ++ tools/libxc/xenctrl.h | 2 +- 9 files changed, 58 insertions(+), 3 deletions(-) diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c index 17efc0f..fa47787 100644 --- a/tools/libxc/xc_cpuid_x86.c +++ b/tools/libxc/xc_cpuid_x86.c @@ -590,6 +590,8 @@ static int xc_cpuid_do_domctl( static char *alloc_str(void) { char *s = malloc(33); + if ( s == NULL ) + return s; memset(s, 0, 33); return s; } @@ -601,6 +603,8 @@ void xc_cpuid_to_str(const unsigned int *regs, char **strs) for ( i = 0; i < 4; i++ ) { strs[i] = alloc_str(); + if ( strs[i] == NULL ) + continue; for ( j = 0; j < 32; j++ ) strs[i][j] = !!((regs[i] & (1U << (31 - j)))) ? ''1'' : ''0''; } @@ -681,7 +685,7 @@ int xc_cpuid_check( const char **config, char **config_transformed) { - int i, j; + int i, j, rc; unsigned int regs[4]; memset(config_transformed, 0, 4 * sizeof(*config_transformed)); @@ -693,6 +697,11 @@ int xc_cpuid_check( if ( config[i] == NULL ) continue; config_transformed[i] = alloc_str(); + if ( config_transformed[i] == NULL ) + { + rc = -ENOMEM; + goto fail_rc; + } for ( j = 0; j < 32; j++ ) { unsigned char val = !!((regs[i] & (1U << (31 - j)))); @@ -709,12 +718,14 @@ int xc_cpuid_check( return 0; fail: + rc = -EPERM; + fail_rc: for ( i = 0; i < 4; i++ ) { free(config_transformed[i]); config_transformed[i] = NULL; } - return -EPERM; + return rc; } /* @@ -759,6 +770,11 @@ int xc_cpuid_set( } config_transformed[i] = alloc_str(); + if ( config_transformed[i] == NULL ) + { + rc = -ENOMEM; + goto fail; + } for ( j = 0; j < 32; j++ ) { diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c index aaf35ca..df59ffb 100644 --- a/tools/libxc/xc_dom_arm.c +++ b/tools/libxc/xc_dom_arm.c @@ -170,6 +170,8 @@ int arch_setup_meminit(struct xc_dom_image *dom) dom->shadow_enabled = 1; dom->p2m_host = xc_dom_malloc(dom, sizeof(xen_pfn_t) * dom->total_pages); + if ( dom->p2m_host == NULL ) + return -EINVAL; /* setup initial p2m */ for ( pfn = 0; pfn < dom->total_pages; pfn++ ) diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c index 21a8e0d..1a14d3c 100644 --- a/tools/libxc/xc_dom_core.c +++ b/tools/libxc/xc_dom_core.c @@ -120,9 +120,17 @@ void *xc_dom_malloc(struct xc_dom_image *dom, size_t size) { struct xc_dom_mem *block; + if ( size > SIZE_MAX - sizeof(*block) ) + { + DOMPRINTF("%s: unreasonable allocation size", __FUNCTION__); + return NULL; + } block = malloc(sizeof(*block) + size); if ( block == NULL ) + { + DOMPRINTF("%s: allocation failed", __FUNCTION__); return NULL; + } memset(block, 0, sizeof(*block) + size); block->next = dom->memblocks; dom->memblocks = block; @@ -138,7 +146,10 @@ void *xc_dom_malloc_page_aligned(struct xc_dom_image *dom, size_t size) block = malloc(sizeof(*block)); if ( block == NULL ) + { + DOMPRINTF("%s: allocation failed", __FUNCTION__); return NULL; + } memset(block, 0, sizeof(*block)); block->mmap_len = size; block->mmap_ptr = mmap(NULL, block->mmap_len, @@ -146,6 +157,7 @@ void *xc_dom_malloc_page_aligned(struct xc_dom_image *dom, size_t size) -1, 0); if ( block->mmap_ptr == MAP_FAILED ) { + DOMPRINTF("%s: mmap failed", __FUNCTION__); free(block); return NULL; } @@ -202,6 +214,7 @@ void *xc_dom_malloc_filemap(struct xc_dom_image *dom, close(fd); if ( block != NULL ) free(block); + DOMPRINTF("%s: failed (on file `%s'')", __FUNCTION__, filename); return NULL; } diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c index 8d0a09f..9843b1f 100644 --- a/tools/libxc/xc_dom_elfloader.c +++ b/tools/libxc/xc_dom_elfloader.c @@ -327,6 +327,8 @@ static elf_errorstatus xc_dom_parse_elf_kernel(struct xc_dom_image *dom) return rc; elf = xc_dom_malloc(dom, sizeof(*elf)); + if ( elf == NULL ) + return -1; dom->private_loader = elf; rc = elf_init(elf, dom->kernel_blob, dom->kernel_size); xc_elf_set_logfile(dom->xch, elf, 1); diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c index 8b6191d..126c0f8 100644 --- a/tools/libxc/xc_dom_x86.c +++ b/tools/libxc/xc_dom_x86.c @@ -760,6 +760,9 @@ int arch_setup_meminit(struct xc_dom_image *dom) } dom->p2m_host = xc_dom_malloc(dom, sizeof(xen_pfn_t) * dom->total_pages); + if ( dom->p2m_host == NULL ) + return -EINVAL; + if ( dom->superpages ) { int count = dom->total_pages >> SUPERPAGE_PFN_SHIFT; diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c index c7835ff..f53ff88 100644 --- a/tools/libxc/xc_domain_restore.c +++ b/tools/libxc/xc_domain_restore.c @@ -1243,6 +1243,11 @@ static int apply_batch(xc_interface *xch, uint32_t dom, struct restore_ctx *ctx, /* Map relevant mfns */ pfn_err = calloc(j, sizeof(*pfn_err)); + if ( pfn_err == NULL ) + { + PERROR("allocation for pfn_err failed"); + return -1; + } region_base = xc_map_foreign_bulk( xch, dom, PROT_WRITE, region_mfn, pfn_err, j); @@ -1532,8 +1537,16 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom, region_mfn = malloc(ROUNDUP(MAX_BATCH_SIZE * sizeof(xen_pfn_t), PAGE_SHIFT)); ctx->p2m_batch = malloc(ROUNDUP(MAX_BATCH_SIZE * sizeof(xen_pfn_t), PAGE_SHIFT)); if (!ctx->hvm && ctx->superpages) + { ctx->p2m_saved_batch malloc(ROUNDUP(MAX_BATCH_SIZE * sizeof(xen_pfn_t), PAGE_SHIFT)); + if ( ctx->p2m_saved_batch == NULL ) + { + ERROR("saved batch memory alloc failed"); + errno = ENOMEM; + goto out; + } + } if ( (ctx->p2m == NULL) || (pfn_type == NULL) || (region_mfn == NULL) || (ctx->p2m_batch == NULL) ) diff --git a/tools/libxc/xc_linux_osdep.c b/tools/libxc/xc_linux_osdep.c index 36832b6..73860a2 100644 --- a/tools/libxc/xc_linux_osdep.c +++ b/tools/libxc/xc_linux_osdep.c @@ -378,6 +378,8 @@ static void *linux_privcmd_map_foreign_range(xc_interface *xch, xc_osdep_handle num = (size + XC_PAGE_SIZE - 1) >> XC_PAGE_SHIFT; arr = calloc(num, sizeof(xen_pfn_t)); + if ( arr == NULL ) + return NULL; for ( i = 0; i < num; i++ ) arr[i] = mfn + i; @@ -402,6 +404,8 @@ static void *linux_privcmd_map_foreign_ranges(xc_interface *xch, xc_osdep_handle num_per_entry = chunksize >> XC_PAGE_SHIFT; num = num_per_entry * nentries; arr = calloc(num, sizeof(xen_pfn_t)); + if ( arr == NULL ) + return NULL; for ( i = 0; i < nentries; i++ ) for ( j = 0; j < num_per_entry; j++ ) diff --git a/tools/libxc/xc_private.c b/tools/libxc/xc_private.c index e891cc8..acaf9e0 100644 --- a/tools/libxc/xc_private.c +++ b/tools/libxc/xc_private.c @@ -771,6 +771,8 @@ const char *xc_strerror(xc_interface *xch, int errcode) errbuf = pthread_getspecific(errbuf_pkey); if (errbuf == NULL) { errbuf = malloc(XS_BUFSIZE); + if ( errbuf == NULL ) + return "(failed to allocate errbuf)"; pthread_setspecific(errbuf_pkey, errbuf); } diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h index 40ee8fc..5697765 100644 --- a/tools/libxc/xenctrl.h +++ b/tools/libxc/xenctrl.h @@ -1827,7 +1827,7 @@ int xc_cpuid_set(xc_interface *xch, int xc_cpuid_apply_policy(xc_interface *xch, domid_t domid); void xc_cpuid_to_str(const unsigned int *regs, - char **strs); + char **strs); /* some strs[] may be NULL if ENOMEM */ int xc_mca_op(xc_interface *xch, struct xen_mc *mc); #endif -- 1.7.2.5
Ian Jackson
2013-Jun-13 18:15 UTC
[PATCH 21/23] libxc: range checks in xc_dom_p2m_host and _guest
These functions take guest pfns and look them up in the p2m. They did no range checking. However, some callers, notably xc_dom_boot.c:setup_hypercall_page want to pass untrusted guest-supplied value(s). It is most convenient to detect this here and return INVALID_MFN. This is part of the fix to a security issue, XSA-55. Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Tim Deegan <tim@xen.org> Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Chuck Anderson <chuck.anderson@oracle.com> v6: Check for underflow too (thanks to Andrew Cooper). --- tools/libxc/xc_dom.h | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/tools/libxc/xc_dom.h b/tools/libxc/xc_dom.h index 5968e7b..86e23ee 100644 --- a/tools/libxc/xc_dom.h +++ b/tools/libxc/xc_dom.h @@ -342,6 +342,8 @@ static inline xen_pfn_t xc_dom_p2m_host(struct xc_dom_image *dom, xen_pfn_t pfn) { if (dom->shadow_enabled) return pfn; + if (pfn < dom->rambase_pfn || pfn >= dom->rambase_pfn + dom->total_pages) + return INVALID_MFN; return dom->p2m_host[pfn - dom->rambase_pfn]; } @@ -350,6 +352,8 @@ static inline xen_pfn_t xc_dom_p2m_guest(struct xc_dom_image *dom, { if (xc_dom_feature_translated(dom)) return pfn; + if (pfn < dom->rambase_pfn || pfn >= dom->rambase_pfn + dom->total_pages) + return INVALID_MFN; return dom->p2m_host[pfn - dom->rambase_pfn]; } -- 1.7.2.5
Ian Jackson
2013-Jun-13 18:15 UTC
[PATCH 22/23] libxc: check blob size before proceeding in xc_dom_check_gzip
From: Matthew Daley <mattjd@gmail.com> This is part of the fix to a security issue, XSA-55. Signed-off-by: Matthew Daley <mattjd@gmail.com> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> v8: Add a comment explaining where the number 6 comes from. v6: This patch is new in v6 of the series. --- tools/libxc/xc_dom_core.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c index 1a14d3c..5f188c1 100644 --- a/tools/libxc/xc_dom_core.c +++ b/tools/libxc/xc_dom_core.c @@ -284,6 +284,11 @@ size_t xc_dom_check_gzip(xc_interface *xch, void *blob, size_t ziplen) unsigned char *gzlen; size_t unziplen; + if ( ziplen < 6 ) + /* Too small. We need (i.e. the subsequent code relies on) + * 2 bytes for the magic number plus 4 bytes length. */ + return 0; + if ( strncmp(blob, "\037\213", 2) ) /* not gzipped */ return 0; -- 1.7.2.5
Ian Jackson
2013-Jun-13 18:15 UTC
[PATCH 23/23] libxc: Better range check in xc_dom_alloc_segment
If seg->pfn is too large, the arithmetic in the range check might overflow, defeating the range check. This is part of the fix to a security issue, XSA-55. Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> --- tools/libxc/xc_dom_core.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c index 5f188c1..3df7171 100644 --- a/tools/libxc/xc_dom_core.c +++ b/tools/libxc/xc_dom_core.c @@ -511,7 +511,8 @@ int xc_dom_alloc_segment(struct xc_dom_image *dom, seg->vstart = start; seg->pfn = (seg->vstart - dom->parms.virt_base) / page_size; - if ( pages > dom->total_pages || /* double test avoids overflow probs */ + if ( pages > dom->total_pages || /* multiple test avoids overflow probs */ + seg->pfn > dom->total_pages || pages > dom->total_pages - seg->pfn) { xc_dom_panic(dom->xch, XC_OUT_OF_MEMORY, -- 1.7.2.5
Ian Jackson writes ("[PATCH v8 00/23] XSA55 libelf fixes for unstable"):> This is version 8 of my series to try to fix libelf and the domain > loader. I hope this will be the final version. > > Barring further showstoppers, and in the absence of objections, I > intend to push this to xen.git#staging tomorrow, and include it in a > revised version of the XSA-55 advisory.I should mention that I have rerun the automatic assembler diff verifications for all three versions of v8 of this series (strictly, for a slightly-prerelease version of v8). I have done this verification for both patches 08/23 ("introduce macros") and 17/23 ("abolish obsolete macros") and where applicable for both amd64 and i386 (but not ARM). Ian.
George Dunlap
2013-Jun-14 14:53 UTC
Re: [PATCH 18/23] libxc: Add range checking to xc_dom_binloader
On Thu, Jun 13, 2013 at 7:14 PM, Ian Jackson <ian.jackson@eu.citrix.com> wrote:> This is a simple binary image loader with its own metadata format. > However, it is too careless with image-supplied values. > > Add the following checks: > > * That the image is bigger than the metadata table; otherwise the > pointer arithmetic to calculate the metadata table location may > yield undefined and dangerous values. > > * When clamping the end of the region to search, that we do not > calculate pointers beyond the end of the image. The C > specification does not permit this and compilers are becoming ever > more determined to miscompile code when they can "prove" various > falsehoods based on assertions from the C spec. > > * That the supplied image is big enough for the text we are allegedly > copying from it. Otherwise we might have a read overrun and copy > the results (perhaps a lot of secret data) into the guest. > > This is part of the fix to a security issue, XSA-55. > > Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > > v6: Add a missing `return -EINVAL'' (Matthew Daley). > Fix an error in the commit message (Matthew Daley). > > v5: This patch is new in this version of the series. > --- > tools/libxc/xc_dom_binloader.c | 13 ++++++++++++- > 1 files changed, 12 insertions(+), 1 deletions(-) > > diff --git a/tools/libxc/xc_dom_binloader.c b/tools/libxc/xc_dom_binloader.c > index d2de04c..7eaf071 100644 > --- a/tools/libxc/xc_dom_binloader.c > +++ b/tools/libxc/xc_dom_binloader.c > @@ -123,9 +123,12 @@ static struct xen_bin_image_table *find_table(struct xc_dom_image *dom) > uint32_t *probe_ptr; > uint32_t *probe_end; > > + if ( dom->kernel_size < sizeof(*table) ) > + return NULL; > probe_ptr = dom->kernel_blob; > probe_end = dom->kernel_blob + dom->kernel_size - sizeof(*table); > - if ( (void*)probe_end > (dom->kernel_blob + 8192) ) > + if ( dom->kernel_size >= 8192 && > + (void*)probe_end > (dom->kernel_blob + 8192) ) > probe_end = dom->kernel_blob + 8192;Wait, what''s going on here? Isn''t the point of this check originally that "probe_end" might be pointing off into nowhere, and you''re going to "clip" it into pointing somewhere reasonable? It doesn''t look like you''ve actually changed any pointer arithmetic -- if either probe_end or dom->kernel_blob + 8192 are wild, then they''ll still be wild after this check, won''t they? The other changes look good. -George
Ian Jackson
2013-Jun-14 15:03 UTC
Re: [PATCH 18/23] libxc: Add range checking to xc_dom_binloader
George Dunlap writes ("Re: [Xen-devel] [PATCH 18/23] libxc: Add range checking to xc_dom_binloader"):> On Thu, Jun 13, 2013 at 7:14 PM, Ian Jackson <ian.jackson@eu.citrix.com> wrote: > > * When clamping the end of the region to search, that we do not > > calculate pointers beyond the end of the image. The C > > specification does not permit this and compilers are becoming ever > > more determined to miscompile code when they can "prove" various > > falsehoods based on assertions from the C spec....> > + if ( dom->kernel_size < sizeof(*table) ) > > + return NULL; > > probe_ptr = dom->kernel_blob; > > probe_end = dom->kernel_blob + dom->kernel_size - sizeof(*table); > > - if ( (void*)probe_end > (dom->kernel_blob + 8192) ) > > + if ( dom->kernel_size >= 8192 && > > + (void*)probe_end > (dom->kernel_blob + 8192) ) > > probe_end = dom->kernel_blob + 8192; > > Wait, what''s going on here? Isn''t the point of this check originally > that "probe_end" might be pointing off into nowhere, and you''re going > to "clip" it into pointing somewhere reasonable?No. The (undocumented) format being parsed here is that the info table should start no later than 8k into the file. probe_end is the first address to no longer look for the table at. Firstly we set probe_end to the end of the image minus the table length - which would imply searching the whole image. The (original and unchanged) purpose of the if statement and assignment is to limit this to 8192 bytes from the start of the image. However there is a bug: if the image is less than 8k long, this involves computing a wild pointer (which is forbidden). So in my patch I add an additional test.> It doesn''t look like you''ve actually changed any pointer arithmetic -- > if either probe_end or dom->kernel_blob + 8192 are wild, then they''ll > still be wild after this check, won''t they?The initial value of probe_end is guaranteed not to be wild. Before my change dom->kernel_blob + 8192 might be computed even if it is wild; after my change it is only computed if it is guaranteed not to be. Ian.
George Dunlap
2013-Jun-14 15:09 UTC
Re: [PATCH 18/23] libxc: Add range checking to xc_dom_binloader
On 14/06/13 16:03, Ian Jackson wrote:> George Dunlap writes ("Re: [Xen-devel] [PATCH 18/23] libxc: Add range checking to xc_dom_binloader"): >> On Thu, Jun 13, 2013 at 7:14 PM, Ian Jackson <ian.jackson@eu.citrix.com> wrote: >>> * When clamping the end of the region to search, that we do not >>> calculate pointers beyond the end of the image. The C >>> specification does not permit this and compilers are becoming ever >>> more determined to miscompile code when they can "prove" various >>> falsehoods based on assertions from the C spec. > ... >>> + if ( dom->kernel_size < sizeof(*table) ) >>> + return NULL; >>> probe_ptr = dom->kernel_blob; >>> probe_end = dom->kernel_blob + dom->kernel_size - sizeof(*table); >>> - if ( (void*)probe_end > (dom->kernel_blob + 8192) ) >>> + if ( dom->kernel_size >= 8192 && >>> + (void*)probe_end > (dom->kernel_blob + 8192) ) >>> probe_end = dom->kernel_blob + 8192; >> Wait, what''s going on here? Isn''t the point of this check originally >> that "probe_end" might be pointing off into nowhere, and you''re going >> to "clip" it into pointing somewhere reasonable? > No. The (undocumented) format being parsed here is that the info > table should start no later than 8k into the file. probe_end is the > first address to no longer look for the table at. > > Firstly we set probe_end to the end of the image minus the table > length - which would imply searching the whole image. The (original > and unchanged) purpose of the if statement and assignment is to limit > this to 8192 bytes from the start of the image. > > However there is a bug: if the image is less than 8k long, this > involves computing a wild pointer (which is forbidden). So in my > patch I add an additional test. > >> It doesn''t look like you''ve actually changed any pointer arithmetic -- >> if either probe_end or dom->kernel_blob + 8192 are wild, then they''ll >> still be wild after this check, won''t they? > The initial value of probe_end is guaranteed not to be wild. > > Before my change dom->kernel_blob + 8192 might be computed even if > it is wild; after my change it is only computed if it is guaranteed > not to be.Oh, I see -- right; so if "dom->kernel_size < 8192", then "dom->kernel_blob+8192" might be wild; but as long as "dom->kernel_size > 8192", then "dom->kernel_blob + 8192" will be fine. And if dom->kernel_size < 8192, then probe_end will already be < dom->kernel_blob+8192, so we don''t need to clip things. Might be nice to put a comment here, so people coming later can make sense out of this. Even if people read the changeset description, they may, like me, not be able to suss out which calculation is in danger of being wild. Other than that: Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>
George Dunlap
2013-Jun-14 15:12 UTC
Re: [PATCH 18/23] libxc: Add range checking to xc_dom_binloader
On Fri, Jun 14, 2013 at 4:09 PM, George Dunlap <george.dunlap@eu.citrix.com> wrote:> On 14/06/13 16:03, Ian Jackson wrote: >> >> George Dunlap writes ("Re: [Xen-devel] [PATCH 18/23] libxc: Add range >> checking to xc_dom_binloader"): >>> >>> On Thu, Jun 13, 2013 at 7:14 PM, Ian Jackson <ian.jackson@eu.citrix.com> >>> wrote: >>>> >>>> * When clamping the end of the region to search, that we do not >>>> calculate pointers beyond the end of the image. The C >>>> specification does not permit this and compilers are becoming ever >>>> more determined to miscompile code when they can "prove" various >>>> falsehoods based on assertions from the C spec. >> >> ... >>>> >>>> + if ( dom->kernel_size < sizeof(*table) ) >>>> + return NULL; >>>> probe_ptr = dom->kernel_blob; >>>> probe_end = dom->kernel_blob + dom->kernel_size - sizeof(*table); >>>> - if ( (void*)probe_end > (dom->kernel_blob + 8192) ) >>>> + if ( dom->kernel_size >= 8192 && >>>> + (void*)probe_end > (dom->kernel_blob + 8192) ) >>>> probe_end = dom->kernel_blob + 8192; >>> >>> Wait, what''s going on here? Isn''t the point of this check originally >>> that "probe_end" might be pointing off into nowhere, and you''re going >>> to "clip" it into pointing somewhere reasonable? >> >> No. The (undocumented) format being parsed here is that the info >> table should start no later than 8k into the file. probe_end is the >> first address to no longer look for the table at. >> >> Firstly we set probe_end to the end of the image minus the table >> length - which would imply searching the whole image. The (original >> and unchanged) purpose of the if statement and assignment is to limit >> this to 8192 bytes from the start of the image. >> >> However there is a bug: if the image is less than 8k long, this >> involves computing a wild pointer (which is forbidden). So in my >> patch I add an additional test. >> >>> It doesn''t look like you''ve actually changed any pointer arithmetic -- >>> if either probe_end or dom->kernel_blob + 8192 are wild, then they''ll >>> still be wild after this check, won''t they? >> >> The initial value of probe_end is guaranteed not to be wild. >> >> Before my change dom->kernel_blob + 8192 might be computed even if >> it is wild; after my change it is only computed if it is guaranteed >> not to be. > > > Oh, I see -- right; so if "dom->kernel_size < 8192", then > "dom->kernel_blob+8192" might be wild; but as long as "dom->kernel_size > > 8192", then "dom->kernel_blob + 8192" will be fine. > > And if dom->kernel_size < 8192, then probe_end will already be < > dom->kernel_blob+8192, so we don''t need to clip things. > > Might be nice to put a comment here, so people coming later can make sense > out of this. Even if people read the changeset description, they may, like > me, not be able to suss out which calculation is in danger of being wild. > > Other than that: > > Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>Although, having said that, wouldn''t it be clearer to do something like: if ( dom->kernel-size - sizeof(*table) > 8192) ptr_end = dom->kernel_blob + 8192 else ptr_end = dom->kernel_size - sizeof(*table); Or something along those lines? -George
George Dunlap
2013-Jun-14 15:13 UTC
Re: [PATCH 18/23] libxc: Add range checking to xc_dom_binloader
On Fri, Jun 14, 2013 at 4:12 PM, George Dunlap <George.Dunlap@eu.citrix.com> wrote:> On Fri, Jun 14, 2013 at 4:09 PM, George Dunlap > <george.dunlap@eu.citrix.com> wrote: >> On 14/06/13 16:03, Ian Jackson wrote: >>> >>> George Dunlap writes ("Re: [Xen-devel] [PATCH 18/23] libxc: Add range >>> checking to xc_dom_binloader"): >>>> >>>> On Thu, Jun 13, 2013 at 7:14 PM, Ian Jackson <ian.jackson@eu.citrix.com> >>>> wrote: >>>>> >>>>> * When clamping the end of the region to search, that we do not >>>>> calculate pointers beyond the end of the image. The C >>>>> specification does not permit this and compilers are becoming ever >>>>> more determined to miscompile code when they can "prove" various >>>>> falsehoods based on assertions from the C spec. >>> >>> ... >>>>> >>>>> + if ( dom->kernel_size < sizeof(*table) ) >>>>> + return NULL; >>>>> probe_ptr = dom->kernel_blob; >>>>> probe_end = dom->kernel_blob + dom->kernel_size - sizeof(*table); >>>>> - if ( (void*)probe_end > (dom->kernel_blob + 8192) ) >>>>> + if ( dom->kernel_size >= 8192 && >>>>> + (void*)probe_end > (dom->kernel_blob + 8192) ) >>>>> probe_end = dom->kernel_blob + 8192; >>>> >>>> Wait, what''s going on here? Isn''t the point of this check originally >>>> that "probe_end" might be pointing off into nowhere, and you''re going >>>> to "clip" it into pointing somewhere reasonable? >>> >>> No. The (undocumented) format being parsed here is that the info >>> table should start no later than 8k into the file. probe_end is the >>> first address to no longer look for the table at. >>> >>> Firstly we set probe_end to the end of the image minus the table >>> length - which would imply searching the whole image. The (original >>> and unchanged) purpose of the if statement and assignment is to limit >>> this to 8192 bytes from the start of the image. >>> >>> However there is a bug: if the image is less than 8k long, this >>> involves computing a wild pointer (which is forbidden). So in my >>> patch I add an additional test. >>> >>>> It doesn''t look like you''ve actually changed any pointer arithmetic -- >>>> if either probe_end or dom->kernel_blob + 8192 are wild, then they''ll >>>> still be wild after this check, won''t they? >>> >>> The initial value of probe_end is guaranteed not to be wild. >>> >>> Before my change dom->kernel_blob + 8192 might be computed even if >>> it is wild; after my change it is only computed if it is guaranteed >>> not to be. >> >> >> Oh, I see -- right; so if "dom->kernel_size < 8192", then >> "dom->kernel_blob+8192" might be wild; but as long as "dom->kernel_size > >> 8192", then "dom->kernel_blob + 8192" will be fine. >> >> And if dom->kernel_size < 8192, then probe_end will already be < >> dom->kernel_blob+8192, so we don''t need to clip things. >> >> Might be nice to put a comment here, so people coming later can make sense >> out of this. Even if people read the changeset description, they may, like >> me, not be able to suss out which calculation is in danger of being wild. >> >> Other than that: >> >> Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com> > > Although, having said that, wouldn''t it be clearer to do something like: > > if ( dom->kernel-size - sizeof(*table) > 8192) > ptr_end = dom->kernel_blob + 8192 > else > ptr_end = dom->kernel_size - sizeof(*table);I mean ptr_end = dom->kernel_blob + dom->kernel_size - sizeof(*table); here of course... -G
Ian Jackson
2013-Jun-14 15:14 UTC
Re: [PATCH 18/23] libxc: Add range checking to xc_dom_binloader
George Dunlap writes ("Re: [Xen-devel] [PATCH 18/23] libxc: Add range checking to xc_dom_binloader"):> if ( dom->kernel-size - sizeof(*table) > 8192) > ptr_end = dom->kernel_blob + 8192 > else > ptr_end = dom->kernel_size - sizeof(*table); > > Or something along those lines?Yes, I said something similar to Chuck. Except that your code has a bug if kernel_size < sizeof(*table) :-). TBH I think I''d rather not disturb this any more than necessary. Ian.
George Dunlap
2013-Jun-14 15:19 UTC
Re: [PATCH 18/23] libxc: Add range checking to xc_dom_binloader
On 14/06/13 16:14, Ian Jackson wrote:> George Dunlap writes ("Re: [Xen-devel] [PATCH 18/23] libxc: Add range checking to xc_dom_binloader"): >> if ( dom->kernel-size - sizeof(*table) > 8192) >> ptr_end = dom->kernel_blob + 8192 >> else >> ptr_end = dom->kernel_size - sizeof(*table); >> >> Or something along those lines? > Yes, I said something similar to Chuck. Except that your code has a > bug if kernel_size < sizeof(*table) :-)....no, because that''s already checked by the previous hunk of your patch. :-)> TBH I think I''d rather not disturb this any more than necessary.I think disturbing it in a way that is obviously correct is better than disturbing it in a way that looks redundant. But I do think that the code in the patch you have, while unnecessarily confusing and likely to trip someone up down the road, will do what the patch claims it will do. -George
Ian Jackson
2013-Jun-14 15:24 UTC
Re: [PATCH 18/23] libxc: Add range checking to xc_dom_binloader
George Dunlap writes ("Re: [Xen-devel] [PATCH 18/23] libxc: Add range checking to xc_dom_binloader"):> I think disturbing it in a way that is obviously correct is better than > disturbing it in a way that looks redundant.That''s an argument. How about this then ? (diff against previous tip followed by revised patch) Ian. commit 202da02f40b1806138419002c3e29dc8ce0e88c2 Author: Ian Jackson <ian.jackson@eu.citrix.com> Date: Fri Jun 14 16:23:44 2013 +0100 Use clearer code for calculating probe_end diff --git a/.topmsg b/.topmsg index c5cff24..1e1b932 100644 --- a/.topmsg +++ b/.topmsg @@ -25,6 +25,8 @@ This is part of the fix to a security issue, XSA-55. Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> +v9: Use clearer code for calculating probe_end in find_table. + v6: Add a missing `return -EINVAL'' (Matthew Daley). Fix an error in the commit message (Matthew Daley). diff --git a/tools/libxc/xc_dom_binloader.c b/tools/libxc/xc_dom_binloader.c index 7eaf071..6469a65 100644 --- a/tools/libxc/xc_dom_binloader.c +++ b/tools/libxc/xc_dom_binloader.c @@ -126,10 +126,10 @@ static struct xen_bin_image_table *find_table(struct xc_dom_image *dom) if ( dom->kernel_size < sizeof(*table) ) return NULL; probe_ptr = dom->kernel_blob; - probe_end = dom->kernel_blob + dom->kernel_size - sizeof(*table); - if ( dom->kernel_size >= 8192 && - (void*)probe_end > (dom->kernel_blob + 8192) ) + if ( dom->kernel_size > (8192 + sizeof(*table)) ) probe_end = dom->kernel_blob + 8192; + else + probe_end = dom->kernel_blob + dom->kernel_size - sizeof(*table); for ( table = NULL; probe_ptr < probe_end; probe_ptr++ ) { From: Ian Jackson <ian.jackson@eu.citrix.com> Subject: [PATCH] libxc: Add range checking to xc_dom_binloader This is a simple binary image loader with its own metadata format. However, it is too careless with image-supplied values. Add the following checks: * That the image is bigger than the metadata table; otherwise the pointer arithmetic to calculate the metadata table location may yield undefined and dangerous values. * When clamping the end of the region to search, that we do not calculate pointers beyond the end of the image. The C specification does not permit this and compilers are becoming ever more determined to miscompile code when they can "prove" various falsehoods based on assertions from the C spec. * That the supplied image is big enough for the text we are allegedly copying from it. Otherwise we might have a read overrun and copy the results (perhaps a lot of secret data) into the guest. This is part of the fix to a security issue, XSA-55. Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> v9: Use clearer code for calculating probe_end in find_table. v6: Add a missing `return -EINVAL'' (Matthew Daley). Fix an error in the commit message (Matthew Daley). v5: This patch is new in this version of the series. --- tools/libxc/xc_dom_binloader.c | 15 +++++++++++++-- 1 files changed, 13 insertions(+), 2 deletions(-) diff --git a/tools/libxc/xc_dom_binloader.c b/tools/libxc/xc_dom_binloader.c index d2de04c..6469a65 100644 --- a/tools/libxc/xc_dom_binloader.c +++ b/tools/libxc/xc_dom_binloader.c @@ -123,10 +123,13 @@ static struct xen_bin_image_table *find_table(struct xc_dom_image *dom) uint32_t *probe_ptr; uint32_t *probe_end; + if ( dom->kernel_size < sizeof(*table) ) + return NULL; probe_ptr = dom->kernel_blob; - probe_end = dom->kernel_blob + dom->kernel_size - sizeof(*table); - if ( (void*)probe_end > (dom->kernel_blob + 8192) ) + if ( dom->kernel_size > (8192 + sizeof(*table)) ) probe_end = dom->kernel_blob + 8192; + else + probe_end = dom->kernel_blob + dom->kernel_size - sizeof(*table); for ( table = NULL; probe_ptr < probe_end; probe_ptr++ ) { @@ -282,6 +285,14 @@ static int xc_dom_load_bin_kernel(struct xc_dom_image *dom) return -EINVAL; } + if ( image_size < skip || + image_size - skip < text_size ) + { + DOMPRINTF("%s: image is too small for declared text size", + __FUNCTION__); + return -EINVAL; + } + memcpy(dest, image + skip, text_size); memset(dest + text_size, 0, bss_size); -- tg: (91d5c0e..) xsa55/binloader (depends on: xsa55/unobsolete)
George Dunlap
2013-Jun-14 15:29 UTC
Re: [PATCH 18/23] libxc: Add range checking to xc_dom_binloader
On 14/06/13 16:24, Ian Jackson wrote:> George Dunlap writes ("Re: [Xen-devel] [PATCH 18/23] libxc: Add range checking to xc_dom_binloader"): >> I think disturbing it in a way that is obviously correct is better than >> disturbing it in a way that looks redundant. > That''s an argument. > > How about this then ? > (diff against previous tip followed by revised patch) > > Ian. > > commit 202da02f40b1806138419002c3e29dc8ce0e88c2 > Author: Ian Jackson <ian.jackson@eu.citrix.com> > Date: Fri Jun 14 16:23:44 2013 +0100 > > Use clearer code for calculating probe_end > > diff --git a/.topmsg b/.topmsg > index c5cff24..1e1b932 100644 > --- a/.topmsg > +++ b/.topmsg > @@ -25,6 +25,8 @@ This is part of the fix to a security issue, XSA-55. > Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > > +v9: Use clearer code for calculating probe_end in find_table. > + > v6: Add a missing `return -EINVAL'' (Matthew Daley). > Fix an error in the commit message (Matthew Daley). > > diff --git a/tools/libxc/xc_dom_binloader.c b/tools/libxc/xc_dom_binloader.c > index 7eaf071..6469a65 100644 > --- a/tools/libxc/xc_dom_binloader.c > +++ b/tools/libxc/xc_dom_binloader.c > @@ -126,10 +126,10 @@ static struct xen_bin_image_table *find_table(struct xc_dom_image *dom) > if ( dom->kernel_size < sizeof(*table) ) > return NULL; > probe_ptr = dom->kernel_blob; > - probe_end = dom->kernel_blob + dom->kernel_size - sizeof(*table); > - if ( dom->kernel_size >= 8192 && > - (void*)probe_end > (dom->kernel_blob + 8192) ) > + if ( dom->kernel_size > (8192 + sizeof(*table)) ) > probe_end = dom->kernel_blob + 8192; > + else > + probe_end = dom->kernel_blob + dom->kernel_size - sizeof(*table); > > for ( table = NULL; probe_ptr < probe_end; probe_ptr++ ) > { > > > > From: Ian Jackson <ian.jackson@eu.citrix.com> > Subject: [PATCH] libxc: Add range checking to xc_dom_binloader > > This is a simple binary image loader with its own metadata format. > However, it is too careless with image-supplied values. > > Add the following checks: > > * That the image is bigger than the metadata table; otherwise the > pointer arithmetic to calculate the metadata table location may > yield undefined and dangerous values. > > * When clamping the end of the region to search, that we do not > calculate pointers beyond the end of the image. The C > specification does not permit this and compilers are becoming ever > more determined to miscompile code when they can "prove" various > falsehoods based on assertions from the C spec. > > * That the supplied image is big enough for the text we are allegedly > copying from it. Otherwise we might have a read overrun and copy > the results (perhaps a lot of secret data) into the guest. > > This is part of the fix to a security issue, XSA-55. > > Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>This looks good -- thanks: Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>