Re: [PATCH v3 5/6] spmi: apple: lock around FIFOs

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

Pre-existing issues:
- [Critical] Out-of-bounds memory write in `spmi_raw_cmd` due to an unsigned integer underflow when calculating remaining bytes for SPMI reply payload copy.
- [High] Improper use of `memcpy` with `writel`/`readl` for stream I/O causes byte reordering and data corruption on Big-Endian architectures.
- [High] The `flush_extra_data` function uses an unbounded `while` loop that can permanently deadlock the SPMI controller and cause a soft lockup.
--

--- Patch [5]: [PATCH v3 5/6] spmi: apple: lock around FIFOs ---
commit a147e75e08f9462d0254d993c5221a8c13ead6d5
Author: Alba Mendez <[email protected]>

spmi: apple: lock around FIFOs

The driver was missing locking around register interactions, which this
commit addresses by adding a mutex around the FIFO operations.

> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> index 8c9a70ba04221..74d4b5d98b113 100644
> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c
[ ... ]
> @@ -87,6 +89,8 @@ static int spmi_raw_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  	size_t i = 0, j;
>  	int ret;
>  
> +	guard(mutex)(&spmi->fifo_lock);
> +
>  	writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
>  
>  	while (i < len) {

[Severity: Critical]
This is a pre-existing issue, but does this code have an integer underflow
when calculating the remaining bytes to copy?

Looking at spmi_raw_cmd() in drivers/spmi/spmi-apple-controller.c:

	while (len_read < ilen) {
		if (readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY) {
			dev_err(&ctrl->dev, "FIFO lacks reply data, controller stuck?\n");
			return -EIO;
		}
		rsp = readl(spmi->regs + SPMI_RSP_REG);
		i = min_t(size_t, sizeof(spmi_cmd), len_read - ilen);
		memcpy(ibuf + len_read, &rsp, i);
		len_read += i;
	}

Since len_read is less than ilen in the loop condition, len_read - ilen
evaluates to a large positive integer. As a result, the min_t() call will
always return sizeof(spmi_cmd), which is 4.

If this performs a 1-byte read, will it blindly copy 4 bytes into a 1-byte
buffer on every iteration, leading to an out-of-bounds memory write?

[Severity: High]
This is a pre-existing issue, but could this implementation violate the I/O
accessor guidelines by mixing memcpy() with writel() and readl() for
stream data?

When writing to the TX FIFO in spmi_raw_cmd():

	while (i < len) {
		j = min_t(size_t, sizeof(spmi_cmd), len - i);
		spmi_cmd = 0;
		memcpy(&spmi_cmd, buf + i, j);
		writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
		i += j;
	}

And when reading from the RX FIFO:

		rsp = readl(spmi->regs + SPMI_RSP_REG);
		i = min_t(size_t, sizeof(spmi_cmd), len_read - ilen);
		memcpy(ibuf + len_read, &rsp, i);

Because writel() and readl() perform implicit byte-swapping on big-endian
architectures, placing bytes into a host-endian u32 with memcpy() before
using these accessors reverses the byte order on the bus.

Should this code use the stream I/O accessors like writesl() and readsl()
instead to preserve byte order consistently across architectures?

[Severity: High]
This isn't a bug introduced by this patch, but can the loop in
flush_extra_data() cause a soft lockup?

In drivers/spmi/spmi-apple-controller.c:

static void flush_extra_data(struct spmi_controller *ctrl)
{
	struct apple_spmi *spmi = spmi_controller_get_drvdata(ctrl);
	bool warned = false;

	while (!(readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY)) {
		if (!warned) {
			dev_warn(&ctrl->dev, "FIFO has extra data\n");
			warned = true;
		}
		readl(spmi->regs + SPMI_RSP_REG);
	}
}

If the hardware misbehaves or enters a faulty state where the
SPMI_RX_FIFO_EMPTY flag is never set, will this unbounded while loop run
indefinitely?

Since it executes with the new fifo_lock held, could this permanently
deadlock any subsequent SPMI transactions and block the CPU core?

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