Re: [PATCH v3 2/6] spmi: apple: Validate FIFO state

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

New issues:
- [High] Unbounded hardware polling loop in flush_extra_data() can cause a kernel soft lockup.
- [Medium] spmi_read_cmd() immediately aborts multi-word reads if the FIFO temporarily drains mid-transaction.

Pre-existing issues:
- [High] Missing validation on the `len` parameter causes an integer underflow when packing commands.
- [High] Lack of synchronization in spmi_read_cmd() and spmi_write_cmd() causes data corruption on concurrent access.
- [High] The driver violates I/O accessor guidelines by using register accessors for FIFOs.
--

--- Patch [2]: [PATCH v3 2/6] spmi: apple: Validate FIFO state ---
commit 93b3f99f86c4f9fba2be0ca9a0aaffd4f71aca98
Author: Alba Mendez <[email protected]>

spmi: apple: Validate FIFO state

Check for data before reading the body of a reply, and check for
end of data afterwards.

> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> index 376cf682c43e0..e037ef914b6c3 100644
> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/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)) {

[Severity: High]
Could this loop lead to a kernel soft lockup?

If the hardware is powered down or in an unexpected state, the empty bit will
never assert. Since there is no iteration limit or timeout, the kernel would
spin indefinitely.

> +		if (!warned) {
> +			dev_warn(&ctrl->dev, "FIFO has extra data\n");
> +			warned = true;
> +		}
> +		readl(spmi->regs + SPMI_RSP_REG);
> +	}
> +}
> +
>  static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  			 u16 saddr, u8 *buf, size_t len)
>  {
[ ... ]
>  	/* Read SPMI data reply */
>  	while (len_read < len) {

[Severity: High]
This is a pre-existing issue, but does this transaction sequence lack
mutual exclusion?

The SPMI subsystem core delegates locking to the driver. Without a lock,
concurrent threads could interleave their writes to SPMI_CMD_REG and read
each other's responses from SPMI_RSP_REG.

The newly added flush_extra_data() might also inadvertently discard
responses intended for concurrently executing threads.

> +		if (readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY) {
> +			dev_err_ratelimited(&ctrl->dev,
> +					    "FIFO lacks reply data, controller stuck?\n");
> +			return -EIO;
> +		}

[Severity: Medium]
Does this immediate error check risk aborting multi-word reads if the FIFO
temporarily drains mid-transaction?

Because the CPU MMIO reads execute much faster than the serial SPMI bus, the
FIFO might briefly empty before the hardware finishes buffering the entire
response.

Aborting immediately would bypass flush_extra_data(), leaving delayed
data stuck in the FIFO and potentially corrupting subsequent transactions.

>  		rsp = readl(spmi->regs + SPMI_RSP_REG);

[Severity: High]
This is a pre-existing issue, but does the use of readl() for a FIFO
violate subsystem endianness guidelines?

According to the I/O accessors guidelines, FIFOs should exclusively use
stream accessors (like readsl()) to prevent implicit CPU-to-device
endianness byteswapping.

On big-endian systems, readl() performs a byteswap, which could result
in a reversed byte stream.

>  		i = 0;
>  		while ((len_read < len) && (i < 4)) {
[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does apple_spmi_pack_cmd()
lack validation on the len parameter?

If a client driver requests a 0-length SPMI transaction, subtracting 1 from
an unsigned size_t len could underflow to SIZE_MAX.

The bitwise OR would then corrupt the opc, sid, and saddr fields, completely
malforming the hardware command:

drivers/spmi/spmi-apple-controller.c:apple_spmi_pack_cmd()
    return opc | sid << 8 | saddr << 16 | (len - 1) | (1 << 15);

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