Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 1/1] of: unittest: overlay: ensure proper alignment of
 copied FDT
From:   Frank Rowand <frowand.list@gmail.com>
To:     Rob Herring <robh+dt@kernel.org>,
        Guenter Roeck <linux@roeck-us.net>
Cc:     Pantelis Antoniou <pantelis.antoniou@konsulko.com>,
        devicetree@vger.kernel.org,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        linux-kernel@vger.kernel.org
References: <20210407205110.2173976-1-frowand.list@gmail.com>
Message-ID: <0851cec1-99d1-7de0-81dc-2c82532dc6f7@gmail.com>
Date:   Wed, 7 Apr 2021 15:59:56 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20210407205110.2173976-1-frowand.list@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Guenter,

On 4/7/21 3:51 PM, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@sony.com>
> 
> The Devicetree standard specifies an 8 byte alignment of the FDT.
> Code in libfdt expects this alignment for an FDT image in memory.
> kmemdup() returns 4 byte alignment on openrisc.  Replace kmemdup()
> with kmalloc(), align pointer, memcpy() to get proper alignment.
> 
> The 4 byte alignment exposed a related bug which triggered a crash
> on openrisc with:
> commit 79edff12060f ("scripts/dtc: Update to upstream version v1.6.0-51-g183df9e9c2b9")
> as reported in:
> https://lore.kernel.org/lkml/20210327224116.69309-1-linux@roeck-us.net/

Can you please test this patch?

The most complete coverage will be a config with:

 OF_UNITTEST=y
 OF_OVERLAY=y
 OF_DYNAMIC=y

One path will be missed if you test with:

 #OF_OVERLAY=n
 #OF_DYNAMIC=n

Thanks!

-Frank

> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---
>  drivers/of/of_private.h | 2 ++
>  drivers/of/overlay.c    | 8 ++++++--
>  drivers/of/unittest.c   | 9 +++++++--
>  3 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index d9e6a324de0a..d717efbd637d 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -8,6 +8,8 @@
>   * Copyright (C) 1996-2005 Paul Mackerras.
>   */
>  
> +#define FDT_ALIGN_SIZE 8
> +
>  /**
>   * struct alias_prop - Alias property in 'aliases' node
>   * @link:	List node to link the structure in aliases_lookup list
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 50bbe0edf538..8b40711ed202 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -1014,7 +1014,7 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree,
>  int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
>  			 int *ovcs_id)
>  {
> -	const void *new_fdt;
> +	void *new_fdt;
>  	int ret;
>  	u32 size;
>  	struct device_node *overlay_root;
> @@ -1036,10 +1036,14 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
>  	 * Must create permanent copy of FDT because of_fdt_unflatten_tree()
>  	 * will create pointers to the passed in FDT in the unflattened tree.
>  	 */
> -	new_fdt = kmemdup(overlay_fdt, size, GFP_KERNEL);
> +	size += FDT_ALIGN_SIZE;
> +	new_fdt = kmalloc(size, GFP_KERNEL);
>  	if (!new_fdt)
>  		return -ENOMEM;
>  
> +	new_fdt = PTR_ALIGN(new_fdt, FDT_ALIGN_SIZE);
> +	memcpy(new_fdt, overlay_fdt, size);
> +
>  	of_fdt_unflatten_tree(new_fdt, NULL, &overlay_root);
>  	if (!overlay_root) {
>  		pr_err("unable to unflatten overlay_fdt\n");
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index eb100627c186..edd6ce807691 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -22,6 +22,7 @@
>  #include <linux/slab.h>
>  #include <linux/device.h>
>  #include <linux/platform_device.h>
> +#include <linux/kernel.h>
>  
>  #include <linux/i2c.h>
>  #include <linux/i2c-mux.h>
> @@ -1415,7 +1416,7 @@ static int __init unittest_data_add(void)
>  	 */
>  	extern uint8_t __dtb_testcases_begin[];
>  	extern uint8_t __dtb_testcases_end[];
> -	const int size = __dtb_testcases_end - __dtb_testcases_begin;
> +	u32 size = __dtb_testcases_end - __dtb_testcases_begin;
>  	int rc;
>  
>  	if (!size) {
> @@ -1425,10 +1426,14 @@ static int __init unittest_data_add(void)
>  	}
>  
>  	/* creating copy */
> -	unittest_data = kmemdup(__dtb_testcases_begin, size, GFP_KERNEL);
> +	size += FDT_ALIGN_SIZE;
> +	unittest_data = kmalloc(size, GFP_KERNEL);
>  	if (!unittest_data)
>  		return -ENOMEM;
>  
> +	unittest_data = PTR_ALIGN(unittest_data, FDT_ALIGN_SIZE);
> +	memcpy(unittest_data, __dtb_testcases_begin, size);
> +
>  	of_fdt_unflatten_tree(unittest_data, NULL, &unittest_data_node);
>  	if (!unittest_data_node) {
>  		pr_warn("%s: No tree to attach; not running tests\n", __func__);
>