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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 29 Jan 2015, Julien Grall wrote:
> 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.

you can use it to find if it is NULL terminated because it returns
maxlen in that case:

       The strnlen() function returns strlen(s), if that is less than maxlen, 
or maxlen if there is no null byte ('\0') among  the
       first maxlen bytes pointed to by s.


> 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.
 
I was just asking because you clearly didn't trust its output earlier
but you did in this case. That's OK for me.

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