Re: [PATCH 5/6] xen/dt-overlay: support phandle-based targeting in overlay_get_nodes_info

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 16 Apr 2026, at 09:36, Orzel, Michal <Michal.Orzel@xxxxxxx> wrote:
> 
> 
> 
> On 16/04/2026 10:21, Luca Fancellu wrote:
>>>>> 
>>>>> diff --git a/xen/common/device-tree/dt-overlay.c 
>>>>> b/xen/common/device-tree/dt-overlay.c
>>>>> index d3d4669718ac..a0dee7edb7e5 100644
>>>>> --- a/xen/common/device-tree/dt-overlay.c
>>>>> +++ b/xen/common/device-tree/dt-overlay.c
>>>>> @@ -286,6 +286,63 @@ static unsigned int overlay_node_count(const void 
>>>>> *overlay_fdt)
>>>>>   return num_overlay_nodes;
>>>>> }
>>>>> 
>>>>> +/*
>>>>> + * Resolve the target path for an overlay fragment.
>>>>> + *
>>>>> + * This is called before fdt_overlay_apply(), so phandle-based targets
>>>>> + * (target = <&label>) are still unresolved (compiled as 0xffffffff by 
>>>>> DTC).
>>>>> + * Handle the two cases that actually occur:
>>>>> + *  - target-path property: the path string is used directly,
>>>>> + *  - target = <&label>: the label is looked up in the overlay's 
>>>>> __fixups__
>>>>> + *    node, then resolved to a path via the base DTB's __symbols__ node.
>>>>> + *
>>>>> + * Returns a pointer into the FDT on success, NULL on failure.
>>>>> + */
>>>>> +static const char *overlay_get_target_path(const void *fdt, const void 
>>>>> *fdto,
>>>>> +                                           int fragment)
>>>>> +{
>>>>> +    const char *path, *fragment_name;
>>>>> +    int fixups_off, symbols_off, property;
>>>>> +    int fragment_name_len;
>>>>> +
>>>>> +    /* Try target-path first (string-based targeting) */
>>>>> +    path = fdt_getprop(fdto, fragment, "target-path", NULL);
>>>>> +    if ( path )
>>>>> +        return path;
>>>>> +
>>>>> +    /* Phandle-based target: resolve via __fixups__ and __symbols__ */
>>>>> +    fixups_off = fdt_path_offset(fdto, "/__fixups__");
>>>>> +    if ( fixups_off < 0 )
>>>>> +        return NULL;
>>>>> +
>>>>> +    symbols_off = fdt_path_offset(fdt, "/__symbols__");
>>>>> +    if ( symbols_off < 0 )
>>>>> +        return NULL;
>>>>> +
>>>>> +    fragment_name = fdt_get_name(fdto, fragment, &fragment_name_len);
>>>>> +    if ( !fragment_name )
>>>>> +        return NULL;
>>>>> +
>>>>> +    fdt_for_each_property_offset(property, fdto, fixups_off)
>>>>> +    {
>>>>> +        const char *val, *label, *p;
>>>>> +        int val_len;
>>>>> +
>>>>> +        val = fdt_getprop_by_offset(fdto, property, &label, &val_len);
>>>>> +        if ( !val )
>>>>> +            continue;
>>>>> +
>>>>> +        /* Match entries of the form "/<fragment_name>:target:0" */
>>>>> +        for ( p = val; p < (val + val_len); p += (strlen(p) + 1) )
>>>> 
>>>> what guarantees us that p will be null terminated, if a malformed overlay
>>>> is passed this strlen can read past the string, we can use strnlen having 
>>>> as
>>>> upper bound a counter=val_len? decreasing counter each iteration.
>>>> 
>>>> Or do you think it can never happen?
>>> In theory it can happen, in practice this is something not usually taken 
>>> into
>>> account. But we can surely stay on the defensive side and do a single check
>>> right after fdt_getprop_by_offset to catch not-NUL terminated stringlist:
>>> if ( !val || !val_len || val[val_len - 1] != '\0' )
>> 
>> Ok, do we also want to print a warning in case of malformed properties 
>> instead
>> of ignoring and continue?
> That's really a matter of taste. I would not do it unless you really want to.

I’ll leave this to the other maintainer, with the fix above:

Reviewed-by: Luca Fancellu <luca.fancellu@xxxxxxx>

Cheers,
Luca