Re: [PATCH V2 XRT Alveo Infrastructure 3/9] of: handle fdt buffer alignment inside unflatten function

[Rob Herring]

On Fri, Nov 19, 2021 at 02:24:06PM -0800, Lizhi Hou wrote:
> Add alignment check to of_fdt_unflatten_tree(). If it is not aligned,
> allocate a aligned buffer and copy the fdt blob. So the caller does not
> have to deal with the buffer alignment before calling this function.
> XRT uses this function to unflatten fdt which is from Alveo firmware.
> 
> Signed-off-by: Sonal Santan <sonal.santan@xxxxxxxxxx>
> Signed-off-by: Max Zhen <max.zhen@xxxxxxxxxx>
> Signed-off-by: Lizhi Hou <lizhi.hou@xxxxxxxxxx>
> ---
>  drivers/of/fdt.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 4546572af24b..d64445e43ceb 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -455,13 +455,28 @@ void *of_fdt_unflatten_tree(const unsigned long *blob,
>  			    struct device_node *dad,
>  			    struct device_node **mynodes)
>  {
> +	void *new_fdt = NULL, *fdt_align;
>  	void *mem;
>  
> +	if (fdt_check_header(blob)) {
> +		pr_err("Invalid fdt blob\n");
> +		return NULL;
> +	}
> +	fdt_align = (void *)PTR_ALIGN(blob, FDT_ALIGN_SIZE);
> +	if (fdt_align != blob) {
> +		new_fdt = kmalloc(fdt_totalsize(blob) + FDT_ALIGN_SIZE, GFP_KERNEL);
> +		if (!new_fdt)
> +			return NULL;
> +		fdt_align = PTR_ALIGN(new_fdt, FDT_ALIGN_SIZE);

Where's the copy?

> +	}
> +
>  	mutex_lock(&of_fdt_unflatten_mutex);
> -	mem = __unflatten_device_tree(blob, dad, mynodes, &kernel_tree_alloc,
> +	mem = __unflatten_device_tree(fdt_align, dad, mynodes, &kernel_tree_alloc,
>  				      true);
>  	mutex_unlock(&of_fdt_unflatten_mutex);
>  
> +	kfree(new_fdt);

You know the unflattened DT just references strings and property values 
from the flattened DT. So you just caused a use after free.

Fix your firmware to align the DT.

Rob