Re: [PATCH 12/16] libelf: Make all callers call elf_check_broken

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 04/06/13 18:59, Ian Jackson wrote:
> 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".
>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
>
> 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 177219f..f1abe15 100644
> --- a/tools/libxc/xc_dom_elfloader.c
> +++ b/tools/libxc/xc_dom_elfloader.c
> @@ -275,6 +275,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__);
> @@ -311,19 +318,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 */
> @@ -337,7 +348,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 ca86ba5..b868fba 100644
> --- a/tools/xcutils/readnotes.c
> +++ b/tools/xcutils/readnotes.c
> @@ -300,6 +300,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));

this should be


if ( elf_check_broken(&elf) )
{
    printk(foo);
    return -EINVAL;
}

Otherwise we exit construct_dom0() with an rc of 0.

During my testing, this involved dying with a general protection fault
very shortly after the dom0 entry point.

~Andrew

>  
> +    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;
>  }
>  
>  /*