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

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 15/04/2026 17:36, Luca Fancellu wrote:
> Hi Michal,
> 
>> On 15 Apr 2026, at 12:36, Michal Orzel <michal.orzel@xxxxxxx> wrote:
>>
>> overlay_get_nodes_info() is called before fdt_overlay_apply() to extract
>> target paths from the overlay. This fails for overlays using phandle-based
>> targeting (target = <&label>) because DTC compiles these as unresolved
>> fixups (target = <0xffffffff>), causing fdt_overlay_target_offset() to
>> return -FDT_ERR_BADPHANDLE. Prior to this change users were forced to
>> manually modify the dtbo (even for hwdom) to switch from target to
>> target-phandle by manually inspecting also the host DTB.
>>
>> Introduce overlay_get_target_path() which directly handles the two
>> targeting cases that occur before fixup resolution:
>> - target-path: the string property is returned directly.
>> - target = <&label>: the label is found in the overlay's __fixups__
>>   node, then resolved to a path via the base DTB's __symbols__ node.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
>> ---
>> xen/common/device-tree/dt-overlay.c | 65 ++++++++++++++++++++++++++---
>> 1 file changed, 59 insertions(+), 6 deletions(-)
>>
>> 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' )

In case of no other remarks, I'd do that on commit.

~Michal