Re: [PATCH RFC v8 3/8] mfd: ls2kbmc: Redraw using exported functions

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

Pre-existing issues:
- [High] System lockup due to 10-second busy-wait inside stop_machine() context.
- [High] MFD subsystem API violation leading to a data race and dangling pointer on a global cell array.
- [High] Use-After-Free of device data due to missing workqueue cleanup on device removal or probe failure.
- [High] Unbalanced resource lifecycle completely breaks the GPIO interrupt delivery.
--

--- Patch [3]: [PATCH RFC v8 3/8] mfd: ls2kbmc: Redraw using exported functions ---
commit 1bc6218749af861d9fbbf175e51a2eba62372b2b
Author: Miao Wang <[email protected]>

mfd: ls2kbmc: Redraw using exported functions

This commit updates the ls2kbmc driver to trigger the redraw of the current
vt using update_screen() inside console_lock() instead of set_console().

> diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> index 434db99ee501b..06ec8143d8cdb 100644
> --- a/drivers/mfd/ls2k-bmc-core.c
> +++ b/drivers/mfd/ls2k-bmc-core.c

[ ... ]

> @@ -310,7 +311,9 @@ static void ls2k_bmc_events_fn(struct work_struct *work)
>  
>  	if (IS_ENABLED(CONFIG_VT)) {
>  		/* Re-push the display due to previous PCI-E loss. */
> -		set_console(vt_move_to_console(MAX_NR_CONSOLES - 1, 1));
> +		console_lock();
> +		update_screen(vc_cons[fg_console].d);
> +		console_unlock();

[Severity: High]
This is a pre-existing issue, but can this function trigger a system lockup?

The BMC reset event schedules bmc_reset_work, which executes this
ls2k_bmc_events_fn() function. Right above this diff context, the function
calls stop_machine() to suspend all CPUs with local interrupts disabled:

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_events_fn() {
        ...
        stop_machine(ls2k_bmc_recover_pci_data, ddata, NULL);
        ...
}

Inside ls2k_bmc_recover_pci_data(), the code busy-waits for 10 seconds:

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_recover_pci_data() {
        ...
        /* Waiting for U-Boot and DDR ready */
        mdelay(LS2K_BMC_RESET_WAIT_TIME);
        ...
}

Since stop_machine() runs with interrupts disabled across all CPUs, does
spinning for 10,000 milliseconds here inevitably cause the hard and soft
lockup watchdogs to fire and crash the system?

[Severity: High]
This is a pre-existing issue, but does this driver leave a dangling pointer
in the global cell array during probe?

In ls2k_bmc_probe(), the code assigns the address of a local stack variable
to the global ls2k_bmc_cells array:

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_probe() {
        ...
        ls2k_bmc_cells[LS2K_BMC_DISPLAY].platform_data = &pd;
        ls2k_bmc_cells[LS2K_BMC_DISPLAY].pdata_size = sizeof(pd);
        ...
}

While devm_mfd_add_devices() clones the data during registration, the
global array itself is improperly defined as mutable state rather than
static const:

drivers/mfd/ls2k-bmc-core.c {
        ...
        static struct mfd_cell ls2k_bmc_cells[] = {
        ...
}

Since the global array retains the pointer to the destroyed stack frame
after the probe returns, could this result in a data race on the global
array and potential use of stack memory if multiple instances probe
concurrently or if the device is re-probed?

[Severity: High]
This is a pre-existing issue, but is there a use-after-free vulnerability
if the device is removed or if probe fails?

In ls2k_bmc_init(), the driver initializes a workqueue on a devm-managed
structure and registers an interrupt:

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_init() {
        ...
        INIT_WORK(&ddata->bmc_reset_work, ls2k_bmc_events_fn);
        ...
}

However, the driver lacks a .remove callback and does not register a devm
action to cancel the work:

drivers/mfd/ls2k-bmc-core.c {
        ...
        static struct pci_driver ls2k_bmc_driver = {
                .name = "ls2k-bmc",
                .id_table = ls2k_bmc_devices,
                .probe = ls2k_bmc_probe,
        };
        ...
}

If the driver is unbound, the devm framework will automatically free ddata.
If the shared interrupt fired and scheduled the work before teardown, could
it execute and dereference the freed ddata inside ls2k_bmc_events_fn()?

[Severity: High]
This is a pre-existing issue, but does the initialization logic break the
GPIO interrupt delivery?

In ls2k_bmc_init(), immediately following a successful IRQ registration,
the code unconditionally tears down the GSI mapping:

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_init() {
        ...
        acpi_unregister_gsi(LS2K_BMC_RESET_GPIO_GSI);
        return ret;
}

Because acpi_unregister_gsi() invokes irq_dispose_mapping() and destroys
the hardware-to-Linux IRQ mapping while the IRQ is still registered,
doesn't this prevent the interrupt from ever firing?

>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3