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

[sashiko-bot@kernel.org]

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