Re: [Xen-devel] [PATCH v3] xen/tools: Introduce QNX IFS loader

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2014-09-22 at 21:12 +0300, Oleksandr Tyshchenko 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.

It might be nice (eventually, not necessarily now) to add something to
the wiki or the docs/ subtree which describes how to build a suitable
QNX binary.

> Change-Id: Id125d6eae5847698fb5734c7f28451f5b752062f

What is this?

[...]
> +    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;
> +    }

This check should be in the parse callback.

> +
> +    /*
> +     * 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 )
> +    {
> +        xc_dom_printf(dom->xch, "%s: image is not a QNX IFS", __FUNCTION__);
> +        return -EINVAL;
> +    }
> +
> +    /* Performs a final check for a valid image size */
> +    if ( (startup_hdr->stored_size + startup_hdr->preboot_size) != 
> dom->kernel_size )

A suitably large stored_size or preboot_size will potentially overflow
the addition and the result could be arranged to be == kernel_size.

Since stored_size and preboot_size are 32- and 16-bit it is (I think)
sufficient to cast to a 64bit type for the addition. Perhaps one way
which is nice and clear in terms of reviewing for security would be 
        uint64_t total_size;
        
        ...
        
        total_size += startup_hdr->stored_size;
        total_size += startup_hdr->preboot_size;
        if ( total_size != dom->kernel_size )
                ...
                
BTW, you might want to check > dom->kernel_size to allow for smaller
images?

> +    {
> +        xc_dom_printf(dom->xch, "%s: QNX IFS has wrong 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) )

You haven't validated startup_size yet, so you can't trust it to not
overrun the buffer. And you need to be careful with that subtraction,
probably starting with validating that one is larger than the other.

You don't see to use preboot_size for anything, perhaps you meant to
range check startup_size above instead?

> +    {
> +        xc_dom_printf(dom->xch, "%s: QNX IFS has wrong checksum", 
> __FUNCTION__);
> +        return -EINVAL;
> +    }
> +
> +    /* Performs a sanity check for a valid startup entry point */
> +    v_end = v_start + dom->kernel_size;
> +    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;
> +    }

I think this check can also move to parse time. (It may not matter but
the other loaders don't rely on rambase_pfn in the probe function and
I'd like to be consistent if possible).

> +
> +    dom->startup_vaddr = startup_hdr->startup_vaddr;

and having move the above I think you can then just assign virt_entry
directly from startup_hdr in the parse function. Since you've validated
the header in the probe function parse can just use it.

> +static int xc_dom_parse_qnx_ifs(struct xc_dom_image *dom)
> +{
> +    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;
> +    v_end = v_start + dom->kernel_size;

[...] put the checks which I called about above here.

> +    /* find kernel segment */
> +    dom->kernel_seg.vstart = v_start;
> +    dom->kernel_seg.vend   = v_end;
> +
> +    dom->parms.virt_entry = dom->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;
> +}

[...]

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel