Re: [PATCH 6/7] state: add fixup to copy state from barebox to kernel device tree

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

On Wed, May 13, 2015 at 12:12:31PM +0200, Marc Kleine-Budde wrote:
> This patch registers a DT fixup, that copies the state nodes from the active
> to the kernel DT. The backend reference will be a phandles.
> 
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  common/state.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 92 insertions(+)
> 
> diff --git a/common/state.c b/common/state.c
> index d6060bb7dff4..ca691d2827fa 100644
> --- a/common/state.c
> +++ b/common/state.c
> @@ -676,6 +676,93 @@ static int state_from_node(struct state *state, struct device_node *node,
>  	return ret;
>  }
>  
> +static int of_state_fixup(struct device_node *root, void *ctx)
> +{
> +	struct state *state = ctx;
> +	const char *compatible = "barebox,state";
> +	struct device_node *new_node, *node, *parent, *backend_node;
> +	struct property *p;
> +	int ret;
> +	phandle phandle;
> +
> +	node = of_find_node_by_path_from(root, state->root->full_name);
> +	if (node) {
> +		/* replace existing node - it will be deleted later */
> +		parent = node->parent;
> +	} else {
> +		char *of_path, *c;
> +
> +		/* look for parent, remove last '/' from path */
> +		of_path = xstrdup(state->root->full_name);
> +		c = strrchr(of_path, '/');
> +		if (!c)
> +			return -ENODEV;
> +		*c = '0';
> +		parent = of_find_node_by_path(of_path);
> +		if (!parent)
> +			parent = root;
> +
> +		free(of_path);
> +	}
> +
> +	/* serialize variable definitions */
> +	new_node = state_to_node(state, parent, STATE_CONVERT_FIXUP);
> +	if (IS_ERR(new_node))
> +		return PTR_ERR(new_node);
> +
> +	/* compatible */
> +	p = of_new_property(new_node, "compatible", compatible,
> +			    strlen(compatible) + 1);
> +	if (!p) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	/* backend-type */
> +	if (!state->backend) {
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +
> +	p = of_new_property(new_node, "backend-type", state->backend->name,
> +			    strlen(state->backend->name) + 1);
> +	if (!p) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	/* backend phandle */
> +	backend_node = of_find_node_by_path_from(root, state->backend->of_path);
> +	if (!backend_node) {
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +
> +	phandle = of_node_create_phandle(backend_node);
> +	ret = of_property_write_u32(new_node, "backend", phandle);
> +	if (ret)
> +		goto out;
> +
> +	/* address-cells + size-cells */
> +	ret = of_property_write_u32(root, "#address-cells", 1);
> +	if (ret)
> +		goto out;
> +
> +	ret = of_property_write_u32(root, "#size-cells", 1);
> +	if (ret)
> +		goto out;
> +
> +	/* delete existing node */
> +	if (node)
> +		of_delete_node(node);
> +
> +	return 0;
> +
> + out:
> +	of_delete_node(new_node);
> +	return ret;
> +}
> +
>  /*
>   * state_new_from_node - create a new state instance from a device_node
>   *
> @@ -697,6 +784,11 @@ struct state *state_new_from_node(const char *name, struct device_node *node)
>  		return ERR_PTR(ret);
>  	}
>  
> +	ret = of_register_fixup(of_state_fixup, state);
> +	if (ret) {
> +		state_release(state);
> +		return ERR_PTR(ret);
> +	}
This is broken. state_new_from_node is called in state_probe:

	static int state_probe(struct device_d *dev)
	{
		...
		state = state_new_from_node(alias, np);
		...
		ret = of_find_path(np, "backend", &path, 0);
		if (ret)
			return ret;
		...

So if there is an error with the backend (or something else later that
makes the state invalid) the fixup is still registered. So when the
fixup is actually called state might be invalid resulting in
of_state_fixup using possibly overwritten memory.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox