Re: [PATCH RFC v8 2/8] mfd: ls2kbmc: Sanity check for the connected pci port

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

Pre-existing issues:
- [Critical] System stall for 10+ seconds with interrupts disabled due to `stop_machine` delay.
- [Critical] Unconditional remapping of hardcoded physical address allows arbitrary physical memory corruption.
- [High] Dynamic mutation of global static MFD cell array leaving a dangling pointer.
- [High] GPIO GSI mapping is unconditionally destroyed immediately after being successfully requested.
- [Medium] Flawed rate limiting logic ignores interrupts for the first 5 minutes of system uptime and incorrectly shares state.
--

--- Patch [2]: [PATCH RFC v8 2/8] mfd: ls2kbmc: Sanity check for the connected pci port ---
commit 9113421fc798a2bad757ae8e5bd2b98d1ed9d8bb
Author: Miao Wang <[email protected]>

mfd: ls2kbmc: Sanity check for the connected pci port

When the bmc resets, the recovery procedure require to reconfigure the
parent device. The driver assumes that the parent device should be LS7A.
Add a sanity check on initialization to ensure this and prevent from
accidentally operating on non-LS7A ports.

> diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> index 335590392240b..434db99ee501b 100644
> --- a/drivers/mfd/ls2k-bmc-core.c
> +++ b/drivers/mfd/ls2k-bmc-core.c
> @@ -35,6 +35,15 @@
>  #define LS2K_IPMI3_RES_START		(LS2K_IPMI2_RES_START + LS2K_IPMI_RES_SIZE)
>  #define LS2K_IPMI4_RES_START		(LS2K_IPMI3_RES_START + LS2K_IPMI_RES_SIZE)
>  
> +/* LS7A port Device IDs */
> +#define DEV_LS7A1K_PCIE_PORT0	0x7a09
> +#define DEV_LS7A1K_PCIE_PORT1	0x7a19
> +#define DEV_LS7A1K_PCIE_PORT2	0x7a29
> +#define DEV_LS7A2K_PCIE_PORT0	0x7a39
> +#define DEV_LS7A2K_PCIE_PORT1	0x7a49
> +#define DEV_LS7A2K_PCIE_PORT2	0x7a59
> +#define DEV_LS7A2K_PCIE_PORT3	0x7a69
> +
>  #define LS7A_PCI_CFG_SIZE		0x100
>  
>  /* LS7A bridge registers */

[ ... ]

> @@ -501,6 +528,11 @@ static int ls2k_bmc_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	if (ret)
>  		return ret;
>  
> +	if (!ls2k_check_parent(dev)) {
> +		dev_err(&dev->dev, "Expected to be connected to LS7A PCI-E port\n");
> +		return -ENODEV;
> +	}
> +
>  	ddata = devm_kzalloc(&dev->dev, sizeof(*ddata), GFP_KERNEL);
>  	if (!ddata)
>  		return -ENOMEM;

[Severity: High]
This isn't a bug introduced by this patch, but looking at ls2k_bmc_probe()
further down, is it safe to assign a local stack variable to the global
MFD cell array?

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

This leaves a dangling pointer in the global ls2k_bmc_cells array when the
function returns. Additionally, the ls2k_bmc_cells array is dynamically
mutated but not defined as static const, which violates MFD subsystem
guidelines. Can this cause memory corruption if the pointer is reused or
if multiple devices are probed?

[Severity: Critical]
This is a pre-existing issue, but I noticed a potentially severe stall in
the reset recovery path. Does this code stall the system with interrupts
disabled for over 10 seconds?

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

Inside the callback:

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

Since stop_machine() halts all CPUs with local interrupts disabled, wouldn't
a 10-second delay (10,000 ms) trigger NMI hard lockups, RCU stalls, and
system watchdog resets?

[Severity: Critical]
This isn't a bug introduced by this patch, but is it safe to hardcode and
unconditionally map a physical address during initialization?

ls2k_bmc_init() {
...
    gpio_base = ioremap(LOONGSON_GPIO_REG_BASE, LOONGSON_GPIO_REG_SIZE);
...
}

Where LOONGSON_GPIO_REG_BASE is 0x1FE00500. Even with the new PCI ID check,
could an attacker with a spoofed PCIe device trigger this init function
and overwrite host physical memory at this hardcoded address?

[Severity: High]
This is a pre-existing issue, but does this cleanup logic incorrectly destroy
the GSI mapping on the probe success path?

ls2k_bmc_init() {
...
    gpio_irq = acpi_register_gsi(NULL, LS2K_BMC_RESET_GPIO_GSI, ...);
...
    ret = devm_request_irq(ddata->dev, gpio_irq, ls2k_bmc_interrupt, ...);
...
    acpi_unregister_gsi(LS2K_BMC_RESET_GPIO_GSI);
    return ret;
}

Since acpi_unregister_gsi() is called unconditionally, won't this silence
the GPIO interrupt entirely even when initialization succeeds?

[Severity: Medium]
This isn't a bug introduced by this patch, but could this rate limiting logic
ignore interrupts for the first 5 minutes of system uptime?

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

Since last_jiffies is initialized to 0, and jiffies initializes to -300*HZ,
time_after() will evaluate to false for the first ~5 minutes. Also, does
using a static variable incorrectly share this state across all potential
device instances?

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