Re: [PATCH] leds: is31fl32xx: Use fwnode/device property APIs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Potential out-of-bounds write to `priv->leds` array due to TOCTOU race condition during child node iteration.
- [High] NULL pointer dereference in `is31fl32xx_parse_dt()` when device match data is absent.
--

commit ed8f6f187305b333f2b97784d1f0f1abc18b4cb5
Author: Rosen Penev <rosenp@gmail.com>

leds: is31fl32xx: Use fwnode/device property APIs

This commit converts the DT-specific helpers in is31fl32xx_parse_dt() and
is31fl32xx_parse_child_dt() to their fwnode/device equivalents. It retains
the skipping of unavailable children and updates the format specifiers.

> diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
> --- a/drivers/leds/leds-is31fl32xx.c
> +++ b/drivers/leds/leds-is31fl32xx.c
[ ... ]
> @@ -422,7 +422,7 @@ static int is31fl32xx_parse_dt(struct device *dev,
>  	int ret = 0;
>  
>  	if ((cdef->output_frequency_setting_reg != IS31FL32XX_REG_NONE) &&

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for cdef to be
NULL here?

If the device binds via the I2C ID table (e.g., fallback matching by node name
without a valid compatible string, or via sysfs new_device with an attached
software node), device_get_match_data(dev) in is31fl32xx_probe() returns NULL
because the is31fl32xx_id table entries have no .driver_data.

If the node also contains children, device_get_child_node_count() allows the
probe to proceed, and is31fl32xx_parse_dt() will then unconditionally
dereference cdef when checking cdef->output_frequency_setting_reg.

Should there be a NULL check for cdef prior to this?

> -	    of_property_read_bool(dev_of_node(dev), "issi,22khz-pwm")) {
> +	    device_property_read_bool(dev, "issi,22khz-pwm")) {
>  
>  		ret = is31fl32xx_write(priv, cdef->output_frequency_setting_reg,
>  				       IS31FL32XX_PWM_FREQUENCY_22KHZ);
> @@ -433,7 +433,7 @@ static int is31fl32xx_parse_dt(struct device *dev,
>  		}
>  	}
>  
> -	for_each_available_child_of_node_scoped(dev_of_node(dev), child) {
> +	device_for_each_child_node_scoped(dev, child) {
>  		struct led_init_data init_data = {};
>  		struct is31fl32xx_led_data *led_data =
>  			&priv->leds[priv->num_leds];

[Severity: High]
This is a pre-existing issue, but could this iteration cause an out-of-bounds
write to the priv->leds array?

In is31fl32xx_probe(), device_get_child_node_count() calculates the number of
available child nodes to allocate the priv->leds array. Later, this loop
iterates over the children and unconditionally increments priv->num_leds while
writing to the array.

If a child node is added concurrently (e.g., via a device tree overlay)
between these two lockless operations, the loop will execute more times
than the allocated count.

Would it be safer to ensure priv->num_leds does not exceed the originally
allocated count within this loop?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907201940.40524-1-rosenp@gmail.com?part=1