Re: [PATCH RFC v4 3/7] mfd: ls2kbmc: Redraw using exported functions

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

Pre-existing issues:
- [High] The global static `mfd_cell` array is dynamically mutated with a stack pointer, violating MFD rules and introducing a data race.
- [High] The `bmc_reset_work` workqueue item is never canceled on driver unbind, leading to a Use-After-Free.
- [High] `acpi_unregister_gsi()` is unconditionally called immediately after successfully requesting the GPIO IRQ, destroying the mapping.
- [High] The workqueue ignores the failure of PCI-E recovery and proceeds to update the screen unconditionally, which can cause a kernel crash.
- [Medium] Untrusted resolution parameters read from the BMC PCI memory can cause an integer overflow when calculating the framebuffer stride.
--

--- Patch [3]: [PATCH RFC v4 3/7] mfd: ls2kbmc: Redraw using exported functions ---
commit 8920bccc1429d53315723d84c6b17a6efd535a5b
Author: Miao Wang <[email protected]>

mfd: ls2kbmc: Redraw using exported functions
    
Use update_screen, i.e. redraw_screen() to trigger the redraw of the
current vt.

Fixes: d952bba3fbb5 ("mfd: ls2kbmc: Add Loongson-2K BMC reset function support")

> diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> index b02e4955e9b04..a5465c42a77b8 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);

[Severity: High]
This isn't a bug introduced by this patch, but does this blindly access the
framebuffer without ensuring the pcie link is actually connected?

Looking at ls2k_bmc_recover_pci_data() in drivers/mfd/ls2k-bmc-core.c, the
failure paths return false (which evaluates to 0, or success). This masks the
failure from stop_machine(). Then in ls2k_bmc_events_fn(), the return value
of stop_machine() is entirely ignored.

If the pcie link recovery fails, won't calling update_screen() here access
device memory over a disconnected pcie link, triggering a master abort and
leading to a fatal kernel panic?

> +		console_unlock();
>  	}
>  }

[Severity: High]
This is a pre-existing issue, but does this driver mutate a global mfd_cell
array with a stack pointer?

In drivers/mfd/ls2k-bmc-core.c, ls2k_bmc_probe() modifies the shared global
array:

ls2k_bmc_probe() {
    ...
    ls2k_bmc_cells[LS2K_BMC_DISPLAY].platform_data = &pd;
    ...
}

If multiple device instances are probed concurrently, won't they corrupt each
other's pointers in the global array ls2k_bmc_cells? Furthermore, after the
probe function returns, doesn't the global array retain a dangling pointer to
the destroyed stack frame? The mfd subsystem also requires that mfd_cell
arrays must be declared as static const.

[Severity: High]
This isn't a bug introduced by this patch, but is the bmc_reset_work workqueue
item ever canceled on driver unbind?

The work struct is initialized in ls2k_bmc_init() in
drivers/mfd/ls2k-bmc-core.c:

    INIT_WORK(&ddata->bmc_reset_work, ls2k_bmc_events_fn);

However, the pci_driver struct ls2k_bmc_driver lacks a .remove callback, and
there is no devm_add_action() hook to call cancel_work_sync().

If the work is scheduled just before the interrupt is freed during device
unbind, wouldn't the workqueue execute ls2k_bmc_events_fn() and dereference
the already freed ddata, causing a use-after-free?

[Severity: High]
This is a pre-existing issue, but is the active interrupt mapping
unconditionally destroyed during initialization?

In ls2k_bmc_init() in drivers/mfd/ls2k-bmc-core.c, acpi_unregister_gsi() is
called unconditionally after requesting the interrupt:

    acpi_unregister_gsi(LS2K_BMC_RESET_GPIO_GSI);
    return ret;

Does this execute irq_dispose_mapping() and tear down the interrupt mapping
while the interrupt is still actively requested, rendering it completely
non-functional?

[Severity: Medium]
This isn't a bug introduced by this patch, but could untrusted resolution
parameters read from the bmc pci memory cause an integer overflow?

In ls2k_bmc_parse_mode() in drivers/mfd/ls2k-bmc-core.c, the parameters are
read and used in 32-bit arithmetic:

    pd->stride = pd->width * depth / 8;

Since the resolution string is parsed using kstrtouint() without bounds
checking, couldn't large input values (such as width = 0x40000000) overflow
the multiplication? This would result in a tiny or zero stride being passed
to the simplefb driver.

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