diff mbox series

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

Message ID 20210405032845.1942533-1-frowand.list@gmail.com
State New
Headers show
Series [1/1] of: properly check for error returned by fdt_get_name() | expand

Commit Message

Frank Rowand April 5, 2021, 3:28 a.m. UTC 
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(-)

Comments

Frank Rowand April 5, 2021, 3:37 a.m. UTC | #1 
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.

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;
>  	}
>
Guenter Roeck April 5, 2021, 11:03 p.m. UTC | #2 
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;
> >  	}
> > 
>
Rob Herring (Arm) April 6, 2021, 7:21 p.m. UTC | #3 
On Sun, Apr 04, 2021 at 10:28:45PM -0500, 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.


Got an ETA for that?

Rob
Rob Herring (Arm) April 6, 2021, 7:21 p.m. UTC | #4 
On Sun, 04 Apr 2021 22:28:45 -0500, 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(-)

> 


Applied, thanks!
Frank Rowand April 6, 2021, 8:30 p.m. UTC | #5 
On 4/6/21 2:21 PM, Rob Herring wrote:
> On Sun, Apr 04, 2021 at 10:28:45PM -0500, 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.


Likely to be tomorrow (Wed 4/7).

-Frank

> 

> Got an ETA for that?

> 

> Rob

> .

>
Frank Rowand April 7, 2021, 9:04 p.m. UTC | #6 
On 4/6/21 3:30 PM, Frank Rowand wrote:
> On 4/6/21 2:21 PM, Rob Herring wrote:

>> On Sun, Apr 04, 2021 at 10:28:45PM -0500, 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.

> 

> Likely to be tomorrow (Wed 4/7).

> 

> -Frank

> 

>>

>> Got an ETA for that?

>>

>> Rob

>> .

>>

> 


The patch to fix the alignment issue is:

  https://lore.kernel.org/linux-devicetree/20210407205110.2173976-1-frowand.list@gmail.com/

Hopefully it will pass testing by Guenter.
  
Sorry about the previous vertical dyslexia response... :-)

-Frank
diff mbox series

Patch

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;
 	}