Re: [PATCH] spi: meson-spifc: fix runtime PM leak on remove

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

Pre-existing issues:
- [High] Kernel stack memory is leaked to hardware registers when copying a partial 32-bit word.
- [High] During system resume, `meson_spifc_resume()` accesses SPI registers while the controller clock is disabled if the device was runtime suspended, triggering a system crash.
- [High] Direct 32-bit casts on potentially unaligned SPI buffers will cause unaligned memory access faults.
--

commit 65d4bcf61796efe6db17033eb44065b91fb07665
Author: Ruoyu Wang <[email protected]>

spi: meson-spifc: fix runtime PM leak on remove

This commit balances a pm_runtime_get_sync() call with pm_runtime_put_noidle()
in the remove function to fix a runtime PM usage counter leak.

> diff --git a/drivers/spi/spi-meson-spifc.c b/drivers/spi/spi-meson-spifc.c
> index d700fa3152231..e36aa94bbdae3 100644
> --- a/drivers/spi/spi-meson-spifc.c
> +++ b/drivers/spi/spi-meson-spifc.c
> @@ -345,6 +345,7 @@ static void meson_spifc_remove(struct platform_device *pdev)
>  {
>  	pm_runtime_get_sync(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_put_noidle(&pdev->dev);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but could this leak uninitialized
kernel stack memory to hardware registers?

In meson_spifc_fill_buffer(), when the remaining length is less than 4 bytes:

drivers/spi/spi-meson-spifc.c:meson_spifc_fill_buffer() {
	...
	while (i < len) {
		if (len - i >= 4)
			data = *(u32 *)buf;
		else
			memcpy(&data, buf, len - i);

		regmap_write(spifc->regmap, REG_C0 + i, data);
	...
}

If len - i < 4, memcpy() copies only 1 to 3 bytes into the local 32-bit
variable data. The remaining bytes in data are left uninitialized.

The entire 32-bit variable is then written to the REG_C0 register,
potentially leaking kernel stack memory.

[Severity: High]
This isn't a bug introduced by this patch, but can this cause a synchronous
external abort during system resume?

In meson_spifc_resume(), the clock is not enabled if the device was runtime
suspended:

drivers/spi/spi-meson-spifc.c:meson_spifc_resume() {
	...
	if (!pm_runtime_suspended(dev)) {
		ret = clk_prepare_enable(spifc->clk);
		if (ret)
			return ret;
	}

	meson_spifc_hw_init(spifc);
	...
}

meson_spifc_hw_init() unconditionally writes to hardware registers via
regmap_update_bits().

If the device was runtime suspended, the clock remains disabled, and
accessing unclocked peripheral registers can cause a system crash.

[Severity: High]
This isn't a bug introduced by this patch, but will this cause unaligned
memory access faults?

Both meson_spifc_drain_buffer() and meson_spifc_fill_buffer() cast the
generic u8 *buf pointer directly to u32 * to perform 4-byte operations:

drivers/spi/spi-meson-spifc.c:meson_spifc_drain_buffer() {
	...
		if (len - i >= 4) {
			*((u32 *)buf) = data;
			buf += 4;
		} else {
	...
}

And similarly in meson_spifc_fill_buffer():

drivers/spi/spi-meson-spifc.c:meson_spifc_fill_buffer() {
	...
	while (i < len) {
		if (len - i >= 4)
			data = *(u32 *)buf;
		else
	...
}

The SPI subsystem does not guarantee that tx_buf or rx_buf are 4-byte
aligned. Casting and dereferencing them as 32-bit pointers constitutes
undefined behavior in C and can lead to alignment faults depending on
the architecture.

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

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic