[PATCH 0/2] of: overlay: avoid potential null pointer exception

[PATCH 0/2] of: overlay: avoid potential null pointer exception

[Michael Riesch]

Hi all,

The function of_overlay_fix_path returns NULL in certain error cases but
of_overlay_apply_symbols (which is the only caller) does not check the
return value. For broken overlays this may result in a null pointer
exception. Fix this by checking the return value and inform the user
what exactly went wrong. To this end, improve the error handling in
of_overlay_apply_tree.

The thread [0] gives a bit more context.

Best regards,
Michael

[0] https://lore.barebox.org/barebox/95ff064f-aa11-c1ce-9d41-e38f2040c565@wolfvision.net/T/#u

Michael Riesch (2):
  of: overlay: improve error handling in of_overlay_apply_tree
  of: overlay: avoid potential null pointer exception

 drivers/of/overlay.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

-- 
2.30.2

[PATCH 1/2] of: overlay: improve error handling in of_overlay_apply_tree

[Michael Riesch]

Propagate any error from of_overlay_apply_symbols and let the user
know if the provided overlay is not applicable.

Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
---
 drivers/of/overlay.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 20a43f5170..20686db511 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -115,8 +115,8 @@ static char *of_overlay_fix_path(struct device_node *root,
 	return basprintf("%s%s", target->full_name, path_tail);
 }
 
