[PATCH v2] leds: max77705: use fwnode_for_each_child_node_scoped() in max77705_add_led()

[PATCH v2] leds: max77705: use fwnode_for_each_child_node_scoped() in max77705_add_led()

[Manush Prajwal]

Rework the subled loop in max77705_add_led() around
fwnode_for_each_child_node_scoped() instead of adding a manual
fwnode_handle_put(child) before the early return on a
max77705_parse_subled() failure, so the reference on the current
child fwnode is released automatically on every exit path from the
loop. child is never used outside the loop, so the now-unused outer
child declaration is dropped along with it.

Suggested-by: Krzysztof Kozlowski <krzk@kernel.org>
Fixes: 3b6eaa3db567 ("leds: Use fwnode_for_each_child_node() instead")
Signed-off-by: Manush Prajwal <manushprajwal555@gmail.com>
---
v2: Rework around fwnode_for_each_child_node_scoped() instead of the
    manual fwnode_handle_put(child) from v1, per Krzysztof
    Kozlowski's review.
---
 drivers/leds/leds-max77705.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/leds/leds-max77705.c b/drivers/leds/leds-max77705.c
index 1e2054c..4fd803c 100644
--- a/drivers/leds/leds-max77705.c
+++ b/drivers/leds/leds-max77705.c
@@ -160,7 +160,6 @@ static int max77705_add_led(struct device *dev, struct regmap *regmap, struct fw
 	struct max77705_led *led;
 	struct led_classdev *cdev;
 	struct mc_subled *info;
-	struct fwnode_handle *child;
 	struct led_init_data init_data = {};

 	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
@@ -191,7 +190,7 @@ static int max77705_add_led(struct device *dev, struct regmap *regmap, struct fw
 		cdev->brightness_set_blocking = max77705_led_brightness_set_multi;
 		cdev->blink_set = max77705_rgb_blink;

-		fwnode_for_each_child_node(np, child) {
+		fwnode_for_each_child_node_scoped(np, child) {
 			ret = max77705_parse_subled(dev, child, &info[i]);
 			if (ret < 0)
 				return ret;
--
2.46.2.windows.1

Re: [PATCH v2] leds: max77705: use fwnode_for_each_child_node_scoped() in max77705_add_led()

[sashiko-bot]

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

New issues:
- [Low] The commit message description is not capitalized.

Pre-existing issues:
- [High] Mismatch between OF and fwnode child iteration APIs causes buffer overflow and ZERO_SIZE_PTR dereference on non-OF platforms.
- [High] Passing `struct fwnode_handle *` to `%pOF` and `%pOFn` format specifiers causes undefined behavior and kernel panics.
- [High] Invalid `container_of` usage leads to out-of-bounds memory accesses and kernel panics.
- [High] Ignored return value from max77705_parse_subled() in the single LED branch causes invalid hardware configuration.
--

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

leds: max77705: use fwnode_for_each_child_node_scoped() in max77705_add_led()
    
Rework the subled loop in max77705_add_led() around
fwnode_for_each_child_node_scoped() instead of adding a manual
fwnode_handle_put(child) before the early return on a
max77705_parse_subled() failure, so the reference on the current
child fwnode is released automatically on every exit path from the
loop. child is never used outside the loop, so the now-unused outer
child declaration is dropped along with it.

> leds: max77705: use fwnode_for_each_child_node_scoped() in max77705_add_led()

[Severity: Low]
This isn't a bug, but should the commit message subject start with a capital
letter ("Use" instead of "use") to match the LED subsystem commit message prefix
conventions?

> diff --git a/drivers/leds/leds-max77705.c b/drivers/leds/leds-max77705.c
> index 1e2054c1bf80b..4fd803c959890 100644
> --- a/drivers/leds/leds-max77705.c
> +++ b/drivers/leds/leds-max77705.c

[ ... ]

> @@ -191,7 +190,7 @@ static int max77705_add_led(struct device *dev, struct regmap *regmap, struct fw
>  		cdev->brightness_set_blocking = max77705_led_brightness_set_multi;
>  		cdev->blink_set = max77705_rgb_blink;

[Severity: High]
This is a pre-existing issue, but does the cdev assignment and subsequent
container_of() usage cause out-of-bounds memory accesses?

In max77705_add_led(), the struct fields are copied by value into the
independent cdev:

    led->cdev = *cdev;

When max77705_rgb_blink() is called for the multicolor LED, it receives
&led->mcdev.led_cdev, but uses container_of() expecting the independent cdev:

    struct max77705_led *led = container_of(cdev, struct max77705_led, cdev);

This yields a pointer shifted by sizeof(struct led_classdev), leading to
out-of-bounds reads.

Conversely, when max77705_add_led() manually calls:

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

The function uses lcdev_to_mccdev() and container_of() expecting the embedded
mcdev:

    struct led_classdev_mc *mcdev = lcdev_to_mccdev(cdev);
    struct max77705_led *led = container_of(mcdev, struct max77705_led, mcdev);

This subtracts sizeof(struct led_classdev) from an offset 0 pointer, resulting
in a negative offset pointer underflow.

> -		fwnode_for_each_child_node(np, child) {
> +		fwnode_for_each_child_node_scoped(np, child) {

[Severity: High]
This is a pre-existing issue, but does this fwnode loop cause a buffer
overflow and ZERO_SIZE_PTR dereference on non-OF platforms?

Earlier in max77705_add_led(), the allocation is sized using an OF-specific API
that returns 0 for non-OF nodes:

    int num_channels = of_get_available_child_count(to_of_node(np));
    ...
    info = devm_kcalloc(dev, num_channels, sizeof(*info), GFP_KERNEL);

If num_channels is 0, devm_kcalloc() allocates 0 bytes and returns
ZERO_SIZE_PTR. However, the fwnode_for_each_child_node_scoped() loop
indiscriminately iterates over all fwnode children. Writing to &info[i] inside
the loop will then dereference the ZERO_SIZE_PTR, causing an immediate page
fault and kernel panic.

>  			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 a %pOFn format specifier?

When parsing fails in 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);

The %pOF specifiers strictly expect a struct device_node *. Because
fwnode_handle is embedded at a non-zero offset within device_node, the
formatting engine treats the fwnode pointer as the start of the device_node
struct, leading to memory corruption or a kernel panic when strings are read
from incorrect offsets.

This also appears to happen in max77705_add_led() for the "color" property
error messages.

>  			if (ret < 0)
>  				return ret;

[Severity: High]
This is also a pre-existing issue, but is the return value of
max77705_parse_subled() ignored in the single LED branch?

In the single LED initialization branch:

    } else {
        info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
        if (!info)
            return -ENOMEM;

        max77705_parse_subled(dev, np, info);

The return value is ignored. If parsing fails, info->channel remains 0 (from
devm_kzalloc()). Since max77705_parse_subled() explicitly rejects !reg (0),
channel 0 is invalid. The driver proceeds to register the LED classdev and calls
max77705_led_brightness_set_single(&led->cdev, LED_OFF), which issues I2C
commands for the invalid channel 0, potentially causing hardware
misconfiguration.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/6a914e1e.7d702ed2.1273e8.2a52@mx.google.com?part=1

Re: [PATCH v2] leds: max77705: use fwnode_for_each_child_node_scoped() in max77705_add_led()

[Krzysztof Kozlowski]

On 28/08/2026 11:00, Manush Prajwal wrote:
> Rework the subled loop in max77705_add_led() around
> fwnode_for_each_child_node_scoped() instead of adding a manual
> fwnode_handle_put(child) before the early return on a
> max77705_parse_subled() failure, so the reference on the current
> child fwnode is released automatically on every exit path from the
> loop. child is never used outside the loop, so the now-unused outer
> child declaration is dropped along with it.
> 
> Suggested-by: Krzysztof Kozlowski <krzk@kernel.org>

That's probably a stretch, I suggested only a solution, not the commit
itself. Anyway, no need to resend to change it.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

Best regards,
Krzysztof

Re: (subset) [PATCH v2] leds: max77705: use fwnode_for_each_child_node_scoped() in max77705_add_led()

[Lee Jones]

On Fri, 28 Aug 2026 14:30:13 +0530, Manush Prajwal wrote:
> Rework the subled loop in max77705_add_led() around
> fwnode_for_each_child_node_scoped() instead of adding a manual
> fwnode_handle_put(child) before the early return on a
> max77705_parse_subled() failure, so the reference on the current
> child fwnode is released automatically on every exit path from the
> loop. child is never used outside the loop, so the now-unused outer
> child declaration is dropped along with it.
> 
> [...]

Applied, thanks!

[1/1] leds: max77705: use fwnode_for_each_child_node_scoped() in max77705_add_led()
      commit: 4805e5ec50d0b355370e3d1ea906af4fc758accf

--
Lee Jones [李琼斯]