From d0790bdad7496e720416b2d4a04563c4c27e7b95 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Fri, 14 Jun 2013 16:43:17 +0100 Subject: [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 Acked-by: Ian Campbell Reviewed-by: Konrad Rzeszutek Wilk --- 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 b82a08c..ea45886 100644 --- a/tools/libxc/xc_dom_elfloader.c +++ b/tools/libxc/xc_dom_elfloader.c @@ -95,7 +95,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 5adc223..3600dca 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 a3310e7..f8be635 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 46ca553..744027e 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 ddc3ed7..ac93858 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 #From a965b8f80388603d439ae2b8ee7b9b018a079f90 Mon Sep 17 00:00:00 2001 #From: Ian Jackson #Date: Fri, 14 Jun 2013 16:43:17 +0100 #Subject: [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". # #Difference in the 4.2 series, compared to unstable: # #* tools/libxc/xc_hvm_build_x86.c:setup_guest and # xen/arch/arm/kernel.c:kernel_try_elf_prepare have different # error handling in 4.2 to unstable; patch adjusted accordingly. # #This is part of the fix to a security issue, XSA-55. # #Signed-off-by: Ian Jackson # #xen-unstable version Reviewed-by: George Dunlap #--- # tools/libxc/xc_dom_elfloader.c | 25 +++++++++++++++++++++---- # tools/libxc/xc_hvm_build_x86.c | 5 +++++ # tools/xcutils/readnotes.c | 3 +++ # xen/arch/arm/kernel.c | 15 ++++++++++++++- # xen/arch/x86/domain_build.c | 28 +++++++++++++++++++++------- # 5 files changed, 64 insertions(+), 12 deletions(-) # diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c index ea45886..4fb4da2 100644 --- a/tools/libxc/xc_dom_elfloader.c +++ b/tools/libxc/xc_dom_elfloader.c @@ -276,6 +276,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__); @@ -312,19 +319,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 */ @@ -338,7 +349,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 ccfd8b5..8165287 100644 --- a/tools/libxc/xc_hvm_build_x86.c +++ b/tools/libxc/xc_hvm_build_x86.c @@ -403,11 +403,16 @@ static int setup_guest(xc_interface *xch, munmap(page0, PAGE_SIZE); } + if ( elf_check_broken(&elf) ) + ERROR("HVM ELF broken: %s", elf_check_broken(&elf)); + free(page_array); return 0; error_out: free(page_array); + if ( elf_check_broken(&elf) ) + ERROR("HVM ELF broken, failing: %s", elf_check_broken(&elf)); return -1; } 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 2d56130..dec0519 100644 --- a/xen/arch/arm/kernel.c +++ b/xen/arch/arm/kernel.c @@ -146,6 +146,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(KERNEL_FLASH_SIZE); info->kernel_img = alloc_xenheap_pages(info->kernel_order, 0); if ( info->kernel_img == NULL ) @@ -160,7 +162,7 @@ static int kernel_try_elf_prepare(struct kernel_info *info) #endif elf_parse_binary(&info->elf.elf); if ( (rc = elf_xen_parse(&info->elf.elf, &info->elf.parms)) != 0 ) - return rc; + goto err; /* * TODO: can the ELF header be used to find the physical address @@ -169,7 +171,18 @@ 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)); + + return rc; } int kernel_prepare(struct kernel_info *info) diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c index a655b21..0dbec96 100644 --- a/xen/arch/x86/domain_build.c +++ b/xen/arch/x86/domain_build.c @@ -374,7 +374,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; @@ -413,14 +413,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 defined(__x86_64__) @@ -734,7 +736,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) ) @@ -914,7 +917,7 @@ int __init construct_dom0( if ( rc < 0 ) { printk("Failed to load the kernel binary\n"); - return rc; + goto out; } bootstrap_map(NULL); @@ -925,7 +928,8 @@ int __init construct_dom0( { 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); @@ -1272,9 +1276,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