Re: [PATCH v4 08/14] mfd: lm3533: Convert to use OF bindings

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] A crafted device tree with a single child node containing multiple matching compatible strings causes a heap buffer overflow in `lm3533_devices`.
- [High] `lm3533_led_probe` mistakenly returns 0 (success) on an error path, causing the device to remain bound and leading to a double-free on removal.
- [Medium] `lm3533_bl_probe` returns a positive integer on an error path, violating the probe function API and triggering a kernel warning.
- [Low] Missing 'led-sources' property causes unintended I2C transactions due to improper checking of negative error codes.
- [Low] The `ti,pwm-config-mask` property is implicitly truncated from 32 bits to 8 bits before validation, allowing invalid values to bypass the bounds check.
--

commit 064deeedced51b760172cc0805ab8c80bd2dafdf
Author: Svyatoslav Ryhel <clamor95@gmail.com>
Date:   Sat Jun 6 04:57:32 2026 +0000

    mfd: lm3533: Convert to use OF bindings
    
    Since there are no users of this driver via platform data, remove the
    platform data support and switch to using Device Tree bindings.

> diff --git a/drivers/mfd/lm3533-core.c b/drivers/mfd/lm3533-core.c
[ ... ]
> @@ -296,129 +266,99 @@ static struct attribute_group lm3533_attribute_group = {
[ ... ]
> +static int lm3533_device_init(struct lm3533 *lm3533)
>  {
[ ... ]
> +	device_for_each_child_node_scoped(dev, child) {
> +		if (count >= nchilds)
> +			break;
>  
[ ... ]
> +		if (fwnode_device_is_compatible(child, "ti,lm3533-als")) {
> +			lm3533_devices[count].name = "lm3533-als";
> +			lm3533_devices[count].of_compatible = "ti,lm3533-als";
> +			lm3533_devices[count].id = PLATFORM_DEVID_NONE;
>  
> +			lm3533->have_als = true;
> +			count++;
> +		}
>  
> +		if (fwnode_device_is_compatible(child, "ti,lm3533-backlight")) {
> +			ret = fwnode_property_read_u32(child, "reg", &reg);

[Severity: High]
Can this loop overflow the lm3533_devices array?

Since the bounds check if (count >= nchilds) break; only happens at the top
of the loop, if a single child node specifies multiple compatible strings (e.g.,
compatible = "ti,lm3533-als", "ti,lm3533-backlight"), multiple if branches
will execute during the same iteration. 

This would increment the count multiple times per node, bypassing the bounds
check and writing past the end of the allocated lm3533_devices buffer.

Should these be else if conditions to ensure only one match per child node?

> diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c
[ ... ]
> @@ -680,15 +684,23 @@ static int lm3533_led_probe(struct platform_device *pdev)
[ ... ]
>  	led->cb.dev = led->cdev.dev;
>  
> -	ret = lm3533_led_setup(led, pdata);
> +	device_property_read_u32(&pdev->dev, "led-max-microamp",
> +				 &led->max_current);
> +	led->max_current = clamp(led->max_current, LM3533_MAX_CURRENT_MIN,
> +				 LM3533_MAX_CURRENT_MAX);
> +
> +	led->pwm = 0;
> +	device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &led->pwm);
> +

[Severity: Low]
Could this allow invalid values to bypass validation?

The property ti,pwm-config-mask is read into the u32 variable led->pwm,
but it is later passed to lm3533_ctrlbank_set_pwm() which accepts a u8
argument. This implicitly truncates the 32-bit integer to 8 bits.

If a device tree mistakenly specifies a value exceeding 255 (like <0x13f>),
it will be truncated to 0x3f before validation. Inside
lm3533_ctrlbank_set_pwm(), the truncated value will easily pass the
val > LM3533_PWM_MAX (0x3f) bounds check, resulting in a silent
misconfiguration.

> +	ret = lm3533_led_setup(led);
>  	if (ret)
>  		goto err_deregister;

[Severity: Low]
Does this code correctly handle negative error codes when led-sources is
missing?

In lm3533_led_probe(), led->num_leds is assigned the result of
device_property_count_u32(..., "led-sources"). If this property is absent,
it returns -EINVAL.

Later in lm3533_led_setup(), the condition if (led->num_leds) will
evaluate to true for negative values. While the inner loop is skipped,
regmap_update_bits() executes with a mask of 0, causing unnecessary hardware
I2C reads. 

Should the condition be > 0 to properly skip the hardware update
when the property is absent? (Note: This same behavior exists in
lm3533_bl_setup() for bl->num_leds).

[Severity: High]
Does the error path for led-sources leak a successful return code?

In lm3533_led_probe(), if the count of led-sources exceeds
LM3533_LVCTRLBANK_MAX, the code logs an error and jumps to err_deregister.
However, the ret variable at this point is 0 from the earlier successful
led_classdev_register() call.

Returning 0 falsely informs the driver core that probe succeeded. When the
module is removed, lm3533_led_remove() will invoke led_classdev_unregister()
on the already-unregistered LED class device, resulting in a use-after-free or
double-free. 

Should ret be set to a negative error code (e.g., -EINVAL) before jumping
to the error label?

> diff --git a/drivers/video/backlight/lm3533_bl.c b/drivers/video/backlight/lm3533_bl.c
[ ... ]
> @@ -304,6 +311,17 @@ static int lm3533_bl_probe(struct platform_device *pdev)
[ ... ]
> +	device_property_read_u32(&pdev->dev, "led-max-microamp",
> +				 &bl->max_current);
> +	bl->max_current = clamp(bl->max_current, LM3533_MAX_CURRENT_MIN,
> +				LM3533_MAX_CURRENT_MAX);
> +
> +	device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &bl->pwm);
> +
> +	ret = lm3533_bl_setup(bl);
> +	if (ret)
> +		return ret;
> +

[Severity: Medium]
Does the error handling for led-sources violate the probe function API?

In lm3533_bl_probe(), if the count of led-sources exceeds
LM3533_HVCTRLBANK_COUNT, the code executes:

return dev_err_probe(&pdev->dev, bl->num_leds,
                     "num of LED sources exceeds max %d\n",
                     LM3533_HVCTRLBANK_COUNT);
                    
dev_err_probe() returns its second argument as the error code. Because
bl->num_leds is a positive value, the probe function returns a positive integer
instead of a negative error code. 

This violates the API and causes the driver core's really_probe() to emit a
warning before overriding the return code to -ENXIO. 

Should this return a negative error code like -EINVAL instead?

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