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

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

Pre-existing issues:
- [High] The global `ls2k_bmc_cells` array is mutated during probe to pass a pointer to a stack-allocated variable (`&pd`), causing an MFD subsystem violation and a stack use-after-free race condition.
- [High] Missing bounds check in `ls2k_bmc_parse_mode` allows the calculated framebuffer size to exceed the fixed 4MB hardware resource.
- [High] The interrupt handler uses a `static unsigned long last_jiffies` for rate-limiting, sharing state across all devices and breaking interrupts for the first 5 minutes of system uptime due to integer underflow.
- [High] The `ls2k_bmc_init` function unconditionally calls `acpi_unregister_gsi()` on the success path, leading to an IRQ mapping use-after-free and silencing hardware interrupts.
--

--- Patch [1]: [PATCH RFC v8 1/8] mfd: ls2kbmc: Make a copy when parsing mode string ---
commit bb986d2e7f482a92c234ff015a64e640a621dc2d
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.

> diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> index 408056bfb2fe7..335590392240b 100644
> --- a/drivers/mfd/ls2k-bmc-core.c
> +++ b/drivers/mfd/ls2k-bmc-core.c
[ ... ]
> @@ -427,34 +427,67 @@ static int ls2k_bmc_init(struct ls2k_bmc_ddata *ddata)
>   */
>  static int ls2k_bmc_parse_mode(struct pci_dev *pdev, struct simplefb_platform_data *pd)
>  {
[ ... ]
> +	if (depth <= 0) {
> +		ret = -EINVAL;
> +		goto invalid_mode;
> +	}
> +	if (pd->width > U32_MAX / depth) {
> +		ret = -EOVERFLOW;
> +		goto invalid_mode;
> +	}
>  
>  	pd->stride = pd->width * depth / 8;
>  	pd->format = depth == 32 ? "a8r8g8b8" : "r5g6b5";

[Severity: High]
This is a pre-existing issue, but is there a missing bounds check against
the maximum framebuffer size?

The framebuffer resource ls2k_display_resources is statically defined with
a size of SZ_4M. While the code checks for integer overflow of width * depth,
could a compromised or misconfigured BMC provide a mode string that results
in a stride * height exceeding the 4MB limit?

If simple-framebuffer maps this region restricted to 4MB, fbcon might rely
on this geometry and attempt out-of-bounds writes beyond the 4MB ioremap
boundary, which could trigger a page fault and panic.

[ ... ]

While reviewing this file, I also noticed a few other pre-existing issues
outside of the modified lines:

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

[Severity: High]
This isn't a bug introduced by this patch, but does the global
ls2k_bmc_cells array have a race condition during probe?

Mutating a shared static global array during probe causes data races if
multiple devices are probed concurrently.

Also, storing a pointer to a local stack variable (pd) in a global array
means a concurrent probe could dereference a dangling stack pointer. The MFD
subsystem requires cell arrays to be static const to prevent this.


drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_interrupt() {
    static unsigned long last_jiffies;
    ...
    if (system_state != SYSTEM_RUNNING)
        return IRQ_HANDLED;

    /* Skip interrupt in LS2K_BMC_INT_INTERVAL */
    if (time_after(jiffies, last_jiffies + LS2K_BMC_INT_INTERVAL)) {
        ...
    }
}

[Severity: High]
This is a pre-existing issue, but does the interrupt rate-limiting logic
fail during early boot?

Using a static variable inside an ISR incorrectly shares the debounce state
across all instances of the device.

Additionally, last_jiffies implicitly initializes to 0. Since the system
jiffies counter starts at a large negative value during early boot,
time_after() will evaluate to false until jiffies wraps around past 0.

Could this cause BMC reset interrupts to be completely ignored for the first
5 minutes of system uptime?


drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_init() {
    ...
    ret = devm_request_irq(ddata->dev, gpio_irq, ls2k_bmc_interrupt,
                           IRQF_SHARED | IRQF_TRIGGER_FALLING,
                           "ls2kbmc gpio", ddata);
    if (ret)
        dev_err(ddata->dev, "Failed to request LS2KBMC GPIO IRQ %d.\n",
                gpio_irq);

    acpi_unregister_gsi(LS2K_BMC_RESET_GPIO_GSI);
    return ret;
}

[Severity: High]
This is a pre-existing issue, but does ls2k_bmc_init() accidentally destroy
the active IRQ mapping on the success path?

Calling acpi_unregister_gsi() on the success path disposes of the mapping
between the hardware GSI and the assigned Linux IRQ number.

Since the domain mapping is destroyed, will the interrupt controller fail
to deliver interrupts to the driver, breaking the BMC reset functionality?

Furthermore, when the device is removed, the managed IRQ framework will
attempt to free the disposed IRQ mapping, which could lead to use-after-free
warnings in the irqdomain code.

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