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

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

New issues:
- [Medium] The newly added error path logs the raw resolution string from the BMC without sanitizing it.

Pre-existing issues:
- [Critical] The `stop_machine()` callback `ls2k_bmc_recover_pci_data()` executes an extremely long `mdelay()`, freezing all CPUs.
- [High] The global `ls2k_bmc_cells` array is dynamically mutated during probe, violating MFD guidelines and creating a data race.
- [High] The shared interrupt handler `ls2k_bmc_interrupt` unconditionally returns `IRQ_HANDLED` without checking hardware status.
- [High] Unconditional dereference of `pdev->bus->self` to access the parent bridge could crash the kernel if the device is attached to the root bus.
- [High] The workqueue `bmc_reset_work` is never canceled upon device removal, leading to a Use-After-Free.
- [High] Missing validation of PCI BAR0 length before mapping memory at a 16MB offset.
- [High] Calculating the framebuffer stride can overflow a 32-bit integer, leading to out-of-bounds memory accesses.
- [High] The driver assumes its parent PCI bridge is a Loongson LS7A and blindly modifies vendor-specific registers.
--

--- Patch [1]: [PATCH RFC v6 1/7] mfd: ls2kbmc: Make a copy when parsing mode string ---
commit 79e8b832a812c3929b8bad068187bcaad3dc3920
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. This commit makes a copy
of the original string and manipulates the copy instead.

> diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> --- 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)
>   */
>  static int ls2k_bmc_parse_mode(struct pci_dev *pdev, struct simplefb_platform_data *pd)
>  {
> -	char *mode;
> +	/* Assume 64 bytes is enough for the resolution string */
> +	char mode_buf[64], mode_buf_orig[64];
> +	char *mode = mode_buf;
> +	const void __iomem *mode_base;
>  	int depth, ret;
>  
>  	/* The last 16M of PCI BAR0 is used to store the resolution string. */
> -	mode = devm_ioremap(&pdev->dev, pci_resource_start(pdev, 0) + SZ_16M, SZ_16M);
> -	if (!mode)
> +	mode_base = ioremap(pci_resource_start(pdev, 0) + SZ_16M,
> +			    sizeof(mode_buf));

[Severity: High]
This is a pre-existing issue, but does this mapping require a validation check
for the PCI BAR0 length?

If a virtual or malformed device advertises a BAR0 smaller than 16MB, could
calling ioremap at this fixed SZ_16M offset lead to a master abort or
unauthorized reads of memory outside the allocated PCI resources?

> +	if (!mode_base)
>  		return -ENOMEM;
[ ... ]
>  	ret = kstrtoint(strsep(&mode, "@"), 10, &depth);
>  	if (ret)
> -		return ret;
> +		goto invalid_mode;
>  
>  	pd->stride = pd->width * depth / 8;

[Severity: High]
This is a pre-existing issue, but can this stride calculation overflow a
32-bit integer?

Since width and depth are parsed from device memory, could large values like
width = 0x20000001 cause the width * depth / 8 calculation to overflow into
a small stride, potentially causing out-of-bounds memory accesses when passed
to the framebuffer subsystem?

>  	pd->format = depth == 32 ? "a8r8g8b8" : "r5g6b5";
>  
>  	return 0;
> +
> +invalid_mode:
> +	dev_err(&pdev->dev, "Invalid resolution string: %s\n", mode_buf_orig);

[Severity: Medium]
Could this log the raw resolution string from the BMC without escaping it?

If a malformed resolution string contains non-printable characters or ANSI
escape codes in the BMC memory, would printing it directly via %s lead to
terminal escape sequence injection and log spoofing?

> +	return ret;
>  }

While reviewing this file, I also noticed a few other pre-existing issues
outside the patch diff that might need attention:

> static struct mfd_cell ls2k_bmc_cells[] = {

[Severity: High]
This is a pre-existing issue, but is it safe to omit the const modifier from
this global mfd_cell array?

In ls2k_bmc_probe(), this array is dynamically mutated:

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

Does assigning the address of a local stack variable to this global array
leave a dangling pointer after the probe function returns, potentially leading
to use-after-free or data corruption if another device is probed? Does this
also violate the MFD subsystem guidelines for static const cell arrays?


> static irqreturn_t ls2k_bmc_interrupt(int irq, void *arg)
> {
> 	...
> 	return IRQ_HANDLED;
> }

[Severity: High]
This is a pre-existing issue, but does this shared interrupt handler properly
check hardware status before returning IRQ_HANDLED?

If it unconditionally returns IRQ_HANDLED without verifying if its device
generated the interrupt, will it falsely claim interrupts meant for other
devices sharing the same IRQ line and cause them to malfunction?


> static void ls2k_bmc_events_fn(struct work_struct *work)
> {
> 	...
> 	stop_machine(ls2k_bmc_recover_pci_data, ddata, NULL);
> 	...
> }

[Severity: Critical]
This is a pre-existing issue, but does ls2k_bmc_recover_pci_data() execute a
long delay inside a stop_machine callback?

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

Since stop_machine() runs with local interrupts disabled on all CPUs, will a
10-second mdelay freeze the entire system and trigger the NMI watchdog hard
lockup detector?


> static void ls2k_bmc_save_pci_data(struct pci_dev *pdev, struct ls2k_bmc_ddata *ddata)
> {
> 	struct pci_dev *parent = pdev->bus->self;
> 	...
> 	pci_read_config_dword(parent, PCI_COMMAND, &ddata->bridge_pci_data.pci_command);

[Severity: High]
This is a pre-existing issue, but does this code unconditionally dereference
the parent pointer?

If the device is attached to the root bus, such as in a VM passthrough setup,
pdev->bus->self would be NULL. Would passing this NULL pointer directly to
pci_read_config_dword() and other PCI accessors cause a kernel panic?


> static int ls2k_bmc_init(struct ls2k_bmc_ddata *ddata)
> {
> 	...
> 	INIT_WORK(&ddata->bmc_reset_work, ls2k_bmc_events_fn);

[Severity: High]
This is a pre-existing issue, but is the bmc_reset_work properly canceled on
device removal?

If the workqueue is initialized but never properly canceled when the driver
is unbound and the ddata structure is freed by devm, could this lead to a
use-after-free if the work is scheduled or running?


> static void ls2k_bmc_save_pci_data(struct pci_dev *pdev, struct ls2k_bmc_ddata *ddata)
> {
> 	struct pci_dev *parent = pdev->bus->self;
> 	...
> 	pci_read_config_dword(parent, LS7A_GEN2_CTL, &ddata->bridge_pci_data.gen2_ctrl);

[Severity: High]
This is a pre-existing issue, but does the driver unconditionally assume its
parent bridge is a Loongson LS7A device?

If the device is attached behind a standard PCIe switch or QEMU root port,
will writing to vendor-specific registers like LS7A_GEN2_CTL corrupt standard
PCI bridge configuration space and potentially break system routing or crash?

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