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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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.

> +=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.

> +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 ?

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.

Ian.

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