Re: [PATCH v5 05/25] xen/arm: check for multiboot nodes only under /chosen

[Julien Grall <julien.grall@arm.com>]

Hi Stefano,

On 10/23/18 3:02 AM, Stefano Stabellini wrote:
> Make sure to only look for multiboot compatible nodes only under
> /chosen, not under any other paths (depth <= 3).
> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> 
> ---
> 
> Changes in v5:
> - add patch
> - add check on return value of fdt_get_path
> ---
>   xen/arch/arm/bootfdt.c | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 8eba42c..a314aca 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -173,7 +173,14 @@ static void __init process_multiboot_node(const void *fdt, int node,
>       bootmodule_kind kind;
>       paddr_t start, size;
>       const char *cmdline;
> -    int len;
> +    int len = sizeof("/chosen");

NIT, I am not convince you win anything with that trick. strlen could be 
optimized by the compiler (we use __builtin_strlen on Arm64).

> +    char path[8]; /* sizeof "/chosen" */
> +    int ret;
> +
> +    /* Check that the node is under "chosen" */
> +    ret = fdt_get_path(fdt, node, path, len);
> +    if ( ret == 0 && strncmp(path, "/chosen", len - 1) )
> +        return;

This is quite odd. You continue if fdt_get_path() returns an error. 
Shouldn't it be:

ret || strncmp(path, "/chosen", strlen("/chosen")

>   
>       prop = fdt_get_property(fdt, node, "reg", &len);
>       if ( !prop )
> @@ -286,8 +293,8 @@ static int __init early_scan_node(const void *fdt,
>   {
>       if ( device_tree_node_matches(fdt, node, "memory") )
>           process_memory_node(fdt, node, name, address_cells, size_cells);
> -    else if ( device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ||
> -              device_tree_node_compatible(fdt, node, "multiboot,module" ))
> +    else if ( depth <= 3 && (device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ||
> +              device_tree_node_compatible(fdt, node, "multiboot,module" )))
>           process_multiboot_node(fdt, node, name, address_cells, size_cells);
>       else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") )
>           process_chosen_node(fdt, node, name, address_cells, size_cells);
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel