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

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

Pre-existing issues:
- [High] The driver dynamically modifies a shared static `mfd_cell` array with a stack-allocated platform data pointer during probe.
- [High] The driver completely freezes the system for over 10 seconds during BMC reset recovery by busy-waiting inside a stop_machine() callback.
- [High] Missing cancellation of workqueue leads to use-after-free on device unbind.
- [High] The driver dynamically maps and unmaps device memory inside a stop_machine() callback, which executes in atomic context with interrupts disabled.
- [High] The driver bypasses GPIO APIs to perform lockless read-modify-write operations on shared SoC-level GPIO controller registers.
- [High] The interrupt handler unconditionally returns IRQ_HANDLED on a shared interrupt line and locklessly accesses a static rate-limiting variable.
--

--- Patch [2]: [PATCH RFC v7 2/8] mfd: ls2kbmc: Sanity check for the connected pci port ---
commit 0143b961f7353bf1b0c1d9d60cca1be9f0e57e23
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 */
[ ... ]
> @@ -490,6 +499,24 @@ static int ls2k_bmc_parse_mode(struct pci_dev *pdev, struct simplefb_platform_da
>  	return ret;
>  }
>  
> +static const struct pci_device_id ls7a_ports[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A1K_PCIE_PORT0) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A1K_PCIE_PORT1) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A1K_PCIE_PORT2) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A2K_PCIE_PORT0) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A2K_PCIE_PORT1) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A2K_PCIE_PORT2) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, DEV_LS7A2K_PCIE_PORT3) },
> +	{ }
> +};
> +
> +static bool ls2k_check_parent(struct pci_dev *dev)
> +{
> +	struct pci_dev *parent = dev->bus->self;
> +
> +	return parent && pci_match_id(ls7a_ports, parent) != NULL;
> +}
> +
>  static int ls2k_bmc_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  {
>  	struct simplefb_platform_data pd;
> @@ -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;

I also noticed some pre-existing issues in this file:

drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_probe() {
    ...
	ret = ls2k_bmc_parse_mode(dev, &pd);
	if (ret)
		return ret;

	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 this dynamically modify a
shared static mfd_cell array with a local stack pointer?

The MFD subsystem API requires mfd_cell arrays to be static const.

Modifying the global array with a pointer to the local stack-allocated "pd"
variable could cause a stack use-after-free during asynchronous or parallel
probing.


drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_events_fn() {
    ...
	stop_machine(ls2k_bmc_recover_pci_data, ddata, NULL);
    ...
}

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

[Severity: High]
This isn't a bug introduced by this patch, but is this a 10-second busy loop?

Since ls2k_bmc_recover_pci_data() runs under stop_machine() via
ls2k_bmc_events_fn(), won't stalling all CPUs with interrupts disabled for
10 seconds trigger the NMI hard lockup watchdog and freeze the system?


drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_init() {
    ...
	INIT_WORK(&ddata->bmc_reset_work, ls2k_bmc_events_fn);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but should this asynchronous workqueue
be canceled on driver removal?

There is no cancel_work_sync() in the device removal path or a managed devm
action.

This means an interrupt could schedule this work right as the device is
unbound, leading to a use-after-free when the work executes after ddata is
freed.


drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_pcie_is_connected() {
    ...
	base = pci_iomap(parent, 0, LS7A_PCI_CFG_SIZE);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but can pci_iomap() dynamically map
device memory using sleeping functions here?

This function is called from ls2k_bmc_recover_pci_data() which executes under
stop_machine() with interrupts disabled.

Could this lead to a panic due to scheduling while atomic?


drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_init() {
    ...
	/* Disable GPIO output */
	val = readl(gpio_base + LOONGSON_GPIO_OEN);
	writel(val | BIT(LS2K_BMC_RESET_GPIO), gpio_base + LOONGSON_GPIO_OEN);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but is this code performing an
unprotected read-modify-write on shared SoC-level GPIO controller registers?

Bypassing GPIO APIs to directly modify global registers could race with the
primary GPIO driver and corrupt configurations for other pins.


drivers/mfd/ls2k-bmc-core.c:ls2k_bmc_init() {
    ...
	ret = devm_request_irq(&pdev->dev, pdev->irq, ls2k_bmc_interrupt,
			       IRQF_SHARED | IRQF_TRIGGER_FALLING, "ls2kbmc pcie", ddata);
    ...
}

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

	return IRQ_HANDLED;
}

[Severity: High]
This isn't a bug introduced by this patch, but does this interrupt handler
unconditionally claim shared interrupts without checking the hardware status?

Because this interrupt is requested with IRQF_SHARED, unconditionally
returning IRQ_HANDLED could cause every interrupt from other shared devices to
trigger a 10-second system freeze, and it defeats spurious interrupt detection.

Additionally, the static last_jiffies is modified locklessly.

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