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

[Julien Grall <julien.grall@linaro.org>]

On 29/01/15 11:03, Stefano Stabellini wrote:
> On Tue, 13 Jan 2015, Julien Grall wrote:
>> 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.
>>
>> The node /aliases will be also copied to allow the user to define aliases
>> which can be used by the guest kernel.
>>
>> A simple partial device tree will look like:
>>
>> /dts-v1/;
>>
>> / {
>>         #address-cells = <2>;
>>         #size-cells = <2>;
>>
>>         passthrough {
>>             compatible = "simple-bus";
>>             ranges;
>>             #address-cells = <2>;
>>             #size-cells = <2>;
>>
>>             /* List of your nodes */
>>         }
>> };
> 
> It would be nice to have an example of this under tools/examples.

Ok. I will add one.

[..]

>> +/*
>> + * Check if a string stored the strings block section is correctly
>> + * nul-terminated.
>> + * off_dt_strings and size_dt_strings fields have been validity-check
>> + * earlier, so it's safe to use them here.
>> + */
>> +static bool check_string(void *fdt, int nameoffset)
>> +{
>> +    const char *str = fdt_string(fdt, nameoffset);
>> +
>> +    for (; nameoffset < fdt_size_dt_strings(fdt); nameoffset++, str++) {
>> +        if (*str == '\0')
>> +            return true;
>> +    }
>> +
>> +    return false;
>> +}
> 
> strnlen?

I could but it would not tell us directly if the string is NULL
terminated or not.

What about memchr?


[..]

>> +static int copy_node_by_path(libxl__gc *gc, const char *path,
>> +                             void *fdt, void *pfdt)
>> +{
>> +    int nodeoff, r;
>> +    const char *name = strrchr(path, '/');
>> +
>> +    if (!name)
>> +        return -FDT_ERR_INTERNAL;
>> +
>> +    name++;
>> +
>> +    /* The FDT function to look at node doesn't take into account the
>> +     * unit (i.e anything after @) when search by name. Check if the
>> +     * name exactly match.
>> +     */
>> +    nodeoff = fdt_path_offset(pfdt, path);
>> +    if (nodeoff < 0)
>> +        return nodeoff;
>> +
>> +    if (strcmp(fdt_get_name(pfdt, nodeoff, NULL), name))
>> +        return -FDT_ERR_NOTFOUND;
> 
> Are we sure that the string returned by fdt_get_name is NULL terminated?

Yes, libfdt does some sanity check on it (see fdt_next_tag case
FDT_BEGIN_NODE).

I tried to fix all the possible security flaw in libfdt (and there is
quite a lot). If we don't trust the rest of libfdt, then we have to
import our own and fix it.

Regards,

-- 
Julien Grall