Date:   Mon, 29 Nov 2021 19:57:19 -0600
From:   Rob Herring <robh@kernel.org>
To:     Lizhi Hou <lizhi.hou@xilinx.com>
Cc:     linux-kernel@vger.kernel.org, linux-fpga@vger.kernel.org,
        maxz@xilinx.com, sonal.santan@xilinx.com, yliu@xilinx.com,
        michal.simek@xilinx.com, stefanos@xilinx.com,
        devicetree@vger.kernel.org, trix@redhat.com, mdf@kernel.org,
        dwmw2@infradead.org, Max Zhen <max.zhen@xilinx.com>
Subject: Re: [PATCH V2 XRT Alveo Infrastructure 3/9] of: handle fdt buffer
 alignment inside unflatten function
Message-ID: <YaWE/2ikgpXi2hzY@robh.at.kernel.org>
References: <20211119222412.1092763-1-lizhi.hou@xilinx.com>
 <20211119222412.1092763-4-lizhi.hou@xilinx.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211119222412.1092763-4-lizhi.hou@xilinx.com>
Precedence: bulk

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@xilinx.com>
> Signed-off-by: Max Zhen <max.zhen@xilinx.com>
> Signed-off-by: Lizhi Hou <lizhi.hou@xilinx.com>
> ---
>  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