Re: [Xen-devel] [PATCH v3 21/24] tools/(lib)xl: Add partial device tree support for ARM

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Ian,

On 23/02/15 12:03, Ian Jackson wrote:
> Julien Grall writes ("[PATCH v3 21/24] tools/(lib)xl: Add partial device tree 
> support for ARM"):
>> Let the user to pass additional nodes to the guest device tree. For this
>> purpose, everything in the node /passthrough from the partial device tree \
> will
>> be copied into the guest device tree.
> 
> Please wrap your commit messages to 70, not 80.

I though commit message has to be wrapped to 80. I will change it.

> 
>> +=item B<device_tree=PATH>
>> +
>> +Specify a partial device tree (compiled via the Device Tree Compiler).
>> +Everything under the node "/passthrough" will be copied into the guest
>> +device tree. For convenience, the node "/aliases" is also copied to allow
>> +the user to defined aliases which can be used by the guest kernel.
> 
> This is rather odd.  The config option is `device_tree' but apparently
> it is only relevant for passthrough and nothing else can be set with
> it.

I had to chose a name for the node and "/passthrough" was the best one
and it won't collapse with the device tree generated by the toolstack.

Although, you can put pretty much everything in the "/passthrough" node.

>> +static int check_partial_fdt(libxl__gc *gc, void *fdt, size_t size)
>> +{
> ...
>> +    /* Check if the *size and off* fields doesn't overrun the totalsize
>> +     * of the partial FDT.
>> +     */
>> +    if (fdt_totalsize(fdt) > size) {
>> +        LOG(ERROR, "Partial FDT totalsize is too big");
>> +        return ERROR_FAIL;
>> +    }
> 
> There's lots and lots of this very fragile binary parsing code.
> 
> Is this facility supposed to take untrusted or partially-trusted
> partial device trees ?

It may take untrusted device tree. I review the libfdt code and try to
fix all possible security issue in the toolstack.

> If so then I suspect we need a different approach.  It might be easer
> to rewrite this whole functionality in a programming language which is
> less fragile in the face of programming errors, than to try to make
> this whole thing secure (and review it).
> 
> I'm definitely having XSA-55 flashbacks.

It's not my plan to have an XSA-55 like :).

As discussed IRL, we can mark this option "unsafe". So any device tree
pass to libxl should be trusted.

I will add an item in the description.

Regards,

-- 
Julien Grall

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