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

[Oleksandr Tyshchenko <oleksandr.tyshchenko@xxxxxxxxxxxxxxx>]

Hi Ian

On Tue, Sep 23, 2014 at 6:53 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> 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?
This Change-Id for our local gerrit. I forgot to drop it.

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

>
>> +
>> +    /*
>> +     * 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 )
>                 ...
ok

>
> BTW, you might want to check > dom->kernel_size to allow for smaller
> images?
No, I would prefer the strong check:
if ( total_size != dom->kernel_size )
   ...

>
>> +    {
>> +        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.
ok

>
> You don't see to use preboot_size for anything, perhaps you meant to
> range check startup_size above instead?
Sorry I don't understand what do you mean.

>
>> +    {
>> +        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).
ok

>
>> +
>> +    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.
ok

>
>> +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.
ok

>
>> +    /* 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.
>



-- 

Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com

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