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 <[email protected]> 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", ®); [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/[email protected]?part=8
