Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues: - [High] Hardware over-voltage protection (OVP) interrupt is incorrectly disabled if the module is active but software evaluates brightness as 0, leaving the hardware unprotected. Pre-existing issues: - [Critical] Type mismatch in driver data leads to memory corruption and kernel panic in wled_remove(). - [High] AB-BA Deadlock between `wled_update_status` and `wled_ovp_irq_handler` via `disable_irq()`. - [High] Unsafe `disable_irq()` calls in `wled_remove()` with potentially invalid or uninitialized IRQ numbers. - [High] Destroying the driver mutex and cancelling work before unregistering the backlight device creates a use-after-destroy race condition via sysfs. - [High] Delayed work `wled->ovp_work` can be scheduled during device probe but is not cancelled on probe error paths, causing a Use-After-Free. -- commit fc9aa7b480cc714e361a97ee0c2edec0701360bf Author: David Heidelberg <[email protected]> backlight: qcom-wled: Fix unbalanced OVP IRQ enable at probe This commit fixes an issue where the OVP interrupt is disabled during probe even if the backlight is already enabled by the bootloader. It does this by checking the hardware MOD_EN register during probe and seeding wled->brightness appropriately. > diff --git a/drivers/video/backlight/qcom-wled.c > b/drivers/video/backlight/qcom-wled.c > index e1962c2d90ed4..eb742a6598173 100644 > --- a/drivers/video/backlight/qcom-wled.c > +++ b/drivers/video/backlight/qcom-wled.c > @@ -1643,13 +1642,8 @@ static int wled_configure_ovp_irq(struct wled *wled, > return 0; > } > > - rc = regmap_read(wled->regmap, wled->ctrl_addr + > - WLED3_CTRL_REG_MOD_EN, &val); > - if (rc < 0) > - return rc; > - > - /* Keep OVP irq disabled until module is enabled */ > - if (!(val & WLED3_CTRL_REG_MOD_EN_MASK)) > + /* Keep the OVP irq disabled until the module is enabled */ > + if (!wled->brightness) > disable_irq(wled->ovp_irq); [Severity: High] Does this leave the hardware unprotected if the module is active but software evaluates brightness as 0? If wled4_setup() explicitly sets MOD_EN to 1, and the initial brightness evaluates to 0 (either via default-brightness or wled_read_brightness), wled->brightness becomes 0. Could this cause disable_irq() to strip protection from the active module? Furthermore, if userspace later writes 0 to turn off the screen, could wled_update_status() check (0 != 0), skip wled_module_enable(0), and leave the unprotected hardware permanently on? [ ... ] > @@ -1734,6 +1729,24 @@ static int wled_probe(struct platform_device *pdev) > > INIT_DELAYED_WORK(&wled->ovp_work, wled_ovp_work); > > + val = WLED_DEFAULT_BRIGHTNESS; > + of_property_read_u32(pdev->dev.of_node, "default-brightness", &val); > + > + /* > + * The module may already be enabled, either by a bootloader that left > + * the backlight lit or by the setup above. Record that, so that the > + * first brightness update does not enable an already enabled module, > + * and so that the OVP irq is armed from probe rather than from that > + * first update. > + */ > + rc = regmap_read(wled->regmap, wled->ctrl_addr + WLED3_CTRL_REG_MOD_EN, > + &mod_en); > + if (rc < 0) > + return rc; > + > + if (mod_en & WLED3_CTRL_REG_MOD_EN_MASK) > + wled->brightness = val; ^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] If default-brightness evaluates to 0, will wled->brightness become 0 here despite MOD_EN being active? [Severity: Critical] This is a pre-existing issue, but does a type mismatch in the driver data lead to memory corruption in wled_remove()? wled_probe() incorrectly sets the platform drvdata to the backlight device: drivers/video/backlight/qcom-wled.c:wled_probe() { ... bl = devm_backlight_device_register(...); platform_set_drvdata(pdev, bl); return PTR_ERR_OR_ZERO(bl); } wled_remove() retrieves this pointer and incorrectly casts it to a struct wled *: drivers/video/backlight/qcom-wled.c:wled_remove() { struct wled *wled = platform_get_drvdata(pdev); mutex_destroy(&wled->lock); ... } Since struct backlight_device has a completely different memory layout than struct wled, does this type confusion corrupt arbitrary memory when unbinding? [Severity: High] This is a pre-existing issue, but does destroying the mutex and cancelling work before unregistering the backlight device create a use-after-destroy race condition? The backlight device is registered using devm_backlight_device_register(), which defers unregistration until after wled_remove() completes. However, wled_remove() manually performs cleanup while the backlight device is still active and its sysfs interface is exposed: drivers/video/backlight/qcom-wled.c:wled_remove() { ... mutex_destroy(&wled->lock); cancel_delayed_work_sync(&wled->ovp_work); ... } Could a concurrent sysfs write to the brightness attribute invoke wled_update_status() and attempt to acquire the already-destroyed mutex? drivers/video/backlight/qcom-wled.c:wled_update_status() { ... mutex_lock(&wled->lock); ... } [Severity: High] This is a pre-existing issue, but could a delayed work use-after-free occur if probe fails? If the hardware asserts a short circuit condition early, the threaded short IRQ handler can execute and schedule wled->ovp_work: drivers/video/backlight/qcom-wled.c:wled_module_enable() { ... schedule_delayed_work(&wled->ovp_work, HZ / 100); ... } If probe subsequently fails during backlight device registration: drivers/video/backlight/qcom-wled.c:wled_probe() { ... bl = devm_backlight_device_register(...); platform_set_drvdata(pdev, bl); return PTR_ERR_OR_ZERO(bl); } Will devres silently free the wled structure without canceling the scheduled delayed work, causing a crash when the workqueue later executes? [Severity: High] This is a pre-existing issue, but can this sequence lead to an AB-BA deadlock? wled_update_status() acquires the mutex before attempting to disable the module: drivers/video/backlight/qcom-wled.c:wled_update_status() { ... mutex_lock(&wled->lock); ... rc = wled_module_enable(wled, !!brightness); ... } wled_module_enable() spin-waits for any currently executing OVP interrupt handlers to finish while holding the mutex: drivers/video/backlight/qcom-wled.c:wled_module_enable() { ... disable_irq(wled->ovp_irq); ... } Concurrently, if the OVP interrupt fires on another CPU, the handler begins executing and blocks attempting to acquire the same mutex: drivers/video/backlight/qcom-wled.c:wled_ovp_irq_handler() { ... mutex_lock(&wled->lock); ... } Would CPU 1 hold the lock and wait for the IRQ handler, while CPU 2 runs the handler and waits for the lock? [Severity: High] This is a pre-existing issue, but does wled_remove() unconditionally disable potentially invalid IRQs? If devm_request_threaded_irq() fails, wled_configure_ovp_irq() explicitly sets ovp_irq to 0: drivers/video/backlight/qcom-wled.c:wled_configure_ovp_irq() { ... if (rc < 0) { wled->ovp_irq = 0; return 0; ... } Additionally, platform_get_irq_byname() can return negative error codes. However, wled_remove() appears to disable both IRQs without validation: drivers/video/backlight/qcom-wled.c:wled_remove() { ... disable_irq(wled->short_irq); disable_irq(wled->ovp_irq); ... } Could passing a negative value cause out-of-bounds descriptor lookups, and could passing 0 disable an essential system interrupt like the timer IRQ? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
