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

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

Pre-existing issues:
- [Critical] Lack of upper bound validation on framebuffer height allows arbitrary out-of-bounds MMIO writes.
- [High] Missing workqueue cancellation causes a Use-After-Free of `ddata` on driver unbind.
- [High] Execution of a 10-second busy-loop and sleeping functions while local interrupts are disabled via `stop_machine()`, causing system lockups.
- [High] The `mfd_cell` array is not `static const` and is dynamically mutated during probe, violating MFD guidelines and creating data races.
- [High] Unsynchronized static IRQ throttling state causes a total loss of interrupts for 5 minutes after boot.
--

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

mfd: ls2kbmc: Redraw using exported functions

This commit updates the ls2kbmc driver to use standard console functions.
It replaces the manual virtual terminal manipulation with console locking
and the update_screen() function.

> 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
> @@ -25,6 +25,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/stop_machine.h>
>  #include <linux/vt_kern.h>
> +#include <linux/console.h>
>  
[ ... ]
> @@ -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 isn't a bug introduced by this patch, but is it possible for
bmc_reset_work to execute after the ddata structure is freed?

The driver initializes bmc_reset_work without a corresponding unbind
cancellation step:

ls2k_bmc_init() {
    ...
    INIT_WORK(&ddata->bmc_reset_work, ls2k_bmc_events_fn);
    ...
}

Since there is no driver remove callback or managed action to cancel the
work, devres will free the IRQ and then ddata on unbind. If the work is
concurrently executing, can it access the freed ddata here in
ls2k_bmc_events_fn()?

ls2k_bmc_events_fn() {
    struct ls2k_bmc_ddata *ddata = container_of(work, ...);
    ...
    stop_machine(ls2k_bmc_recover_pci_data, ddata, NULL);
}

[Severity: High]
This is a pre-existing issue, but does executing stop_machine() here
cause system lockups?

ls2k_bmc_events_fn() {
    ...
    stop_machine(ls2k_bmc_recover_pci_data, ddata, NULL);
    ...
}

The callback runs with local interrupts disabled across all CPUs.
Inside this atomic context, the callback spins for 10 seconds:

ls2k_bmc_recover_pci_data() {
    ...
    mdelay(LS2K_BMC_RESET_WAIT_TIME);
    ...
}

It also invokes mapping operations that can sleep:

ls2k_bmc_pcie_is_connected() {
    ...
    base = pci_iomap(parent, 0, LS7A_PCI_CFG_SIZE);
    ...
}

Can this combination of mdelay and sleeping functions inside
stop_machine() lead to deadlocks or watchdog panics?

[Severity: High]
This is a pre-existing issue, but does mutating ls2k_bmc_cells during
probe create a data race and a dangling pointer?

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

Since ls2k_bmc_cells is a shared global array and missing the const
qualifier:

static struct mfd_cell ls2k_bmc_cells[] = { ... };

Assigning the address of a local stack variable (&pd) to platform_data
leaves a dangling pointer once probe returns. Concurrent probes could
also overwrite this shared global state. Could this be updated to use
static const cell definitions to follow MFD guidelines?

[Severity: High]
This is a pre-existing issue, but does the interrupt handler drop events
for the first 5 minutes after boot?

ls2k_bmc_interrupt() {
    static unsigned long last_jiffies;
    ...
    if (time_after(jiffies, last_jiffies + LS2K_BMC_INT_INTERVAL)) {
        schedule_work(&ddata->bmc_reset_work);
        last_jiffies = jiffies;
    }
    ...
}

At boot, jiffies initializes to a large negative number. Since
last_jiffies is zero-initialized, time_after() evaluates to false
until jiffies crosses zero, which takes about 5 minutes.

Additionally, since last_jiffies is static and unsynchronized, could
multiple device instances corrupt this shared throttling state?

[Severity: Critical]
This is a pre-existing issue, but does the lack of upper bound
validation on the framebuffer height allow arbitrary out-of-bounds
memory writes?

ls2k_bmc_parse_mode() {
    ...
    ret = kstrtouint(strsep(&mode, "-"), 10, &pd->height);
    if (ret)
        goto invalid_mode;
    ...
    pd->stride = pd->width * depth / 8;
    pd->format = depth == 32 ? "a8r8g8b8" : "r5g6b5";
    ...
}

The height is parsed from an untrusted string without an upper limit
check against the 4MB maximum resource size.

Since stride and height remain unbounded, the simplefb driver will
configure a framebuffer using this huge geometry.

When fbcon writes text to the console, can this calculate memory offsets
that write far beyond the 4MB ioremap region?

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