[PATCH] leds: max77705: fix fwnode reference leak in max77705_add_led()

[PATCH] leds: max77705: fix fwnode reference leak in max77705_add_led()

[Manush Prajwal]

In the RGB LED path of max77705_add_led(), the
fwnode_for_each_child_node() loop over the subled child nodes
returns directly on a max77705_parse_subled() failure without
releasing the reference held on the current child, since
fwnode_for_each_child_node() is not a scoped/cleanup-based
iterator and expects the caller to drop the reference itself on
any early exit from the loop body.

Call fwnode_handle_put() on the child fwnode before returning to
fix the leak.

Fixes: 3b6eaa3db567 ("leds: Use fwnode_for_each_child_node() instead")
Signed-off-by: Manush Prajwal <manushprajwal555@gmail.com>
---
 drivers/leds/leds-max77705.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/leds-max77705.c b/drivers/leds/leds-max77705.c
index 1e2054c..a5030c3 100644
--- a/drivers/leds/leds-max77705.c
+++ b/drivers/leds/leds-max77705.c
@@ -193,8 +193,10 @@ static int max77705_add_led(struct device *dev, struct regmap *regmap, struct fw

 		fwnode_for_each_child_node(np, child) {
 			ret = max77705_parse_subled(dev, child, &info[i]);
-			if (ret < 0)
+			if (ret < 0) {
+				fwnode_handle_put(child);
 				return ret;
+			}

 			info[i].intensity = 0;
 			i++;
--
2.46.2.windows.1

Re: [PATCH] leds: max77705: fix fwnode reference leak in max77705_add_led()

[Krzysztof Kozlowski]

On 28/08/2026 10:46, Manush Prajwal wrote:
> In the RGB LED path of max77705_add_led(), the
> fwnode_for_each_child_node() loop over the subled child nodes
> returns directly on a max77705_parse_subled() failure without
> releasing the reference held on the current child, since
> fwnode_for_each_child_node() is not a scoped/cleanup-based
> iterator and expects the caller to drop the reference itself on
> any early exit from the loop body.
> 
> Call fwnode_handle_put() on the child fwnode before returning to
> fix the leak.
> 
> Fixes: 3b6eaa3db567 ("leds: Use fwnode_for_each_child_node() instead")
> Signed-off-by: Manush Prajwal <manushprajwal555@gmail.com>
> ---
>  drivers/leds/leds-max77705.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/leds-max77705.c b/drivers/leds/leds-max77705.c
> index 1e2054c..a5030c3 100644
> --- a/drivers/leds/leds-max77705.c
> +++ b/drivers/leds/leds-max77705.c
> @@ -193,8 +193,10 @@ static int max77705_add_led(struct device *dev, struct regmap *regmap, struct fw
> 
>  		fwnode_for_each_child_node(np, child) {

Would fwnode_for_each_child_node_scoped() work here?

>  			ret = max77705_parse_subled(dev, child, &info[i]);
> -			if (ret < 0)
> +			if (ret < 0) {
> +				fwnode_handle_put(child);
>  				return ret;
> +			}
> 
Best regards,
Krzysztof

Re: [PATCH] leds: max77705: fix fwnode reference leak in max77705_add_led()

[sashiko-bot]

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

New issues:
- [Low] The commit message subject violates the LED subsystem naming conventions regarding capitalization.

Pre-existing issues:
- [Critical] Passing the wrong `led_classdev` reference to `max77705_led_brightness_set_multi` causes an out-of-bounds pointer dereference.
- [Critical] Shared `max77705_rgb_blink()` callback uses incorrect `container_of` arithmetic for multicolor LEDs, causing out-of-bounds pointer dereference.
- [High] The driver passes a `struct fwnode_handle *` to the `%pOF` and `%pOFn` format specifiers.
- [High] Mixing OF-specific `of_get_available_child_count()` with generic `fwnode_for_each_child_node()` causes a zero-allocation buffer overflow on non-Device Tree backends.
- [High] The return value of `max77705_parse_subled()` is completely ignored in the non-RGB single-LED registration path.
- [High] Valid hardware channel 0 is erroneously rejected by the `!reg` check in `max77705_parse_subled()`.
- [High] Uninitialized return value 'ret' in 'max77705_led_brightness_set()'.
--

commit fd8007d53291c3b9d58b3510fc44a4453a7cab3d
Author: Manush Prajwal <manushprajwal555@gmail.com>

leds: max77705: fix fwnode reference leak in max77705_add_led()

In the RGB LED path of max77705_add_led(), the
fwnode_for_each_child_node() loop over the subled child nodes
returns directly on a max77705_parse_subled() failure without
releasing the reference held on the current child.

Fixes: 3b6eaa3db567 ("leds: Use fwnode_for_each_child_node() instead")

> leds: max77705: fix fwnode reference leak in max77705_add_led()

[Severity: Low]
Is this commit message following the LED subsystem naming conventions? The
LED subsystem guide mandates: "Always capitalize the description after the
subsystem prefix". Should "fix" be capitalized?

> diff --git a/drivers/leds/leds-max77705.c b/drivers/leds/leds-max77705.c
> index 1e2054c1bf80b..a5030c329587b 100644
> --- a/drivers/leds/leds-max77705.c
> +++ b/drivers/leds/leds-max77705.c
> @@ -193,8 +193,10 @@ static int max77705_add_led(struct device *dev, struct regmap *regmap, struct fw
>  
>  		fwnode_for_each_child_node(np, child) {

[Severity: High]
This isn't a bug introduced by this patch, but mixing OF-specific
of_get_available_child_count() with generic fwnode_for_each_child_node()
causes a zero-allocation buffer overflow on non-Device Tree backends.

If the driver is instantiated via a non-OF backend (like software nodes),
to_of_node(np) returns NULL, causing of_get_available_child_count() to
return 0:

drivers/leds/leds-max77705.c:max77705_add_led() {
    ...
    int num_channels = of_get_available_child_count(to_of_node(np));
    ...
    info = devm_kcalloc(dev, num_channels, sizeof(*info), GFP_KERNEL);
    ...

devm_kcalloc() evaluates to ZERO_SIZE_PTR since num_channels is 0. Does
this fwnode_for_each_child_node() loop then iterate available child nodes
for non-OF backends anyway, resulting in writes to the ZERO_SIZE_PTR in
the loop body?

>  			ret = max77705_parse_subled(dev, child, &info[i]);

[Severity: High]
This is a pre-existing issue, but does max77705_parse_subled() pass a
struct fwnode_handle * to the %pOF format specifier?

drivers/leds/leds-max77705.c:max77705_parse_subled() {
    ...
    return dev_err_probe(dev, -EINVAL, "invalid \"reg\" of %pOFn\n", np);
    ...
    return dev_err_probe(dev, ret,
                         "failed to parse \"color\" of %pOF\n", np);

Since %pOF strictly expects a struct device_node *, passing a struct
fwnode_handle * causes the vsnprintf handler to read from invalid internal
struct offsets, which triggers a kernel panic when formatting an invalid
fwnode_handle as a device_node. This also occurs in max77705_add_led():

drivers/leds/leds-max77705.c:max77705_add_led() {
    ...
    return dev_err_probe(dev, ret,
                  "failed to parse \"color\" of %pOF\n", np);
    ...


[Severity: High]
This is a pre-existing issue, but does max77705_parse_subled() erroneously
reject valid hardware channel 0?

drivers/leds/leds-max77705.c:max77705_parse_subled() {
    ...
    ret = fwnode_property_read_u32(np, "reg", &reg);
    if (ret || !reg || reg >= MAX77705_LED_NUM_LEDS)
        return dev_err_probe(dev, -EINVAL, "invalid \"reg\" of %pOFn\n", np);
    ...

When parsing a subled node with reg = 0 (a perfectly valid hardware channel
mapped to MAX77705_RGBLED_REG_LED0BRT), the condition evaluates to true
because !reg is true for 0. Does this logic incorrectly return -EINVAL,
making it impossible to configure the first hardware LED channel?

> -			if (ret < 0)
> +			if (ret < 0) {
> +				fwnode_handle_put(child);
>  				return ret;
> +			}
>  
>  			info[i].intensity = 0;
>  			i++;

[Severity: High]
This is a pre-existing issue, but is the return value of
max77705_parse_subled() completely ignored in the non-RGB single-LED
registration path?

drivers/leds/leds-max77705.c:max77705_add_led() {
    ...
    } else {
        ...
        max77705_parse_subled(dev, np, info);

        led->subled_info = info;
    ...

When processing a single-color LED, max77705_parse_subled() is called, which
can return a negative error code. Does failing to check this return code
allow the driver to ignore the error and proceed registering an LED with
zero-initialized values?


[Severity: Critical]
This is a pre-existing issue, but does max77705_add_led() pass the wrong
led_classdev reference to max77705_led_brightness_set_multi()?

drivers/leds/leds-max77705.c:max77705_add_led() {
    ...
    ret = devm_led_classdev_multicolor_register_ext(dev, &led->mcdev, &init_data);
    if (ret)
        return ret;

    ret = max77705_led_brightness_set_multi(&led->cdev, LED_OFF);
    ...

The driver manually calls max77705_led_brightness_set_multi() to set the
initial state of an RGB LED unconditionally on probe. It incorrectly passes
&led->cdev instead of the registered multicolor cdev &led->mcdev.led_cdev.

drivers/leds/leds-max77705.c:max77705_led_brightness_set_multi() {
    struct led_classdev_mc *mcdev = lcdev_to_mccdev(cdev);
    struct max77705_led *led = container_of(mcdev, struct max77705_led, mcdev);
    ...

The target function uses container_of() to subtract the offset of mcdev,
yielding a pointer that points into incorrectly aligned or preceding memory.
Does this memory corruption result in a consistent kernel panic during the
probe phase?


[Severity: Critical]
This is a pre-existing issue, but does the max77705_rgb_blink() callback use
incorrect container_of arithmetic for multicolor LEDs?

drivers/leds/leds-max77705.c:max77705_add_led() {
    ...
        cdev->blink_set = max77705_rgb_blink;
    ...

When the LED subsystem triggers the blink_set callback for an RGB LED, it
passes &led->mcdev.led_cdev.

drivers/leds/leds-max77705.c:max77705_rgb_blink() {
    struct max77705_led *led = container_of(cdev, struct max77705_led, cdev);
    ...

max77705_rgb_blink() assumes it receives the single-color &led->cdev and
resolves the parent struct using container_of(cdev, struct max77705_led,
cdev). This subtracts 0 instead of the offset of mcdev, producing a pointer
that points inside the structure rather than at its beginning. Does this
cause an out-of-bounds pointer dereference leading to memory corruption?


[Severity: High]
This isn't a bug introduced by this patch, but does
max77705_led_brightness_set() return an uninitialized stack variable?

drivers/leds/leds-max77705.c:max77705_led_brightness_set() {
    int ret;

    for (int i = 0; i < num_colors; i++) {
        ...
    }
    return ret;
}

If a multi-color LED node is defined with no available child subleds,
num_channels evaluates to 0. The loop never executes, and the function
returns the uninitialized stack variable ret directly to callers. Can this
cause the probe function to fail randomly or mask real errors?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/6a914ad5.1e1ac41d.2a3f62.4979@mx.google.com?part=1