|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 12/16] libelf: Make all callers call elf_check_broken
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@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> v2 was Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>
> 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));
> +
I know this is just nitpicking, but trailing whitespace. As far as I
can tell, this is the only instance in the whole series
~Andrew
> + 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));
>
> + 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;
> }
>
> /*
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |