[PATCH v3] xen/tools: Introduce QNX IFS loader

[PATCH v3] xen/tools: Introduce QNX IFS loader

[Oleksandr Tyshchenko]

The following patch adds ability to load OS image filesystem.
The patch was developed according to instruction:
http://www.qnx.com/developers/docs/6.4.1/neutrino/building/load_process.html
and tested on omap5_uevm/dra7xx_evm boards with compressed/uncompressed images
on top of XEN 4.4/4.5.
The image was downloaded from:
http://community.qnx.com/sf/wiki/do/viewPage/projects.bsp/wiki/TexasInstrumentsOMAP5432EVM
and modified to run on top of XEN.

Changes in v02:
- Rewrite code and changed license to LGPL
- Addressed Julien's technical review comments
- Added image validatation step by performing a checksum calculation

Changes in v03:
- Add some range checks according to the XSA-95
- Add a check for a valid load address
- Drop searching stuff
- Addressed other Julien's and Ian's comments

Oleksandr Tyshchenko (1):
  xen/tools: Introduce QNX IFS loader

 tools/libxc/Makefile              |   1 +
 tools/libxc/xc_dom.h              |   3 +
 tools/libxc/xc_dom_qnxifsloader.c | 215 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 219 insertions(+)
 create mode 100644 tools/libxc/xc_dom_qnxifsloader.c

-- 
1.9.1

[PATCH v3] xen/tools: Introduce QNX IFS loader

[Oleksandr Tyshchenko]

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.

Change-Id: Id125d6eae5847698fb5734c7f28451f5b752062f
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.h              |   3 +
 tools/libxc/xc_dom_qnxifsloader.c | 215 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 219 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.h b/tools/libxc/xc_dom.h
index 92db8d0..bf9ea3c 100644
--- a/tools/libxc/xc_dom.h
+++ b/tools/libxc/xc_dom.h
@@ -151,6 +151,9 @@ struct xc_dom_image {
     struct xc_dom_arch *arch_hooks;
     /* allocate up to virt_alloc_end */
     int (*allocate) (struct xc_dom_image * dom, xen_vaddr_t up_to);
+
+    /* qnx loader */
+    uint32_t startup_vaddr;
 };
 
 /* --- pluggable kernel loader ------------------------------------- */
