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

[Oleksandr Tyshchenko <oleksandr.tyshchenko@xxxxxxxxxxxxxxx>]

Hi Ian

On Thu, Sep 18, 2014 at 4:50 AM, Ian Campbell <ian.campbell@xxxxxxxxxx> wrote:
> 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.
ok.
I meant that the code below looks on 4 KB boundaries for the image
identifier bytes.

>
>> > +    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.
ok, but this does not correlate to a page meaning, just identical sizes

I would like to say a bit more about this scanning procedure:
We need to scan because we have N raw bytes (preboot_size) from the
beginning of the image to the startup header,
where "startup_vaddr" is located (we have to obtain this entry
address). Image starts on 4 byte boundary.
This "preboot_size" value depends on how the IFS is created
(attributes in buildfile). The image could or couldn't have these N
raw bytes.
In our case we have only 8 raw bytes with next attributes:
[virtual=armle-v7,raw +compress]
Although I set ranges to 4 KB as it was mentioned in howto, I do not
think that "preboot_size" can be comparable with such size.
I think that value caused by assumption that this can be multiple OS
images (so, the image may have many headers).

>
> 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.
ok, I will add a check

>
> 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.
ok, I will think about it

>
> Ian.
>

-- 

Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com

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