Re: [PATCH RFC v3 1/7] mfd: ls2kbmc: Make a copy when parsing mode string

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

New issues:
- [High] The newly added `mode == NULL` check for the depth token strictly rejects valid mode strings that do not contain an `@` character (refresh rate).

Pre-existing issues:
- [High] The static global `mfd_cell` array is dynamically mutated during probe using a local stack pointer, causing a data race and violating MFD guidelines.
- [High] Missing NULL check for the parent PCI device leads to a guaranteed kernel crash on VM passthrough or root bus topologies.
- [High] Integer overflow during framebuffer stride calculation allows a maliciously crafted stride of zero.
- [High] The shared interrupt handler unconditionally returns `IRQ_HANDLED` without checking if the interrupt actually originated from its device.
- [High] The driver executes extremely long busy-waits (e.g., `mdelay(10000)`) inside a `stop_machine()` callback, which runs in an atomic context with interrupts disabled on all CPUs.
--

--- Patch [1]: [PATCH RFC v3 1/7] mfd: ls2kbmc: Make a copy when parsing mode string ---
commit 84c24ac01fbdd9a196a1f9f0c91b270f741fb568
Author: Miao Wang <[email protected]>

mfd: ls2kbmc: Make a copy when parsing mode string

When parsing the mode string from BMC, the string is manipulated
in-place with strsep(), preventing from parsing it again. Make a copy of
the original string and manipulate the copy instead to fix this.

Fixes: 0d64f6d1ffe9 ("mfd: ls2kbmc: Introduce Loongson-2K BMC core driver")
Signed-off-by: Miao Wang <[email protected]>

> diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> index 408056bfb2fe7..27f6e096404d6 100644
> --- a/drivers/mfd/ls2k-bmc-core.c
> +++ b/drivers/mfd/ls2k-bmc-core.c
> @@ -427,34 +427,54 @@ static int ls2k_bmc_init(struct ls2k_bmc_ddata *ddata)
[ ... ]
>  	ret = kstrtoint(strsep(&mode, "@"), 10, &depth);
>  	if (ret)
>  		return ret;
> +	if (mode == NULL)
> +		goto invalid_mode;

[Severity: High]
Does this new check break backward compatibility?

If the resolution string doesn't contain a refresh rate (e.g.
"video=1920x1080-32"), strsep() will return the remaining string for depth,
and set mode to NULL.

The kstrtoint() succeeds, but then the new "if (mode == NULL)" check will
trigger, unconditionally rejecting valid mode strings without an explicit
refresh rate.

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

[Severity: High]
This is a pre-existing issue, but does this calculation check for integer
overflow?

If a compromised BMC hardware endpoint supplies maliciously large values
(e.g., width = 0x40000000 and depth = 8), this calculation could wrap
around to 0, resulting in a zero-size framebuffer mapping.


I also noticed some pre-existing issues in other parts of the driver:

[Severity: High]
This is a pre-existing issue, but does ls2k_bmc_probe() dynamically mutate a
global static array during probe?

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);
    ...
}

Assigning a local stack pointer to a mutable global static MFD cell array
creates a data race and a use-after-free hazard if multiple instances of
this driver probe concurrently.


[Severity: High]
This is a pre-existing issue, but is it possible for the parent pointer to
be NULL in ls2k_bmc_save_pci_data()?

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_save_pci_data() {
    struct pci_dev *parent = pdev->bus->self;
    ...
    pci_read_config_dword(parent, PCI_COMMAND, &ddata->bridge_pci_data.pci_command);
    ...
}

If the device is connected directly to a root bus (e.g., passed through via
VFIO to a VM), pdev->bus->self will be NULL, which would cause a kernel
panic due to a NULL pointer dereference.


[Severity: High]
This is a pre-existing issue, but does the shared interrupt handler check
the hardware to confirm the interrupt's provenance?

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_interrupt() {
    ...
    if (time_after(jiffies, last_jiffies + LS2K_BMC_INT_INTERVAL)) {
        schedule_work(&ddata->bmc_reset_work);
    ...
    return IRQ_HANDLED;
}

Because it is registered with IRQF_SHARED, if another device on the same
PCI IRQ line fires an interrupt, the kernel assumes this driver handled it,
potentially starving the other device and spuriously triggering a BMC reset.


[Severity: High]
This is a pre-existing issue, but does this code execute an extremely long
busy-wait in an atomic context?

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

The BMC reset workqueue calls stop_machine(ls2k_bmc_recover_pci_data, ...).
Inside this callback, mdelay() busy-waits for 10 seconds. Since
stop_machine() runs in an atomic context with interrupts disabled on all
CPUs, this can cause a system freeze and a hard lockup/NMI watchdog panic.

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