[Skiboot] [PATCH] core/device.c: Fix dt_find_compatible_node

[Andrew Donnellan]

On 07/02/18 13:56, Alistair Popple wrote:
> dt_find_compatible_node() and dt_find_compatible_node_on_chip() are used to
> find device nodes under a parent/root node with a given compatible
> property.
> 
> dt_next(root, prev) is used to walk the child nodes of the given parent and
> takes two arguments - root contains the parent node to walk whilst prev
> contains the previous child to search from so that it can be used as an
> iterator over all children nodes.
> 
> The first iteration of dt_find_compatible_node(root, prev) calls
> dt_next(root, root) which is not a well defined operation as prev is
> assumed to be child of the root node. The result is that when a node
> contains no children it will start returning the parent nodes siblings
> until it hits the top of the tree at which point a NULL derefence is
> attempted when looking for the root nodes parent.
> 
> Dereferencing NULL can result in undesirable data exceptions during system
> boot and untimely non-hilarious system crashes. dt_next() should not be
> called with prev == root. Instead we add a check to dt_next() such that
> passing prev = NULL will cause it to start iterating from the first child
> node (if any).
> 
> Also add a unit test for this case to run-device.c.
> 
> Signed-off-by: Alistair Popple <alistair at popple.id.au>

This is both less buggy and clearer.

Reviewed-by: Andrew Donnellan <andrew.donnellan at au1.ibm.com>

> ---
>   core/device.c          | 16 ++++++++++------
>   core/test/run-device.c |  1 +
>   2 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/core/device.c b/core/device.c
> index fc1db568..194eaef3 100644
> --- a/core/device.c
> +++ b/core/device.c
> @@ -608,6 +608,12 @@ struct dt_node *dt_first(const struct dt_node *root)
>   struct dt_node *dt_next(const struct dt_node *root,
>   			const struct dt_node *prev)
>   {
> +	if (!prev)
> +		prev = dt_first(root);
> +
> +	if (!prev)
> +		return NULL;
> +
>   	/* Children? */
>   	if (!list_empty(&prev->children))
>   		return dt_first(prev);
> @@ -719,10 +725,9 @@ struct dt_node *dt_find_compatible_node(struct dt_node *root,
>   					struct dt_node *prev,
>   					const char *compat)
>   {
> -	struct dt_node *node;
> +	struct dt_node *node = prev;
>   
> -	node = prev ? dt_next(root, prev) : root;
> -	for (; node; node = dt_next(root, node))
> +	while ((node = dt_next(root, node)))
>   		if (dt_node_is_compatible(node, compat))
>   			return node;
>   	return NULL;
> @@ -964,10 +969,9 @@ struct dt_node *dt_find_compatible_node_on_chip(struct dt_node *root,
>   						const char *compat,
>   						uint32_t chip_id)
>   {
> -	struct dt_node *node;
> +	struct dt_node *node = prev;
>   
> -	node = prev ? dt_next(root, prev) : root;
> -	for (; node; node = dt_next(root, node)) {
> +	while ((node = dt_next(root, node))) {
>   		u32 cid = __dt_get_chip_id(node);
>   		if (cid == chip_id &&
>   		    dt_node_is_compatible(node, compat))
> diff --git a/core/test/run-device.c b/core/test/run-device.c
> index 326be3ff..6d3842e6 100644
> --- a/core/test/run-device.c
> +++ b/core/test/run-device.c
> @@ -327,6 +327,7 @@ int main(void)
>   	gc2 = dt_new(c1, "node-without-compatible");
>   	assert(__dt_find_property(gc2, "compatible") == NULL);
>   	assert(!dt_node_is_compatible(gc2, "any-property"));
> +	assert(dt_find_compatible_node(gc2, NULL, "node-without-compatible") == NULL);
>   
>   	assert(dt_find_compatible_node(root, NULL, "generic-fake-bus") == c2);
>   	assert(dt_find_compatible_node(root, c2, "generic-fake-bus") == NULL);
> 

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan at au1.ibm.com  IBM Australia Limited