* Re: [PATCH v4] xen/tools: Introduce QNX IFS loader
2014-09-24 15:43 ` Oleksandr Tyshchenko
@ 2014-09-25 10:25 ` Ian Campbell
2014-09-25 15:51 ` Oleksandr Tyshchenko
2014-09-25 15:54 ` Andrii Tseglytskyi
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Ian Campbell @ 2014-09-25 10:25 UTC (permalink / raw)
To: Oleksandr Tyshchenko
Cc: tim, julien.grall, Ian.Jackson, stefano.stabellini, xen-devel
On Wed, 2014-09-24 at 18:43 +0300, Oleksandr Tyshchenko wrote:
> Signed-off-by: Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Julien Grall <julien.grall@linaro.org>
I think we have essentially decided to follow the procedure outlined by
Ian in http://article.gmane.org/gmane.comp.emulators.xen.devel/216261
(at least noone objected). So this is missing at least "an ack from a
colleague". Has this really been through a "serious and thorough
security review" internally in the two days since Ian wrote that
mail?
Are you sure you want to present this version under the terms laid out
by Ian in the second-from-last paragraph of his mail?
Ian.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v4] xen/tools: Introduce QNX IFS loader
2014-09-25 10:25 ` Ian Campbell
@ 2014-09-25 15:51 ` Oleksandr Tyshchenko
0 siblings, 0 replies; 10+ messages in thread
From: Oleksandr Tyshchenko @ 2014-09-25 15:51 UTC (permalink / raw)
To: Ian Campbell
Cc: Tim Deegan, Julien Grall, Ian Jackson, Stefano Stabellini,
xen-devel@lists.xen.org
Hi, Ian
On Thu, Sep 25, 2014 at 1:25 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>
> On Wed, 2014-09-24 at 18:43 +0300, Oleksandr Tyshchenko wrote:
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Julien Grall <julien.grall@linaro.org>
>
> I think we have essentially decided to follow the procedure outlined by
> Ian in http://article.gmane.org/gmane.comp.emulators.xen.devel/216261
> (at least noone objected). So this is missing at least "an ack from a
> colleague". Has this really been through a "serious and thorough
> security review" internally in the two days since Ian wrote that
> mail?
Yes, it has. Two my colleagues have reviewed this patch.
Moreover, today we have checked this patch on current master.
>
> Are you sure you want to present this version under the terms laid out
> by Ian in the second-from-last paragraph of his mail?
Yes, I am.
>
> Ian.
>
--
Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4] xen/tools: Introduce QNX IFS loader
2014-09-24 15:43 ` Oleksandr Tyshchenko
2014-09-25 10:25 ` Ian Campbell
@ 2014-09-25 15:54 ` Andrii Tseglytskyi
2014-09-25 15:55 ` Andrii Anisov
2014-09-26 14:37 ` Ian Jackson
3 siblings, 0 replies; 10+ messages in thread
From: Andrii Tseglytskyi @ 2014-09-25 15:54 UTC (permalink / raw)
To: Oleksandr Tyshchenko
Cc: Ian Campbell, Stefano Stabellini, Ian Jackson, Julien Grall,
Tim Deegan, xen-devel@lists.xen.org
On Wed, Sep 24, 2014 at 6:43 PM, Oleksandr Tyshchenko
<oleksandr.tyshchenko@globallogic.com> wrote:
> This patch was developed according to instruction:
> http://www.qnx.com/developers/docs/6.4.1/neutrino/building/load_process.html
>
> Add ability to load QNX IFS image. The loader probe function
> based on "Writing an IPL Program" howto from qnx.com
> and performs image validation and startup entry point location.
> Because of the fact that the image is already in place some
> customizations have been done. Also in case of multiple images
> (very seldom case) only the first image will be loaded.
>
> There are some restrictions:
> 1. The base address of the image (image attribute) and the location
> of RAM in system must be synchronized with RAM base address in Xen.
> 2. The QNX IFS image must be created as a simple binary image.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
Acked-by: Andrii Tseglytskyi <andrii.tseglytskyi@globallogic.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Julien Grall <julien.grall@linaro.org>
> ---
> tools/libxc/Makefile | 1 +
> tools/libxc/xc_dom_qnxifsloader.c | 228 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 229 insertions(+)
> create mode 100644 tools/libxc/xc_dom_qnxifsloader.c
>
> diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
> index 2cca2b2..1462f53 100644
> --- a/tools/libxc/Makefile
> +++ b/tools/libxc/Makefile
> @@ -66,6 +66,7 @@ GUEST_SRCS-y += xc_dom_elfloader.c
> GUEST_SRCS-$(CONFIG_X86) += xc_dom_bzimageloader.c
> GUEST_SRCS-$(CONFIG_X86) += xc_dom_decompress_lz4.c
> GUEST_SRCS-$(CONFIG_ARM) += xc_dom_armzimageloader.c
> +GUEST_SRCS-$(CONFIG_ARM) += xc_dom_qnxifsloader.c
> GUEST_SRCS-y += xc_dom_binloader.c
> GUEST_SRCS-y += xc_dom_compat_linux.c
>
> diff --git a/tools/libxc/xc_dom_qnxifsloader.c b/tools/libxc/xc_dom_qnxifsloader.c
> new file mode 100644
> index 0000000..8b03885
> --- /dev/null
> +++ b/tools/libxc/xc_dom_qnxifsloader.c
> @@ -0,0 +1,228 @@
> +/*
> + * Xen domain builder -- QNX IFS bits
> + *
> + * Parse and load QNX IFS image.
> + *
> + * Copyright (C) 2014, Globallogic.
> + *
> + * 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 <stdio.h>
> +#include <stdlib.h>
> +#include <inttypes.h>
> +
> +#include "xg_private.h"
> +#include "xc_dom.h"
> +
> +struct startup_header {
> + uint32_t signature; /* Header sig */
> + uint16_t version; /* Header vers */
> + uint8_t flags1; /* Misc flags */
> + uint8_t flags2; /* No flags defined yet */
> + uint16_t header_size; /* sizeof(struct startup_header) */
> + uint16_t machine; /* Machine type */
> + uint32_t startup_vaddr; /* Virtual Address to transfer */
> + /* to after IPL is done */
> + uint32_t paddr_bias; /* Value to add to physical address */
> + /* to get a value to put into a */
> + /* pointer and indirected through */
> + uint32_t image_paddr; /* Physical address of image */
> + uint32_t ram_paddr; /* Physical address of RAM to copy */
> + /* image to (startup_size bytes copied) */
> + uint32_t ram_size; /* Amount of RAM used by the startup */
> + /* program and executables contained */
> + /* in the file system */
> + uint32_t startup_size; /* Size of startup (never compressed) */
> + uint32_t stored_size; /* Size of entire image */
> + uint32_t imagefs_paddr; /* Set by IPL to where the imagefs is */
> + /* when startup runs */
> + uint32_t imagefs_size; /* Size of uncompressed imagefs */
> + uint16_t preboot_size; /* Size of loaded before header */
> + uint16_t zero0; /* Zeros */
> + uint32_t zero[3]; /* Zeros */
> + uint32_t info[48]; /* Array of startup_info* structures */
> +};
> +
> +#define STARTUP_HDR_SIGNATURE 0x00ff7eeb
> +
> +static int calc_checksum(uint32_t *ptr, uint32_t size)
> +{
> + int sum = 0;
> +
> + while ( size > 0 )
> + {
> + sum += *ptr++;
> + size -= 4;
> + }
> +
> + return sum;
> +}
> +
> +static int xc_dom_probe_qnx_ifs(struct xc_dom_image *dom)
> +{
> + struct startup_header *startup_hdr;
> +
> + if ( dom->kernel_blob == NULL )
> + {
> + xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
> + "%s: no QNX IFS loaded", __FUNCTION__);
> + return -EINVAL;
> + }
> +
> + /* Performs a preliminary checks for a valid image size */
> + if ( dom->kernel_size < sizeof(struct startup_header) )
> + {
> + xc_dom_printf(dom->xch, "%s: QNX IFS is too small", __FUNCTION__);
> + return -EINVAL;
> + }
> +
> + /*
> + * Performs a check for a valid OS signature. We assume that nothing
> + * preceded by startup header because we expect a simple binary image.
> + */
> + startup_hdr = (struct startup_header *)dom->kernel_blob;
> + if ( (startup_hdr->signature != STARTUP_HDR_SIGNATURE) ||
> + (startup_hdr->preboot_size != 0) )
> + {
> + xc_dom_printf(dom->xch, "%s: image is not a QNX IFS", __FUNCTION__);
> + return -EINVAL;
> + }
> +
> + /* Performs a check for a valid startup header size */
> + if ( startup_hdr->header_size != sizeof(struct startup_header) )
> + {
> + xc_dom_printf(dom->xch, "%s: QNX IFS has wrong startup header size", __FUNCTION__);
> + return -EINVAL;
> + }
> +
> + /* Performs a check for a valid stored size */
> + if ( startup_hdr->stored_size != dom->kernel_size )
> + {
> + xc_dom_printf(dom->xch, "%s: QNX IFS has wrong stored size", __FUNCTION__);
> + return -EINVAL;
> + }
> +
> + /* Performs a check for a valid startup size */
> + if ( startup_hdr->startup_size >= startup_hdr->stored_size )
> + {
> + xc_dom_printf(dom->xch, "%s: QNX IFS has wrong startup size", __FUNCTION__);
> + return -EINVAL;
> + }
> +
> + /* Performs a checksums on the startup and the OS image filesystem */
> + if ( (calc_checksum((uint32_t *)startup_hdr, startup_hdr->startup_size) != 0) ||
> + (calc_checksum((uint32_t *)startup_hdr + startup_hdr->startup_size/4,
> + startup_hdr->stored_size - startup_hdr->startup_size) != 0) )
> + {
> + xc_dom_printf(dom->xch, "%s: QNX IFS has wrong checksum", __FUNCTION__);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int xc_dom_parse_qnx_ifs(struct xc_dom_image *dom)
> +{
> + struct startup_header *startup_hdr;
> + uint64_t v_start, v_end;
> + uint64_t rambase = dom->rambase_pfn << XC_PAGE_SHIFT;
> +
> + DOMPRINTF_CALLED(dom->xch);
> +
> + /* Do not load kernel at the very first RAM address */
> + v_start = rambase + 0x8000;
> +
> + if ( dom->kernel_size > UINT64_MAX - v_start )
> + {
> + xc_dom_printf(dom->xch, "%s: QNX IFS is too big", __FUNCTION__);
> + return -EINVAL;
> + }
> +
> + v_end = v_start + dom->kernel_size;
> +
> + /* find kernel segment */
> + dom->kernel_seg.vstart = v_start;
> + dom->kernel_seg.vend = v_end;
> +
> + startup_hdr = (struct startup_header *)dom->kernel_blob;
> +
> + /* Performs a sanity check for a valid startup entry point */
> + if ( (startup_hdr->startup_vaddr < v_start) ||
> + (startup_hdr->startup_vaddr > v_end) )
> + {
> + xc_dom_printf(dom->xch, "%s: QNX IFS has wrong startup entry point", __FUNCTION__);
> + return -EINVAL;
> + }
> +
> + dom->parms.virt_entry = startup_hdr->startup_vaddr;
> + dom->parms.virt_base = rambase;
> +
> + dom->guest_type = "xen-3.0-armv7l";
> + DOMPRINTF("%s: %s: RAM starts at %"PRI_xen_pfn,
> + __FUNCTION__, dom->guest_type, dom->rambase_pfn);
> + DOMPRINTF("%s: %s: 0x%" PRIx64 " -> 0x%" PRIx64 "",
> + __FUNCTION__, dom->guest_type,
> + dom->kernel_seg.vstart, dom->kernel_seg.vend);
> +
> + return 0;
> +}
> +
> +static int xc_dom_load_qnx_ifs(struct xc_dom_image *dom)
> +{
> + void *dst;
> +
> + 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 seg %#"PRIx64"-%#"PRIx64,
> + __func__, dom->kernel_seg.vstart, dom->kernel_seg.vend);
> + DOMPRINTF("%s: copy %zd bytes from blob %p to dst %p",
> + __func__, dom->kernel_size, dom->kernel_blob, dst);
> +
> + memcpy(dst, dom->kernel_blob, dom->kernel_size);
> +
> + return 0;
> +}
> +
> +static struct xc_dom_loader qnx_ifs_loader = {
> + .name = "QNX IFS",
> + .probe = xc_dom_probe_qnx_ifs,
> + .parser = xc_dom_parse_qnx_ifs,
> + .loader = xc_dom_load_qnx_ifs,
> +};
> +
> +static void __init register_loader(void)
> +{
> + xc_dom_register_loader(&qnx_ifs_loader);
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> --
> 1.9.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
--
Andrii Tseglytskyi | Embedded Dev
GlobalLogic
www.globallogic.com
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v4] xen/tools: Introduce QNX IFS loader
2014-09-24 15:43 ` Oleksandr Tyshchenko
2014-09-25 10:25 ` Ian Campbell
2014-09-25 15:54 ` Andrii Tseglytskyi
@ 2014-09-25 15:55 ` Andrii Anisov
2014-09-26 14:37 ` Ian Jackson
3 siblings, 0 replies; 10+ messages in thread
From: Andrii Anisov @ 2014-09-25 15:55 UTC (permalink / raw)
To: Oleksandr Tyshchenko
Cc: Ian Campbell, Stefano Stabellini, Ian Jackson, Julien Grall,
Tim Deegan, xen-devel@lists.xen.org
Acked-by: Andrii Anisov <andrii.anisov@globallogic.com>
Andrii Anisov | Team Lead
GlobalLogic
Kyiv, 03038, Protasov Business Park, M.Grinchenka, 2/1
P +38.044.492.9695x3664 M +380505738852 S andriyanisov
www.globallogic.com
http://www.globallogic.com/email_disclaimer.txt
On Wed, Sep 24, 2014 at 6:43 PM, Oleksandr Tyshchenko
<oleksandr.tyshchenko@globallogic.com> wrote:
> This patch was developed according to instruction:
> http://www.qnx.com/developers/docs/6.4.1/neutrino/building/load_process.html
>
> Add ability to load QNX IFS image. The loader probe function
> based on "Writing an IPL Program" howto from qnx.com
> and performs image validation and startup entry point location.
> Because of the fact that the image is already in place some
> customizations have been done. Also in case of multiple images
> (very seldom case) only the first image will be loaded.
>
> There are some restrictions:
> 1. The base address of the image (image attribute) and the location
> of RAM in system must be synchronized with RAM base address in Xen.
> 2. The QNX IFS image must be created as a simple binary image.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Julien Grall <julien.grall@linaro.org>
> ---
> tools/libxc/Makefile | 1 +
> tools/libxc/xc_dom_qnxifsloader.c | 228 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 229 insertions(+)
> create mode 100644 tools/libxc/xc_dom_qnxifsloader.c
>
> diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
> index 2cca2b2..1462f53 100644
> --- a/tools/libxc/Makefile
> +++ b/tools/libxc/Makefile
> @@ -66,6 +66,7 @@ GUEST_SRCS-y += xc_dom_elfloader.c
> GUEST_SRCS-$(CONFIG_X86) += xc_dom_bzimageloader.c
> GUEST_SRCS-$(CONFIG_X86) += xc_dom_decompress_lz4.c
> GUEST_SRCS-$(CONFIG_ARM) += xc_dom_armzimageloader.c
> +GUEST_SRCS-$(CONFIG_ARM) += xc_dom_qnxifsloader.c
> GUEST_SRCS-y += xc_dom_binloader.c
> GUEST_SRCS-y += xc_dom_compat_linux.c
>
> diff --git a/tools/libxc/xc_dom_qnxifsloader.c b/tools/libxc/xc_dom_qnxifsloader.c
> new file mode 100644
> index 0000000..8b03885
> --- /dev/null
> +++ b/tools/libxc/xc_dom_qnxifsloader.c
> @@ -0,0 +1,228 @@
> +/*
> + * Xen domain builder -- QNX IFS bits
> + *
> + * Parse and load QNX IFS image.
> + *
> + * Copyright (C) 2014, Globallogic.
> + *
> + * 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 <stdio.h>
> +#include <stdlib.h>
> +#include <inttypes.h>
> +
> +#include "xg_private.h"
> +#include "xc_dom.h"
> +
> +struct startup_header {
> + uint32_t signature; /* Header sig */
> + uint16_t version; /* Header vers */
> + uint8_t flags1; /* Misc flags */
> + uint8_t flags2; /* No flags defined yet */
> + uint16_t header_size; /* sizeof(struct startup_header) */
> + uint16_t machine; /* Machine type */
> + uint32_t startup_vaddr; /* Virtual Address to transfer */
> + /* to after IPL is done */
> + uint32_t paddr_bias; /* Value to add to physical address */
> + /* to get a value to put into a */
> + /* pointer and indirected through */
> + uint32_t image_paddr; /* Physical address of image */
> + uint32_t ram_paddr; /* Physical address of RAM to copy */
> + /* image to (startup_size bytes copied) */
> + uint32_t ram_size; /* Amount of RAM used by the startup */
> + /* program and executables contained */
> + /* in the file system */
> + uint32_t startup_size; /* Size of startup (never compressed) */
> + uint32_t stored_size; /* Size of entire image */
> + uint32_t imagefs_paddr; /* Set by IPL to where the imagefs is */
> + /* when startup runs */
> + uint32_t imagefs_size; /* Size of uncompressed imagefs */
> + uint16_t preboot_size; /* Size of loaded before header */
> + uint16_t zero0; /* Zeros */
> + uint32_t zero[3]; /* Zeros */
> + uint32_t info[48]; /* Array of startup_info* structures */
> +};
> +
> +#define STARTUP_HDR_SIGNATURE 0x00ff7eeb
> +
> +static int calc_checksum(uint32_t *ptr, uint32_t size)
> +{
> + int sum = 0;
> +
> + while ( size > 0 )
> + {
> + sum += *ptr++;
> + size -= 4;
> + }
> +
> + return sum;
> +}
> +
> +static int xc_dom_probe_qnx_ifs(struct xc_dom_image *dom)
> +{
> + struct startup_header *startup_hdr;
> +
> + if ( dom->kernel_blob == NULL )
> + {
> + xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
> + "%s: no QNX IFS loaded", __FUNCTION__);
> + return -EINVAL;
> + }
> +
> + /* Performs a preliminary checks for a valid image size */
> + if ( dom->kernel_size < sizeof(struct startup_header) )
> + {
> + xc_dom_printf(dom->xch, "%s: QNX IFS is too small", __FUNCTION__);
> + return -EINVAL;
> + }
> +
> + /*
> + * Performs a check for a valid OS signature. We assume that nothing
> + * preceded by startup header because we expect a simple binary image.
> + */
> + startup_hdr = (struct startup_header *)dom->kernel_blob;
> + if ( (startup_hdr->signature != STARTUP_HDR_SIGNATURE) ||
> + (startup_hdr->preboot_size != 0) )
> + {
> + xc_dom_printf(dom->xch, "%s: image is not a QNX IFS", __FUNCTION__);
> + return -EINVAL;
> + }
> +
> + /* Performs a check for a valid startup header size */
> + if ( startup_hdr->header_size != sizeof(struct startup_header) )
> + {
> + xc_dom_printf(dom->xch, "%s: QNX IFS has wrong startup header size", __FUNCTION__);
> + return -EINVAL;
> + }
> +
> + /* Performs a check for a valid stored size */
> + if ( startup_hdr->stored_size != dom->kernel_size )
> + {
> + xc_dom_printf(dom->xch, "%s: QNX IFS has wrong stored size", __FUNCTION__);
> + return -EINVAL;
> + }
> +
> + /* Performs a check for a valid startup size */
> + if ( startup_hdr->startup_size >= startup_hdr->stored_size )
> + {
> + xc_dom_printf(dom->xch, "%s: QNX IFS has wrong startup size", __FUNCTION__);
> + return -EINVAL;
> + }
> +
> + /* Performs a checksums on the startup and the OS image filesystem */
> + if ( (calc_checksum((uint32_t *)startup_hdr, startup_hdr->startup_size) != 0) ||
> + (calc_checksum((uint32_t *)startup_hdr + startup_hdr->startup_size/4,
> + startup_hdr->stored_size - startup_hdr->startup_size) != 0) )
> + {
> + xc_dom_printf(dom->xch, "%s: QNX IFS has wrong checksum", __FUNCTION__);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int xc_dom_parse_qnx_ifs(struct xc_dom_image *dom)
> +{
> + struct startup_header *startup_hdr;
> + uint64_t v_start, v_end;
> + uint64_t rambase = dom->rambase_pfn << XC_PAGE_SHIFT;
> +
> + DOMPRINTF_CALLED(dom->xch);
> +
> + /* Do not load kernel at the very first RAM address */
> + v_start = rambase + 0x8000;
> +
> + if ( dom->kernel_size > UINT64_MAX - v_start )
> + {
> + xc_dom_printf(dom->xch, "%s: QNX IFS is too big", __FUNCTION__);
> + return -EINVAL;
> + }
> +
> + v_end = v_start + dom->kernel_size;
> +
> + /* find kernel segment */
> + dom->kernel_seg.vstart = v_start;
> + dom->kernel_seg.vend = v_end;
> +
> + startup_hdr = (struct startup_header *)dom->kernel_blob;
> +
> + /* Performs a sanity check for a valid startup entry point */
> + if ( (startup_hdr->startup_vaddr < v_start) ||
> + (startup_hdr->startup_vaddr > v_end) )
> + {
> + xc_dom_printf(dom->xch, "%s: QNX IFS has wrong startup entry point", __FUNCTION__);
> + return -EINVAL;
> + }
> +
> + dom->parms.virt_entry = startup_hdr->startup_vaddr;
> + dom->parms.virt_base = rambase;
> +
> + dom->guest_type = "xen-3.0-armv7l";
> + DOMPRINTF("%s: %s: RAM starts at %"PRI_xen_pfn,
> + __FUNCTION__, dom->guest_type, dom->rambase_pfn);
> + DOMPRINTF("%s: %s: 0x%" PRIx64 " -> 0x%" PRIx64 "",
> + __FUNCTION__, dom->guest_type,
> + dom->kernel_seg.vstart, dom->kernel_seg.vend);
> +
> + return 0;
> +}
> +
> +static int xc_dom_load_qnx_ifs(struct xc_dom_image *dom)
> +{
> + void *dst;
> +
> + 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 seg %#"PRIx64"-%#"PRIx64,
> + __func__, dom->kernel_seg.vstart, dom->kernel_seg.vend);
> + DOMPRINTF("%s: copy %zd bytes from blob %p to dst %p",
> + __func__, dom->kernel_size, dom->kernel_blob, dst);
> +
> + memcpy(dst, dom->kernel_blob, dom->kernel_size);
> +
> + return 0;
> +}
> +
> +static struct xc_dom_loader qnx_ifs_loader = {
> + .name = "QNX IFS",
> + .probe = xc_dom_probe_qnx_ifs,
> + .parser = xc_dom_parse_qnx_ifs,
> + .loader = xc_dom_load_qnx_ifs,
> +};
> +
> +static void __init register_loader(void)
> +{
> + xc_dom_register_loader(&qnx_ifs_loader);
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> --
> 1.9.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v4] xen/tools: Introduce QNX IFS loader
2014-09-24 15:43 ` Oleksandr Tyshchenko
` (2 preceding siblings ...)
2014-09-25 15:55 ` Andrii Anisov
@ 2014-09-26 14:37 ` Ian Jackson
2014-09-26 16:21 ` Oleksandr Tyshchenko
3 siblings, 1 reply; 10+ messages in thread
From: Ian Jackson @ 2014-09-26 14:37 UTC (permalink / raw)
To: Oleksandr Tyshchenko
Cc: julien.grall, tim, ian.campbell, stefano.stabellini, xen-devel
Oleksandr Tyshchenko writes ("[PATCH v4] xen/tools: Introduce QNX IFS loader"):
> This patch was developed according to instruction:
> http://www.qnx.com/developers/docs/6.4.1/neutrino/building/load_process.html
...
> +static int xc_dom_probe_qnx_ifs(struct xc_dom_image *dom)
> +{
...
> + /* Performs a checksums on the startup and the OS image filesystem */
> + if ( (calc_checksum((uint32_t *)startup_hdr, startup_hdr->startup_size) != 0) ||
> + (calc_checksum((uint32_t *)startup_hdr + startup_hdr->startup_size/4,
> + startup_hdr->stored_size - startup_hdr->startup_size) != 0) )
Suppose that the incoming image is corrupt or malicious and
startup_header.startup_size and dom->kernel_size are both equal to
sizeof(startup_header)+1.
(By hand I count startup_header to have size 64, so assuming that's
right, and writing things in hex:)
Then the first call to calc_checksum looks like this:
calc_checksum( dom->kernel_blob, 0x41 )
For the first 0x10 iterations calc_checksum will read successive
uint32_t's from kernel_blob (for a total of 0x40 bytes) and reduce
size to 0x01.
The next iteration of calc_checksum will read a uint32_t from
dom->kernel_blob+0x40. But kernel_size==0x41 so this is a 3-byte
buffer read overrun - a vulnerability, technically, I think.
But worse happens next. calc_checksum then has size=0x01 and does
size -= 4;
leaving size with the value 0xfffffffd. Because size is a uint32_t
this is positive, not negative, and satisfies the test in the loop.
I.e. calc_checksum will continue to iterate forever. It will keep
reading memory at ever increasing addresses until it hits an invalid
address, and then crash.
I'm afraid I think this is a readily exploitable denial of service
vulnerability.
Ian.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v4] xen/tools: Introduce QNX IFS loader
2014-09-26 14:37 ` Ian Jackson
@ 2014-09-26 16:21 ` Oleksandr Tyshchenko
2014-09-26 16:35 ` Ian Jackson
0 siblings, 1 reply; 10+ messages in thread
From: Oleksandr Tyshchenko @ 2014-09-26 16:21 UTC (permalink / raw)
To: Ian Jackson
Cc: Julien Grall, Tim Deegan, Ian Campbell, Stefano Stabellini,
xen-devel@lists.xen.org
Hi Ian.
Thank you for your review.
On Fri, Sep 26, 2014 at 5:37 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Oleksandr Tyshchenko writes ("[PATCH v4] xen/tools: Introduce QNX IFS loader"):
>> This patch was developed according to instruction:
>> http://www.qnx.com/developers/docs/6.4.1/neutrino/building/load_process.html
> ...
>> +static int xc_dom_probe_qnx_ifs(struct xc_dom_image *dom)
>> +{
> ...
>> + /* Performs a checksums on the startup and the OS image filesystem */
>> + if ( (calc_checksum((uint32_t *)startup_hdr, startup_hdr->startup_size) != 0) ||
>> + (calc_checksum((uint32_t *)startup_hdr + startup_hdr->startup_size/4,
>> + startup_hdr->stored_size - startup_hdr->startup_size) != 0) )
>
> Suppose that the incoming image is corrupt or malicious and
> startup_header.startup_size and dom->kernel_size are both equal to
> sizeof(startup_header)+1.
>
> (By hand I count startup_header to have size 64, so assuming that's
> right, and writing things in hex:)
ok. Maybe, do you mean that (stored_size == kernel_size) instead of
(startup_size == kernel_size)?
1. If No (startup_size == kernel_size) -> I think, there is no issue.
We will not calculate checksum in this case.
Before calculating checksums we check stored_size too.
- assume that (stored_size != kernel_size):
it this case probe will fail.
- assume that (stored_size == kernel_size):
it this case probe will fail too, because stored_size must be greater
than startup_size.
2. If Yes (stored_size == kernel_size)
Let's look in to additional requirements:
- startup_size should be corrupted too so that (startup_size < stored_size)
- first checksum verification should returns success.
Is it possible ?
>
> Then the first call to calc_checksum looks like this:
> calc_checksum( dom->kernel_blob, 0x41 )
>
> For the first 0x10 iterations calc_checksum will read successive
> uint32_t's from kernel_blob (for a total of 0x40 bytes) and reduce
> size to 0x01.
>
> The next iteration of calc_checksum will read a uint32_t from
> dom->kernel_blob+0x40. But kernel_size==0x41 so this is a 3-byte
> buffer read overrun - a vulnerability, technically, I think.
>
> But worse happens next. calc_checksum then has size=0x01 and does
> size -= 4;
> leaving size with the value 0xfffffffd. Because size is a uint32_t
> this is positive, not negative, and satisfies the test in the loop.
>
> I.e. calc_checksum will continue to iterate forever. It will keep
> reading memory at ever increasing addresses until it hits an invalid
> address, and then crash.
>
> I'm afraid I think this is a readily exploitable denial of service
> vulnerability.
I agree with you that size should be int32_t and ((size % 4) == 0).
It is my mistake.
>
> Ian.
--
Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v4] xen/tools: Introduce QNX IFS loader
2014-09-26 16:21 ` Oleksandr Tyshchenko
@ 2014-09-26 16:35 ` Ian Jackson
2014-09-26 16:47 ` Oleksandr Tyshchenko
0 siblings, 1 reply; 10+ messages in thread
From: Ian Jackson @ 2014-09-26 16:35 UTC (permalink / raw)
To: Oleksandr Tyshchenko
Cc: Julien Grall, Tim Deegan, Ian Campbell, Stefano Stabellini,
xen-devel@lists.xen.org
Oleksandr Tyshchenko writes ("Re: [PATCH v4] xen/tools: Introduce QNX IFS loader"):
> On Fri, Sep 26, 2014 at 5:37 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> > Suppose that the incoming image is corrupt or malicious and
> > startup_header.startup_size and dom->kernel_size are both equal to
> > sizeof(startup_header)+1.
...
> ok. Maybe, do you mean that (stored_size == kernel_size) instead of
> (startup_size == kernel_size)?
You are right that I had failed to properly analyse the condition on
startup_size and stored_size. I guess that just goes to show how hard
this is.
But I think there is still an attack. Consider:
startup_size == 3;
stored_size == kernel_size == sizeof(startup_header);
Then the first calc_checksum gets 3 as a size argument and loops
forever scanning memory until it crashes.
Ian.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v4] xen/tools: Introduce QNX IFS loader
2014-09-26 16:35 ` Ian Jackson
@ 2014-09-26 16:47 ` Oleksandr Tyshchenko
0 siblings, 0 replies; 10+ messages in thread
From: Oleksandr Tyshchenko @ 2014-09-26 16:47 UTC (permalink / raw)
To: Ian Jackson
Cc: Julien Grall, Tim Deegan, Ian Campbell, Stefano Stabellini,
xen-devel@lists.xen.org
On Fri, Sep 26, 2014 at 7:35 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Oleksandr Tyshchenko writes ("Re: [PATCH v4] xen/tools: Introduce QNX IFS loader"):
>> On Fri, Sep 26, 2014 at 5:37 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
>> > Suppose that the incoming image is corrupt or malicious and
>> > startup_header.startup_size and dom->kernel_size are both equal to
>> > sizeof(startup_header)+1.
> ...
>> ok. Maybe, do you mean that (stored_size == kernel_size) instead of
>> (startup_size == kernel_size)?
>
> You are right that I had failed to properly analyse the condition on
> startup_size and stored_size. I guess that just goes to show how hard
> this is.
>
> But I think there is still an attack. Consider:
> startup_size == 3;
> stored_size == kernel_size == sizeof(startup_header);
>
> Then the first calc_checksum gets 3 as a size argument and loops
> forever scanning memory until it crashes.
agree
>
> Ian.
--
Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com
^ permalink raw reply [flat|nested] 10+ messages in thread