Re: [PATCH 1/1] of: properly check for error returned by fdt_get_name()

[Guenter Roeck <linux@roeck-us.net>]

On Sun, Apr 04, 2021 at 10:37:35PM -0500, Frank Rowand wrote:
> Hi Guenter,
> 
> Can you please test this patch to see if it prevents the crash on
> openrisc that you reported in
> https://lore.kernel.org/lkml/20210327224116.69309-1-linux@roeck-us.net/
> 
> Just after start of unittest you should see a warning about
> testcases.
> 

Trying again:

With this patch applied, the kernel no longer crashes, and the log message
is as expected:

### dt-test ### start of unittest - you will see error messages
### dt-test ### unittest_data_add: unflatten testcases tree failed

Tested-by: Guenter Roeck <linux@roeck-us.net>

> Thanks,
> 
> Frank
> 
> 
> On 4/4/21 10:28 PM, frowand.list@gmail.com wrote:
> > From: Frank Rowand <frank.rowand@sony.com>
> > 
> > fdt_get_name() returns error values via a parameter pointer
> > instead of in function return.  Fix check for this error value
> > in populate_node() and callers of populate_node().
> > 
> > Chasing up the caller tree showed callers of various functions
> > failing to initialize the value of pointer parameters that
> > can return error values.  Initialize those values to NULL.
> > 
> > The bug was introduced by
> > commit e6a6928c3ea1 ("of/fdt: Convert FDT functions to use libfdt")
> > but this patch can not be backported directly to that commit
> > because the relevant code has further been restructured by
> > commit dfbd4c6eff35 ("drivers/of: Split unflatten_dt_node()")
> > 
> > The bug became visible by triggering 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/
> > 
> > Fixes: commit 79edff12060f ("scripts/dtc: Update to upstream version v1.6.0-51-g183df9e9c2b9")
> > Reported-by: Guenter Roeck <linux@roeck-us.net>
> > Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> > 
> > ---
> > 
> > This patch papers over the unaligned pointer passed to
> > of_fdt_unflatten_tree() bug that Guenter reported in
> > https://lore.kernel.org/lkml/20210327224116.69309-1-linux@roeck-us.net/
> > 
> > I will create a separate patch to fix that problem.
> > 
> >  drivers/of/fdt.c      | 36 +++++++++++++++++++++++-------------
> >  drivers/of/overlay.c  |  2 +-
> >  drivers/of/unittest.c | 15 ++++++++++-----
> >  3 files changed, 34 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index dcc1dd96911a..adb26aff481d 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -205,7 +205,7 @@ static void populate_properties(const void *blob,
> >  		*pprev = NULL;
> >  }
> >  
> > -static bool populate_node(const void *blob,
> > +static int populate_node(const void *blob,
> >  			  int offset,
> >  			  void **mem,
> >  			  struct device_node *dad,
> > @@ -214,24 +214,24 @@ static bool populate_node(const void *blob,
> >  {
> >  	struct device_node *np;
> >  	const char *pathp;
> > -	unsigned int l, allocl;
> > +	int len;
> >  
> > -	pathp = fdt_get_name(blob, offset, &l);
> > +	pathp = fdt_get_name(blob, offset, &len);
> >  	if (!pathp) {
> >  		*pnp = NULL;
> > -		return false;
> > +		return len;
> >  	}
> >  
> > -	allocl = ++l;
> > +	len++;
> >  
> > -	np = unflatten_dt_alloc(mem, sizeof(struct device_node) + allocl,
> > +	np = unflatten_dt_alloc(mem, sizeof(struct device_node) + len,
> >  				__alignof__(struct device_node));
> >  	if (!dryrun) {
> >  		char *fn;
> >  		of_node_init(np);
> >  		np->full_name = fn = ((char *)np) + sizeof(*np);
> >  
> > -		memcpy(fn, pathp, l);
> > +		memcpy(fn, pathp, len);
> >  
> >  		if (dad != NULL) {
> >  			np->parent = dad;
> > @@ -295,6 +295,7 @@ static int unflatten_dt_nodes(const void *blob,
> >  	struct device_node *nps[FDT_MAX_DEPTH];
> >  	void *base = mem;
> >  	bool dryrun = !base;
> > +	int ret;
> >  
> >  	if (nodepp)
> >  		*nodepp = NULL;
> > @@ -322,9 +323,10 @@ static int unflatten_dt_nodes(const void *blob,
> >  		    !of_fdt_device_is_available(blob, offset))
> >  			continue;
> >  
> > -		if (!populate_node(blob, offset, &mem, nps[depth],
> > -				   &nps[depth+1], dryrun))
> > -			return mem - base;
> > +		ret = populate_node(blob, offset, &mem, nps[depth],
> > +				   &nps[depth+1], dryrun);
> > +		if (ret < 0)
> > +			return ret;
> >  
> >  		if (!dryrun && nodepp && !*nodepp)
> >  			*nodepp = nps[depth+1];
> > @@ -372,6 +374,10 @@ void *__unflatten_device_tree(const void *blob,
> >  {
> >  	int size;
> >  	void *mem;
> > +	int ret;
> > +
> > +	if (mynodes)
> > +		*mynodes = NULL;
> >  
> >  	pr_debug(" -> unflatten_device_tree()\n");
> >  
> > @@ -392,7 +398,7 @@ void *__unflatten_device_tree(const void *blob,
> >  
> >  	/* First pass, scan for size */
> >  	size = unflatten_dt_nodes(blob, NULL, dad, NULL);
> > -	if (size < 0)
> > +	if (size <= 0)
> >  		return NULL;
> >  
> >  	size = ALIGN(size, 4);
> > @@ -410,12 +416,16 @@ void *__unflatten_device_tree(const void *blob,
> >  	pr_debug("  unflattening %p...\n", mem);
> >  
> >  	/* Second pass, do actual unflattening */
> > -	unflatten_dt_nodes(blob, mem, dad, mynodes);
> > +	ret = unflatten_dt_nodes(blob, mem, dad, mynodes);
> > +
> >  	if (be32_to_cpup(mem + size) != 0xdeadbeef)
> >  		pr_warn("End of tree marker overwritten: %08x\n",
> >  			be32_to_cpup(mem + size));
> >  
> > -	if (detached && mynodes) {
> > +	if (ret <= 0)
> > +		return NULL;
> > +
> > +	if (detached && mynodes && *mynodes) {
> >  		of_node_set_flag(*mynodes, OF_DETACHED);
> >  		pr_debug("unflattened tree is detached\n");
> >  	}
> > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> > index 50bbe0edf538..e12c643b6ba8 100644
> > --- a/drivers/of/overlay.c
> > +++ b/drivers/of/overlay.c
> > @@ -1017,7 +1017,7 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
> >  	const void *new_fdt;
> >  	int ret;
> >  	u32 size;
> > -	struct device_node *overlay_root;
> > +	struct device_node *overlay_root = NULL;
> >  
> >  	*ovcs_id = 0;
> >  	ret = 0;
> > diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> > index eb100627c186..f9b5b698249f 100644
> > --- a/drivers/of/unittest.c
> > +++ b/drivers/of/unittest.c
> > @@ -1408,7 +1408,7 @@ static void attach_node_and_children(struct device_node *np)
> >  static int __init unittest_data_add(void)
> >  {
> >  	void *unittest_data;
> > -	struct device_node *unittest_data_node, *np;
> > +	struct device_node *unittest_data_node = NULL, *np;
> >  	/*
> >  	 * __dtb_testcases_begin[] and __dtb_testcases_end[] are magically
> >  	 * created by cmd_dt_S_dtb in scripts/Makefile.lib
> > @@ -1417,10 +1417,10 @@ static int __init unittest_data_add(void)
> >  	extern uint8_t __dtb_testcases_end[];
> >  	const int size = __dtb_testcases_end - __dtb_testcases_begin;
> >  	int rc;
> > +	void *ret;
> >  
> >  	if (!size) {
> > -		pr_warn("%s: No testcase data to attach; not running tests\n",
> > -			__func__);
> > +		pr_warn("%s: testcases is empty\n", __func__);
> >  		return -ENODATA;
> >  	}
> >  
> > @@ -1429,9 +1429,14 @@ static int __init unittest_data_add(void)
> >  	if (!unittest_data)
> >  		return -ENOMEM;
> >  
> > -	of_fdt_unflatten_tree(unittest_data, NULL, &unittest_data_node);
> > +	ret = of_fdt_unflatten_tree(unittest_data, NULL, &unittest_data_node);
> > +	if (!ret) {
> > +		pr_warn("%s: unflatten testcases tree failed\n", __func__);
> > +		kfree(unittest_data);
> > +		return -ENODATA;
> > +	}
> >  	if (!unittest_data_node) {
> > -		pr_warn("%s: No tree to attach; not running tests\n", __func__);
> > +		pr_warn("%s: testcases tree is empty\n", __func__);
> >  		kfree(unittest_data);
> >  		return -ENODATA;
> >  	}
> > 
>