Re: [PATCH v3 4/4] backlight: qcom-wled: Read back the programmed brightness at probe

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Type confusion and memory corruption in wled_remove() due to incorrect drvdata pointer.

Pre-existing issues:
- [High] Use-after-free on probe error path due to uncancelled delayed work.
--

commit 925c19cfe92bc196afb0b7a2001d793450d1b94f
Author: David Heidelberg <david@ixit.cz>

backlight: qcom-wled: Read back the programmed brightness at probe

Since the previous change wled_probe() seeds wled->brightness from
MOD_EN, but with default-brightness rather than the level the hardware
is actually driving, so a backlight left lit by the bootloader still
visibly steps on the first update. Add wled_read_brightness() to seed
both wled->brightness and the initial backlight property.

> diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> index eb742a6598173..471a0cb2b81f6 100644
> --- a/drivers/video/backlight/qcom-wled.c
> +++ b/drivers/video/backlight/qcom-wled.c

[ ... ]

> @@ -1757,7 +1809,7 @@ static int wled_probe(struct platform_device *pdev)
>  
>  	memset(&props, 0, sizeof(struct backlight_properties));
>  	props.type = BACKLIGHT_RAW;
> -	props.brightness = val;
> +	props.brightness = wled->brightness ?: val;
>  	props.max_brightness = wled->max_brightness;
>  	bl = devm_backlight_device_register(&pdev->dev, wled->name,
>  					    &pdev->dev, wled,
>  					    &wled_ops, &props);
>  	platform_set_drvdata(pdev, bl);

[Severity: Critical]
Does this code incorrectly set the platform drvdata to a backlight_device
pointer instead of the expected wled structure pointer?

Looking at wled_remove(), it retrieves the platform drvdata but dereferences
it as a struct wled:

static void wled_remove(struct platform_device *pdev)
{
        struct wled *wled = platform_get_drvdata(pdev);

        mutex_destroy(&wled->lock);
        cancel_delayed_work_sync(&wled->ovp_work);
        ...
}

Because the memory layout does not match, can this cause type confusion,
destruction of uninitialized or unrelated mutexes, and guaranteed memory
corruption or a kernel panic on removal?

>  	return PTR_ERR_OR_ZERO(bl);

[Severity: High]
This is a pre-existing issue, but if devm_backlight_device_register() fails,
this function returns an error and devm frees the wled structure without
canceling any potentially scheduled delayed work. Is it possible for a
spurious IRQ to trigger a use-after-free here?

wled_probe() configures and implicitly enables the short IRQ earlier via
devm_request_threaded_irq(). It also configures the OVP IRQ, which sets
wled->ovp_irq > 0. If the short IRQ triggers during or just before this
failure, wled_short_irq_handler() will execute and call
wled_module_enable(wled, true).

Because wled->ovp_irq > 0, wled_module_enable() schedules the delayed work:

        schedule_delayed_work(&wled->ovp_work, HZ / 100);

Since the probe error path does not call cancel_delayed_work_sync(), this
leaves the work on the queue. When the delayed work executes, it calls:

static void wled_ovp_work(struct work_struct *work)
{
        struct wled *wled = container_of(work,
                                         struct wled, ovp_work.work);
        enable_irq(wled->ovp_irq);
}

Can this access the already freed wled structure during the delayed work
execution, resulting in a use-after-free?

>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914-qcom-wled-backlight-v3-0-d69fe9843841@ixit.cz?part=4