diff --git a/tools/libxc/xc_dom_qnxifsloader.c b/tools/libxc/xc_dom_qnxifsloader.c
new file mode 100644
index 0000000..6987b4f
--- /dev/null
+++ b/tools/libxc/xc_dom_qnxifsloader.c
@@ -0,0 +1,215 @@
+/*
+ * 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;
+    uint64_t v_start, v_end;
+    uint64_t rambase = dom->rambase_pfn << XC_PAGE_SHIFT;
+
+    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;
+    }
+
+    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;
+    }
+
+    /*
+     * 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 )
+    {
+        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) )
+    {
+        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;
+    }
+
+    dom->startup_vaddr = startup_hdr->startup_vaddr;
+
+    return 0;
+}
+
+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;
+
+    /* 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;
+}
+
+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

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

[Ian Campbell]

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.

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

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH v3] xen/tools: Introduce QNX IFS loader"):
> 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 
...               
> BTW, you might want to check > dom->kernel_size to allow for smaller
> images?
...
> 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.

These would all have been security bugs if the v3 patch had been
accepted.  They would have been bugs that would potentially amount to
privilege escalation for very many Xen installations.

I think we should be considering whether to take an approach similar
to that taken in libelf after XSA-55.  The code can probably be
reused.

Ian.

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

[Ian Campbell]

On Tue, 2014-09-23 at 17:19 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH v3] xen/tools: Introduce QNX IFS loader"):
> > 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 
> ...               
> > BTW, you might want to check > dom->kernel_size to allow for smaller
> > images?
> ...
> > 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.
> 
> These would all have been security bugs if the v3 patch had been
> accepted.  They would have been bugs that would potentially amount to
> privilege escalation for very many Xen installations.

Well, those booting untrusted QNX guests on ARM, which won't be many
yet, but point taken...

> I think we should be considering whether to take an approach similar
> to that taken in libelf after XSA-55.  The code can probably be
> reused.

I think something like that would be good, but would be a much bigger
yakk than we can reasonably ask to be shaved here, since it would need
to transition the core xc_dom builder code and all of the loaders for
both ARM and x86.

And its certainly not 4.5 material at this point.

Ian.

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

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH v3] xen/tools: Introduce QNX IFS loader"):
> On Tue, 2014-09-23 at 17:19 +0100, Ian Jackson wrote:
> > These would all have been security bugs if the v3 patch had been
> > accepted.  They would have been bugs that would potentially amount to
> > privilege escalation for very many Xen installations.
> 
> Well, those booting untrusted QNX guests on ARM, which won't be many
> yet, but point taken...

No.  The loader would run whenever it seems the appropriate image
type, so everyone with it compiled in is vulnerable.

Admittedly you are right that this is only ARM users.

> > I think we should be considering whether to take an approach similar
> > to that taken in libelf after XSA-55.  The code can probably be
> > reused.
> 
> I think something like that would be good, but would be a much bigger
> yakk than we can reasonably ask to be shaved here, since it would need
> to transition the core xc_dom builder code and all of the loaders for
> both ARM and x86.
> 
> And its certainly not 4.5 material at this point.

In that case this code needs a very thorough review process.

I suggest the following approach: the submitters conduct a very
serious and thorough security review.  When they are happy that they
have a bug-free submission, they send it with at least an ack from a
colleague.

I will then review it in detail looking for security bug.  If I find
even one the whole patch will be rejected for 4.5 and we will look at
the more substantial approach for post-4.5.

This may sound harsh, but security review of this kind of code is very
difficult work and not particularly reliable at finding bugs.  A
system where the patch is simply resubmitted, after fixing those bugs
found by the first security review, will probably result in
undiscovered bugs being accepted.

Ian.

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

[Oleksandr Tyshchenko]

Hi Ian

On Tue, Sep 23, 2014 at 8:00 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH v3] xen/tools: Introduce QNX IFS loader"):
>> On Tue, 2014-09-23 at 17:19 +0100, Ian Jackson wrote:
>> > These would all have been security bugs if the v3 patch had been
>> > accepted.  They would have been bugs that would potentially amount to
>> > privilege escalation for very many Xen installations.
>>
>> Well, those booting untrusted QNX guests on ARM, which won't be many
>> yet, but point taken...
>
> No.  The loader would run whenever it seems the appropriate image
> type, so everyone with it compiled in is vulnerable.
>
> Admittedly you are right that this is only ARM users.
>
>> > I think we should be considering whether to take an approach similar
>> > to that taken in libelf after XSA-55.  The code can probably be
>> > reused.
>>
>> I think something like that would be good, but would be a much bigger
>> yakk than we can reasonably ask to be shaved here, since it would need
>> to transition the core xc_dom builder code and all of the loaders for
>> both ARM and x86.
>>
>> And its certainly not 4.5 material at this point.
>
> In that case this code needs a very thorough review process.
>
> I suggest the following approach: the submitters conduct a very
> serious and thorough security review.  When they are happy that they
> have a bug-free submission, they send it with at least an ack from a
> colleague.
>
> I will then review it in detail looking for security bug.  If I find
> even one the whole patch will be rejected for 4.5 and we will look at
> the more substantial approach for post-4.5.
>
> This may sound harsh, but security review of this kind of code is very
> difficult work and not particularly reliable at finding bugs.  A
> system where the patch is simply resubmitted, after fixing those bugs
> found by the first security review, will probably result in
> undiscovered bugs being accepted.
>
> Ian.

Thank you for your comment. I got it.

-- 

Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com

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

[Ian Campbell]

On Tue, 2014-09-23 at 18:00 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH v3] xen/tools: Introduce QNX IFS loader"):
> > On Tue, 2014-09-23 at 17:19 +0100, Ian Jackson wrote:
> > > These would all have been security bugs if the v3 patch had been
> > > accepted.  They would have been bugs that would potentially amount to
> > > privilege escalation for very many Xen installations.
> > 
> > Well, those booting untrusted QNX guests on ARM, which won't be many
> > yet, but point taken...
> 
> No.  The loader would run whenever it seems the appropriate image
> type, so everyone with it compiled in is vulnerable.

Yes, I realised this during dinner.

Ian.

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

[Oleksandr Tyshchenko]

Hi Ian

On Tue, Sep 23, 2014 at 6:53 PM, Ian Campbell <Ian.Campbell@citrix.com> 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

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

[Ian Campbell]

On Tue, 2014-09-23 at 19:24 +0300, Oleksandr Tyshchenko wrote:

> > 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 )
>    ...

OK, assuming you are sure that those two things always add up to the
total (what about startup_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.

I mean that you validate preboot_size but then don't use it, while you
use startup_size without validating it. I was wondering if perhaps you
were accidentally checking the wrong one.

Ian.

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

[Oleksandr Tyshchenko]

On Tue, Sep 23, 2014 at 7:37 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Tue, 2014-09-23 at 19:24 +0300, Oleksandr Tyshchenko wrote:
>
>> > 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 )
>>    ...
>
> OK, assuming you are sure that those two things always add up to the
> total (what about startup_size?)
I only know that startup_size < total_size, I can add this check

>
>> >
>> >> +    {
>> >> +        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.
>
> I mean that you validate preboot_size but then don't use it, while you
> use startup_size without validating it. I was wondering if perhaps you
> were accidentally checking the wrong one.
I will add additional checks for valid startup_size.
But I would like to check that preboot_size = 0 (since we are using binary.boot)

So our checks for valid size:
1. total_size = kernel_size
2. startup_size < total_size
3. header_size = sizeof(struct startup_header);


>
> Ian.
>



-- 

Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com