From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Campbell <ian.campbell@citrix.com>
Subject: Re: [PATCH v2] xen/tools: Introduce QNX IFS loader
Date: Thu, 18 Sep 2014 02:50:21 +0100
Message-ID: <1411005021.1920.9.camel@citrix.com>
References: <1410971686-28977-1-git-send-email-oleksandr.tyshchenko@globallogic.com>
	<1410971686-28977-2-git-send-email-oleksandr.tyshchenko@globallogic.com>
	<5419CBE5.2030205@linaro.org>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <5419CBE5.2030205@linaro.org>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Julien Grall <julien.grall@linaro.org>
Cc: Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>, stefano.stabellini@eu.citrix.com, tim@xen.org, xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

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.