Re: [PATCH 5/9] of: property: implement the .property_get_names() fwnode operation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] of: property: of_fwnode_property_get_names() sets *props to ZERO_SIZE_PTR
--

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

of: property: implement the .property_get_names() fwnode operation

This commit implements the .property_get_names() fwnode callback for OF
nodes by iterating over the device node properties and returning a newly
allocated string array of property names.

> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 45e55f6184880..16c9c307633bd 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c

[ ... ]

> @@ -1120,6 +1121,34 @@ of_fwnode_property_read_string_array(const struct fwnode_handle *fwnode,
>  		of_property_count_strings(node, propname);
>  }
>  
> +static int of_fwnode_property_get_names(const struct fwnode_handle *fwnode,
> +					char ***props)
> +{
> +	const struct device_node *node = to_of_node(fwnode);
> +	struct property *prop;
> +	int count = 0, i = 0;
> +	char **names;
> +
> +	for_each_property_of_node(node, prop)
> +		count++;
> +
> +	names = kcalloc(count, sizeof(*names), GFP_KERNEL);
> +	if (!names)
> +		return -ENOMEM;
> +
> +	for_each_property_of_node(node, prop) {
> +		names[i] = kstrdup(prop->name, GFP_KERNEL);
> +		if (!names[i]) {
> +			kfree_strarray(names, i);
> +			return -ENOMEM;
> +		}
> +		i++;
> +	}
> +
> +	*props = names;
> +	return count;
> +}

[Severity: Medium]
What happens in of_fwnode_property_get_names() if node->properties is NULL
(for example, on a dynamically created empty node from
of_changeset_create_node() or a node whose properties were removed via
of_remove_property())?

When count is 0, kcalloc(0, sizeof(*names), GFP_KERNEL) returns
ZERO_SIZE_PTR, which passes the !names check and sets *props to
ZERO_SIZE_PTR before returning 0.

However, the documentation for struct fwnode_operations in
include/linux/fwnode.h states:

 * @property_get_names: Retrieve the list of all properties of this firmware
 *			node. On success: returns the number of properties and
 *			sets *props to an array of NULL-terminated strings that
 *			must be freed by the user with kfree_strarray(). If
 *			there are no properties, 0 is returned and the props
 *			pointer is not changed. Returns negative error number on
 *			failure.

Should of_fwnode_property_get_names() return 0 early when count is 0 so
that *props is left untouched?

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