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

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

Pre-existing issues:
- [High] The driver illegally modifies a shared static `mfd_cell` array (`l=
s2k_bmc_cells`) during probe, assigning a pointer to a stack-allocated vari=
able.
- [High] Massive `mdelay` (10+ seconds) inside a `stop_machine` callback wi=
ll freeze the system and trigger hard lockups.
- [High] Video mode dimensions parsed from untrusted BMC memory are not bou=
nds-checked, causing an out-of-bounds write in the DRM subsystem.
--

--- Patch [1]: [PATCH RFC v5 1/7] mfd: ls2kbmc: Make a copy when parsing mo=
de string ---
commit 04a85f9ac065d47d2e55cbd8e495ab417bf78739
Author: Miao Wang <[email protected]>
Subject: mfd: ls2kbmc: Make a copy when parsing mode string

This commit resolves an issue where the resolution string mapped from
the BMC was being manipulated in-place by strsep(). It creates a copy
of the string to parse instead.

> diff --git a/drivers/mfd/ls2k-bmc-core.c b/drivers/mfd/ls2k-bmc-core.c
> index 408056bfb2fe7..fc9695eedd366 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 *dda=
ta)
[ ... ]
> -	ret =3D kstrtoint(strsep(&mode, "x"), 10, &pd->width);
> +	ret =3D kstrtouint(strsep(&mode, "x"), 10, &pd->width);
>  	if (ret)
> -		return ret;
> +		goto invalid_mode;
> =20
> -	ret =3D kstrtoint(strsep(&mode, "-"), 10, &pd->height);
> +	if (mode =3D=3D NULL) {
> +		ret =3D -EINVAL;
> +		goto invalid_mode;
> +	}
> +	ret =3D kstrtouint(strsep(&mode, "-"), 10, &pd->height);
>  	if (ret)
> -		return ret;
> +		goto invalid_mode;
> =20
> +	if (mode =3D=3D NULL) {
> +		ret =3D -EINVAL;
> +		goto invalid_mode;
> +	}
>  	ret =3D kstrtoint(strsep(&mode, "@"), 10, &depth);
>  	if (ret)
> -		return ret;
> +		goto invalid_mode;
> =20
>  	pd->stride =3D pd->width * depth / 8;
>  	pd->format =3D depth =3D=3D 32 ? "a8r8g8b8" : "r5g6b5";
> =20
>  	return 0;

[Severity: High]
This isn't a bug introduced by this patch, but does this parsing logic need
bounds checking for the dimensions provided by the BMC?

The parsed width, height, and stride are used to configure the framebuffer.
However, the backing memory is capped at a fixed 4MB size:

static struct resource ls2k_display_resources[] =3D {
        DEFINE_RES_MEM_NAMED(LS2K_DISPLAY_RES_START, SZ_4M, "simpledrm-res"=
),
};

If a compromised or misconfigured BMC provides abnormally large dimensions,
could this lead to out-of-bounds writes past the 4MB limit when the
framebuffer is cleared or rendered?

> +
> +invalid_mode:
> +	dev_err(&pdev->dev, "Invalid resolution string: %s\n", mode_buf_orig);
> +	return ret;
>  }
> =20
>  static int ls2k_bmc_probe(struct pci_dev *dev, const struct pci_device_i=
d *id)

[Severity: High]
This is a pre-existing issue, but does ls2k_bmc_probe() leave a dangling
pointer by assigning a stack variable to a global array?

The driver modifies the global static array ls2k_bmc_cells during probe:

ls2k_bmc_probe() {
        ...
        ls2k_bmc_cells[LS2K_BMC_DISPLAY].platform_data =3D &pd;
        ls2k_bmc_cells[LS2K_BMC_DISPLAY].pdata_size =3D sizeof(pd);
        ...
}

Since pd is local to the stack during probe, the global array is left with a
dangling pointer once the function returns. Could this cause a stack
use-after-free if another device instance probes concurrently or accesses
the array later? Additionally, shouldn't MFD cell arrays be static const?

[Severity: High]
This is another pre-existing issue, but does ls2k_bmc_events_fn() trigger
hard lockups by sleeping inside stop_machine()?

When a BMC reset occurs, the driver executes ls2k_bmc_events_fn(), which
calls stop_machine() to freeze all CPUs with local interrupts disabled:

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

Inside ls2k_bmc_recover_pci_data(), the code waits for the BMC to become
ready using a 10-second busy loop:

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

Does holding all CPUs with interrupts disabled for 10 seconds
unconditionally trigger NMI watchdogs and RCU stall panics?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260804-ls2kbmc-mo=
[email protected]?part=3D1