Re: [PATCH v2 2/3] backlight: qcom-wled: Fix unbalanced OVP IRQ enable at probe

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing platform_set_drvdata() in wled_probe() leads to a guaranteed NULL pointer dereference in wled_remove().
- [High] Incorrect teardown order in wled_remove() causes mutex use-after-destroy and use-after-free via re-armed delayed work.
- [High] A race condition between wled_short_irq_handler() and wled_update_status() can leave the hardware module incorrectly enabled when requested off.
- [High] Missing cancel_delayed_work_sync() on the probe error path leads to Use-After-Free if the short IRQ fires during probe.
- [High] wled_remove() unconditionally calls disable_irq() on short_irq and ovp_irq, which may be unallocated or negative.
--

commit 43030df8f21835cdbc934e6ed48a2467d72a8a08
Author: David Heidelberg <david@ixit.cz>

backlight: qcom-wled: Fix unbalanced OVP IRQ enable at probe

wled_configure_ovp_irq() derives the initial state of the OVP interrupt
from the hardware but wled->brightness is left at zero. This patch moves
the MOD_EN readback into wled_probe() and seeds wled->brightness to fix
the unbalanced IRQ enable.

> diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> index 22c35da72dce3..43e9f6f755582 100644
> --- a/drivers/video/backlight/qcom-wled.c
> +++ b/drivers/video/backlight/qcom-wled.c
> @@ -1627,7 +1627,6 @@ static int wled_configure_ovp_irq(struct wled *wled,
>  				  struct platform_device *pdev)
>  {
>  	int rc;
> -	u32 val;
>  
>  	wled->ovp_irq = platform_get_irq_byname(pdev, "ovp");
>  	if (wled->ovp_irq < 0) {

[Severity: High]
This isn't a regression introduced by this patch, but if
platform_get_irq_byname() fails, wled->ovp_irq retains the negative error
code, and wled_configure_ovp_irq() returns 0.

Later, during unbind:

drivers/video/backlight/qcom-wled.c:wled_remove() {
    ...
    disable_irq(wled->short_irq);
    disable_irq(wled->ovp_irq);
    ...
}

Does this unconditional disable_irq() risk passing an unallocated negative
value (or 0, if the threaded IRQ request fails and sets it to 0),
potentially crippling system IRQ 0?

[ ... ]
> @@ -1742,9 +1755,6 @@ static int wled_probe(struct platform_device *pdev)
>  	if (rc < 0)
>  		return rc;
>  
> -	val = WLED_DEFAULT_BRIGHTNESS;
> -	of_property_read_u32(pdev->dev.of_node, "default-brightness", &val);
> -
>  	memset(&props, 0, sizeof(struct backlight_properties));
>  	props.type = BACKLIGHT_RAW;
>  	props.brightness = val;

[Severity: Critical]
This is a pre-existing regression, but I noticed wled_probe() successfully
registers the backlight device and returns:

drivers/video/backlight/qcom-wled.c:wled_probe() {
    ...
    bl = devm_backlight_device_register(&pdev->dev, wled->name,
                                        &pdev->dev, wled,
                                        &wled_ops, &props);
    return PTR_ERR_OR_ZERO(bl);
}

Since platform_set_drvdata() is never called in the probe path, won't
platform_get_drvdata(pdev) return NULL in wled_remove()?

drivers/video/backlight/qcom-wled.c:wled_remove() {
    struct wled *wled = platform_get_drvdata(pdev);
    
    mutex_destroy(&wled->lock);
    ...
}

Could this lead to a guaranteed NULL pointer dereference when the device
is unbound?

[Severity: High]
This is another pre-existing regression, but looking at the error paths at
the end of wled_probe():

If wled_configure_short_irq() succeeds, it registers and enables the
short IRQ immediately. If a subsequent step like wled_configure_ovp_irq() or
devm_backlight_device_register() fails, wled_probe() returns an error code:

drivers/video/backlight/qcom-wled.c:wled_probe() {
    ...
    rc = wled_configure_ovp_irq(wled, pdev);
    if (rc < 0)
        return rc;
    ...
}

If the hardware asserts the short IRQ concurrently during this failure,
wled_short_irq_handler() will run and schedule the delayed work:

drivers/video/backlight/qcom-wled.c:wled_short_irq_handler() {
    ...
    rc = wled_module_enable(wled, true);
    ...
}

Since there is no cancel_delayed_work_sync() on the probe error path,
will this result in a use-after-free when devres cleans up and frees the
wled struct while the delayed work is still queued?

[Severity: High]
This isn't a regression introduced by this patch, but looking at the
teardown sequence in wled_remove():

drivers/video/backlight/qcom-wled.c:wled_remove() {
    struct wled *wled = platform_get_drvdata(pdev);

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

If the short IRQ fires concurrently before disable_irq() is called, could
the IRQ handler attempt to acquire the destroyed mutex?

Additionally, since wled_short_irq_handler() unconditionally re-enables the
module, could it asynchronously re-arm wled->ovp_work after the cancellation
has already finished, leading to a use-after-free when the struct is freed?

[Severity: High]
This is also a pre-existing regression, but I noticed
wled_short_irq_handler() doesn't seem to check if the backlight was
intentionally disabled:

drivers/video/backlight/qcom-wled.c:wled_short_irq_handler() {
    ...
    msleep(WLED_SHORT_DLY_MS);
    rc = wled_module_enable(wled, true);
    ...
}

If userspace intentionally disables the backlight via wled_update_status()
by setting brightness to 0, and a short circuit IRQ fires concurrently or
shortly after, wouldn't the handler force the hardware back on?

Does this leave the hardware enabled indefinitely while the software state
considers it off, ignoring further requests to set brightness to 0?

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