Re: [PATCH v2] xen/tools: Introduce QNX IFS loader

[Ian Campbell <ian.campbell@citrix.com>]

On Wed, 2014-09-17 at 10:59 -0700, Julien Grall wrote:
> > +static int xc_dom_probe_qnx_ifs(struct xc_dom_image *dom)
> > +{
> > +    struct startup_header *startup_hdr;
> > +    uint32_t start_addr, end_addr;
> > +
> > +    if ( dom->kernel_blob == NULL )
> > +    {
> > +        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
> > +                     "%s: no QNX IFS loaded", __FUNCTION__);
> > +        return -EINVAL;
> > +    }
> > +
> > +    /* Scan 4KB boundaries for the valid OS signature */

Is this correct? You appear to be scanning at 4 byte boundaries over a
range of 4K.

> > +    start_addr = *(uint32_t *)&dom->kernel_blob;
> > +    end_addr = start_addr + 0x1000;
> 
> I took me a couple of minutes to understand where does the "0x1000" 
> comes from. I would use "4 << 10" here.

That's definitely not an improvement.

PAGE_SIZE might be.

The code also needs to take more care not to run off the end of the
kernel image, e.g. a maliciously short one, or one with a malicious
start_addr.

It also needs to not trust any of the values read from the header and
range check them all etc. The patches from XSA-95 have some examples of
the sorts of checks which are needed for this sort of thing, plus the
zImage loader generally ought to serve as an example.

Ian.