Re: [PATCH 4/9] device property: provide fwnode_property_get_names()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] device property: missing NULL check for props in fwnode_property_get_names()
--

commit c6de0ca7eaa5586d9db8c473e272baa86e7c8e04
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

device property: provide fwnode_property_get_names()

This commit adds fwnode_property_get_names() to retrieve the list of all
property names for a given firmware node, merging properties from both the
primary and secondary firmware nodes.

> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index a97575ff91990..7b0df3dfae53b 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c

[ ... ]

> @@ -542,6 +544,81 @@ int fwnode_property_match_string(const struct fwnode_handle *fwnode,
>  }
>  EXPORT_SYMBOL_GPL(fwnode_property_match_string);
>  
> +/**
> + * fwnode_property_get_names - get the list of all properties of this fwnode
> + * @fwnode: Firmware node to get the list of properties for
> + * @props: Pointer to an array of strings. It will contain the address of the
> + *	   string list containing the property names.
> + *
> + * Return: Number of property names written into *props. If > 0, the caller must
> + *	   free the array with kfree_strarray(). *props is only valid for
> + *	   a positive return value.
> + *	   %-EINVAL if arguments are not valid
> + *	   %-ENXIO if no suitable firmware interface is present
> + *	   %-ENOMEM if memory allocation failed
> + *
> + * The list also includes the properties of the secondary firmware node, if
> + * any. Properties present in both nodes are listed once.
> + */
> +int fwnode_property_get_names(const struct fwnode_handle *fwnode, char ***props)
> +{
> +	int ret1, ret2 = -ENXIO, n1, n2, i, j, count;
> +	char **prim = NULL, **sec = NULL, **merged;
> +
> +	if (IS_ERR_OR_NULL(fwnode))
> +		return -EINVAL;
> +
> +	ret1 = fwnode_call_int_op(fwnode, property_get_names, &prim);
> +	if (!IS_ERR_OR_NULL(fwnode->secondary))
> +		ret2 = fwnode_call_int_op(fwnode->secondary, property_get_names, &sec);
> +
> +	if (ret1 == -ENXIO && ret2 == -ENXIO)
> +		return -ENXIO;
> +
> +	n1 = max(ret1, 0);
> +	n2 = max(ret2, 0);
> +
> +	CLASS(strarray, prim_props)(prim, n1);
> +	CLASS(strarray, sec_props)(sec, n2);
> +
> +	if (ret1 < 0 && ret1 != -ENXIO)
> +		return ret1;
> +
> +	if (ret2 < 0 && ret2 != -ENXIO)
> +		return ret2;
> +
> +	if (n1 == 0 && n2 == 0)
> +		return 0;
> +
> +	merged = kcalloc(n1 + n2, sizeof(*merged), GFP_KERNEL);
> +	if (!merged)
> +		return -ENOMEM;
> +
> +	/* The strings are now owned by merged, only free the container. */
> +	for (i = 0; i < n1; i++) {
> +		merged[i] = prim_props.array[i];
> +		prim_props.array[i] = NULL;
> +	}
> +	count = n1;
> +
> +	/* Properties from the primary node take precedence. */
> +	for (i = 0; i < n2; i++) {
> +		for (j = 0; j < n1; j++) {
> +			if (!strcmp(merged[j], sec_props.array[i]))
> +				break;
> +		}
> +
> +		if (j >= n1) {
> +			merged[count++] = sec_props.array[i];
> +			sec_props.array[i] = NULL;
> +		}
> +	}
> +
> +	*props = merged;
> +	return count;
> +}

[Severity: Medium]
Should fwnode_property_get_names() in drivers/base/property.c also check
!props at entry and return -EINVAL?

The kernel-doc states that -EINVAL is returned if arguments are not valid,
and unlike fwnode_property_read_string_array(), props is not optional.
Because fwnode_property_get_names() passes &prim and &sec to the backend
property_get_names callbacks rather than forwarding props, the backend
callbacks never validate props either.

If a caller passes NULL for props (for instance, expecting it to query the
property count like fwnode_property_read_string_array()), it will return 0
instead of -EINVAL when the node has no properties, or dereference a NULL
props pointer at *props = merged and leak merged and all transferred property
name strings when the node has at least one property.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-fwnode-for-each-prop-v1-0-6e28fe1c72c4@oss.qualcomm.com?part=4