Re: [PATCH v3 4/6] spmi: apple: Implement remaining commands

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Integer underflow in SPMI read loop size calculation causes an out-of-bounds write (buffer overflow).
- [Medium] Using `memcpy()` combined with `writel()`/`readl()` to pack/unpack stream data breaks big-endian compatibility and causes data corruption.
--

--- Patch [4]: [PATCH v3 4/6] spmi: apple: Implement remaining commands ---
commit 92726351469b11b82f4d483115faf989e21b09b0
Author: Alba Mendez <[email protected]>

spmi: apple: Implement remaining commands

This commit adds support for zero write and power management commands to the
Apple SPMI controller. It also refactors the command transmission logic into
shared raw command helper functions for both read and write paths.

> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c

[ ... ]

> @@ -77,18 +77,26 @@ static void flush_extra_data(struct spmi_controller *ctrl)
> -static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
> -			 u16 saddr, u8 *buf, size_t len)
> +static int spmi_raw_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
> +			 u16 param, const u8 *buf, size_t len, u8 *ibuf, size_t ilen)
>  {
>  	struct apple_spmi *spmi = spmi_controller_get_drvdata(ctrl);
> -	u32 spmi_cmd = apple_spmi_pack_cmd(opc, sid, saddr, len);
> +	u32 spmi_cmd = apple_spmi_pack_cmd(opc, sid, param);
>  	u32 reply, rsp;
>  	size_t len_read = 0;
> -	u8 i;
> +	size_t i = 0, j;
>  	int ret;
>  
>  	writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
>  
> +	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);

[Severity: Medium]
Will this sequence cause data corruption on big-endian architectures?

By using memcpy() to copy the byte buffer directly into a u32 variable, and
then passing that variable to writel(), the implicit CPU-to-little-endian
byteswap in writel() will reverse the byte order on the bus.

The previous code avoided this by packing the integer using manual bit shifts,
which preserves the byte layout regardless of CPU endianness.

> +		i += j;
> +	}
> +
>  	ret = apple_spmi_wait_rx_not_empty(ctrl);

[ ... ]

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

[Severity: Critical]
Does this length calculation underflow and cause an out-of-bounds write?

Since len_read starts at 0 and the loop condition ensures it is strictly less
than ilen, the subtraction of len_read - ilen will mathematically yield a
negative number. When evaluated as an unsigned size_t, this underflows to a
massive positive number.

Because of this underflow, min_t() will always select sizeof(spmi_cmd), which
is 4. If a caller requests a read smaller than 4 bytes, memcpy() will
unconditionally write 4 bytes and overflow the destination buffer. Should this
calculation be ilen - len_read instead?

[Severity: Medium]
Similar to the write path above, does unpacking the result of readl() using
memcpy() introduce an endianness issue?

readl() performs an implicit little-endian-to-CPU byteswap, which is then
copied sequentially into the byte buffer, potentially scrambling the payload
on big-endian systems.

> +		len_read += i;
>  	}
>  
>  	flush_extra_data(ctrl);
>  
> -	if (~FIELD_GET(SPMI_REPLY_FRAME_PARITY_STATUS, reply) & ((1 << len) - 1)) {
> +	if (!ilen && !FIELD_GET(SPMI_REPLY_ACK, reply)) {
> +		dev_err(&ctrl->dev, "command not acknowledged\n");
> +		return -EIO;
> +	}

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