-static void of_overlay_apply_symbols(struct device_node *root,
-				     struct device_node *overlay)
+static int of_overlay_apply_symbols(struct device_node *root,
+				    struct device_node *overlay)
 {
 	const char *old_path;
 	char *new_path;
@@ -129,12 +129,12 @@ static void of_overlay_apply_symbols(struct device_node *root,
 
 	if (!overlay_symbols) {
 		pr_debug("overlay doesn't have a __symbols__ node\n");
-		return;
+		return -EINVAL;
 	}
 
 	if (!root_symbols) {
 		pr_info("root doesn't have a __symbols__ node\n");
-		return;
+		return -EINVAL;
 	}
 
 	list_for_each_entry(prop, &overlay_symbols->properties, list) {
@@ -148,6 +148,8 @@ static void of_overlay_apply_symbols(struct device_node *root,
 			 prop->name, new_path);
 		of_property_write_string(root_symbols, prop->name, new_path);
 	}
+
+	return 0;
 }
 
 static int of_overlay_apply_fragment(struct device_node *root,
@@ -190,7 +192,9 @@ int of_overlay_apply_tree(struct device_node *root,
 		goto out_err;
 
 	/* Copy symbols from resolved overlay to base device tree */
-	of_overlay_apply_symbols(root, resolved);
+	err = of_overlay_apply_symbols(root, resolved);
+	if (err)
+		goto out_err;
 
 	/* Copy nodes and properties from resolved overlay to root */
 	for_each_child_of_node(resolved, fragment) {
-- 
2.30.2

[PATCH 2/2] of: overlay: avoid potential null pointer exception

[Michael Riesch]

The function of_overlay_fix_path returns NULL in certain error cases but
of_overlay_apply_symbols (which is the only caller) does not check the
return value. For broken overlays this may result in a null pointer
exception. Fix this by checking the return value and inform the user
what exactly went wrong.

Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
---
 drivers/of/overlay.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 20686db511..884cdf8928 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -102,8 +102,10 @@ static char *of_overlay_fix_path(struct device_node *root,
 		if (of_get_child_by_name(fragment, "__overlay__"))
 			break;
 	}
-	if (!fragment)
+	if (!fragment) {
+		pr_info("could not find __overlay__ node\n");
 		return NULL;
+	}
 
 	target = find_target(root, fragment);
 	if (!target)
@@ -143,6 +145,8 @@ static int of_overlay_apply_symbols(struct device_node *root,
 
 		old_path = of_property_get_value(prop);
 		new_path = of_overlay_fix_path(root, overlay, old_path);
+		if (!new_path)
+			return -EINVAL;
 
 		pr_debug("add symbol %s with new path %s\n",
 			 prop->name, new_path);
-- 
2.30.2

Re: [PATCH 0/2] of: overlay: avoid potential null pointer exception

[Sascha Hauer]

On Mon, Sep 05, 2022 at 12:07:15PM +0200, Michael Riesch wrote:
> Hi all,
> 
> The function of_overlay_fix_path returns NULL in certain error cases but
> of_overlay_apply_symbols (which is the only caller) does not check the
> return value. For broken overlays this may result in a null pointer
> exception. Fix this by checking the return value and inform the user
> what exactly went wrong. To this end, improve the error handling in
> of_overlay_apply_tree.
> 
> The thread [0] gives a bit more context.
> 
> Best regards,
> Michael
> 
> [0] https://lore.barebox.org/barebox/95ff064f-aa11-c1ce-9d41-e38f2040c565@wolfvision.net/T/#u
> 
> Michael Riesch (2):
>   of: overlay: improve error handling in of_overlay_apply_tree
>   of: overlay: avoid potential null pointer exception

Applied, thanks

Sascha

> 
>  drivers/of/overlay.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> -- 
> 2.30.2
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/2] of: overlay: improve error handling in of_overlay_apply_tree

[Michael Riesch]

Hi all,

On 9/5/22 12:07, Michael Riesch wrote:
> Propagate any error from of_overlay_apply_symbols and let the user
> know if the provided overlay is not applicable.
> 
> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
> ---
>  drivers/of/overlay.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 20a43f5170..20686db511 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -115,8 +115,8 @@ static char *of_overlay_fix_path(struct device_node *root,
>  	return basprintf("%s%s", target->full_name, path_tail);
>  }
>  
> -static void of_overlay_apply_symbols(struct device_node *root,
> -				     struct device_node *overlay)
> +static int of_overlay_apply_symbols(struct device_node *root,
> +				    struct device_node *overlay)
>  {
>  	const char *old_path;
>  	char *new_path;
> @@ -129,12 +129,12 @@ static void of_overlay_apply_symbols(struct device_node *root,
>  
>  	if (!overlay_symbols) {
>  		pr_debug("overlay doesn't have a __symbols__ node\n");
> -		return;
> +		return -EINVAL;

Come to think of it, do all overlays need to provide a __symbols__ node?
If not, this check is overly strict.

>  	}
>  
>  	if (!root_symbols) {
>  		pr_info("root doesn't have a __symbols__ node\n");
> -		return;
> +		return -EINVAL;

Ditto for the root.

>  	}
>  
>  	list_for_each_entry(prop, &overlay_symbols->properties, list) {
> @@ -148,6 +148,8 @@ static void of_overlay_apply_symbols(struct device_node *root,
>  			 prop->name, new_path);
>  		of_property_write_string(root_symbols, prop->name, new_path);
>  	}
> +
> +	return 0;
>  }
>  
>  static int of_overlay_apply_fragment(struct device_node *root,
> @@ -190,7 +192,9 @@ int of_overlay_apply_tree(struct device_node *root,
>  		goto out_err;
>  
>  	/* Copy symbols from resolved overlay to base device tree */
> -	of_overlay_apply_symbols(root, resolved);
> +	err = of_overlay_apply_symbols(root, resolved);
> +	if (err)
> +		goto out_err;

If both checks need to be relaxed, the complete patch should be reverted
I guess :-/

Thanks in advance for your comments.

Best regards,
Michael

>  
>  	/* Copy nodes and properties from resolved overlay to root */
>  	for_each_child_of_node(resolved, fragment) {

Re: [PATCH 1/2] of: overlay: improve error handling in of_overlay_apply_tree

[Michael Tretter]

On Wed, 21 Sep 2022 08:55:12 +0200, Michael Riesch wrote:
> On 9/5/22 12:07, Michael Riesch wrote:
> > Propagate any error from of_overlay_apply_symbols and let the user
> > know if the provided overlay is not applicable.
> > 
> > Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
> > ---
> >  drivers/of/overlay.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> > index 20a43f5170..20686db511 100644
> > --- a/drivers/of/overlay.c
> > +++ b/drivers/of/overlay.c
> > @@ -115,8 +115,8 @@ static char *of_overlay_fix_path(struct device_node *root,
> >  	return basprintf("%s%s", target->full_name, path_tail);
> >  }
> >  
> > -static void of_overlay_apply_symbols(struct device_node *root,
> > -				     struct device_node *overlay)
> > +static int of_overlay_apply_symbols(struct device_node *root,
> > +				    struct device_node *overlay)
> >  {
> >  	const char *old_path;
> >  	char *new_path;
> > @@ -129,12 +129,12 @@ static void of_overlay_apply_symbols(struct device_node *root,
> >  
> >  	if (!overlay_symbols) {
> >  		pr_debug("overlay doesn't have a __symbols__ node\n");
> > -		return;
> > +		return -EINVAL;
> 
> Come to think of it, do all overlays need to provide a __symbols__ node?
> If not, this check is overly strict.

Overlays don't need a __symbols__ node. It would be only required, if overlays
are stacked and the second overlay refers to nodes of the first overlay by
labels. Having no __symbols__ in the overlay is a success path and the message
is just a debug message.

> 
> >  	}
> >  
> >  	if (!root_symbols) {
> >  		pr_info("root doesn't have a __symbols__ node\n");
> > -		return;
> > +		return -EINVAL;
> 
> Ditto for the root.

I'm not sure what should happen, if the root does not have __symbols__.
Barebox wouldn't be able to copy the __symbols__ of the overlay, but this
still wouldn't be a problem unless overlays are stacked. In the stacking case,
only applying the second overlay should fail.

Maybe, we should add a new __symbols__ node, if the root doesn't have a
__symbols__ node?

> 
> >  	}
> >  
> >  	list_for_each_entry(prop, &overlay_symbols->properties, list) {
> > @@ -148,6 +148,8 @@ static void of_overlay_apply_symbols(struct device_node *root,
> >  			 prop->name, new_path);
> >  		of_property_write_string(root_symbols, prop->name, new_path);
> >  	}
> > +
> > +	return 0;
> >  }
> >  
> >  static int of_overlay_apply_fragment(struct device_node *root,
> > @@ -190,7 +192,9 @@ int of_overlay_apply_tree(struct device_node *root,
> >  		goto out_err;
> >  
> >  	/* Copy symbols from resolved overlay to base device tree */
> > -	of_overlay_apply_symbols(root, resolved);
> > +	err = of_overlay_apply_symbols(root, resolved);
> > +	if (err)
> > +		goto out_err;
> 
> If both checks need to be relaxed, the complete patch should be reverted
> I guess :-/

What did you do to run into this error? What was your expectation?

Michael

Re: [PATCH 1/2] of: overlay: improve error handling in of_overlay_apply_tree

[Michael Riesch]

Hi Michael,

On 9/21/22 09:57, Michael Tretter wrote:
> On Wed, 21 Sep 2022 08:55:12 +0200, Michael Riesch wrote:
>> On 9/5/22 12:07, Michael Riesch wrote:
>>> Propagate any error from of_overlay_apply_symbols and let the user
>>> know if the provided overlay is not applicable.
>>>
>>> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
>>> ---
>>>  drivers/of/overlay.c | 14 +++++++++-----
>>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>> index 20a43f5170..20686db511 100644
>>> --- a/drivers/of/overlay.c
>>> +++ b/drivers/of/overlay.c
>>> @@ -115,8 +115,8 @@ static char *of_overlay_fix_path(struct device_node *root,
>>>  	return basprintf("%s%s", target->full_name, path_tail);
>>>  }
>>>  
>>> -static void of_overlay_apply_symbols(struct device_node *root,
>>> -				     struct device_node *overlay)
>>> +static int of_overlay_apply_symbols(struct device_node *root,
>>> +				    struct device_node *overlay)
>>>  {
>>>  	const char *old_path;
>>>  	char *new_path;
>>> @@ -129,12 +129,12 @@ static void of_overlay_apply_symbols(struct device_node *root,
>>>  
>>>  	if (!overlay_symbols) {
>>>  		pr_debug("overlay doesn't have a __symbols__ node\n");
>>> -		return;
>>> +		return -EINVAL;
>>
>> Come to think of it, do all overlays need to provide a __symbols__ node?
>> If not, this check is overly strict.
> 
> Overlays don't need a __symbols__ node. It would be only required, if overlays
> are stacked and the second overlay refers to nodes of the first overlay by
> labels. Having no __symbols__ in the overlay is a success path and the message
> is just a debug message.

Thanks for the clarification. We need to fix this one, then.

Seeing that the patch is in next: Am I supposed to send an incremental
"fixup! ..." patch which can be squashed? Or should I send a proper
patch with a Fixes: tag?

>>>  	}
>>>  
>>>  	if (!root_symbols) {
>>>  		pr_info("root doesn't have a __symbols__ node\n");
>>> -		return;
>>> +		return -EINVAL;
>>
>> Ditto for the root.
> 
> I'm not sure what should happen, if the root does not have __symbols__.
> Barebox wouldn't be able to copy the __symbols__ of the overlay, but this
> still wouldn't be a problem unless overlays are stacked. In the stacking case,
> only applying the second overlay should fail.

I can reestablish the behavior before this patch, i.e., __symbols__ is
optional in both root and overlay.

> Maybe, we should add a new __symbols__ node, if the root doesn't have a
> __symbols__ node?

If this is desired, I can implement the change -- but is it desired?

>>>  	}
>>>  
>>>  	list_for_each_entry(prop, &overlay_symbols->properties, list) {
>>> @@ -148,6 +148,8 @@ static void of_overlay_apply_symbols(struct device_node *root,
>>>  			 prop->name, new_path);
>>>  		of_property_write_string(root_symbols, prop->name, new_path);
>>>  	}
>>> +
>>> +	return 0;
>>>  }
>>>  
>>>  static int of_overlay_apply_fragment(struct device_node *root,
>>> @@ -190,7 +192,9 @@ int of_overlay_apply_tree(struct device_node *root,
>>>  		goto out_err;
>>>  
>>>  	/* Copy symbols from resolved overlay to base device tree */
>>> -	of_overlay_apply_symbols(root, resolved);
>>> +	err = of_overlay_apply_symbols(root, resolved);
>>> +	if (err)
>>> +		goto out_err;
>>
>> If both checks need to be relaxed, the complete patch should be reverted
>> I guess :-/
> 
> What did you do to run into this error? What was your expectation?

Well I tried to apply an overlay without __symbols__ :-) (which did work
before).

Best regards,
Michael