Re: [PATCH v5 3/6] spmi: apple: check transaction status

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

Pre-existing issues:
- [High] Missing synchronization in SPMI read and write commands leads to data races and hardware state corruption.
- [High] Signed integer overflow undefined behavior (UB) during bitwise shifts in command packing and data transfer.
--

--- Patch [3]: [PATCH v5 3/6] spmi: apple: check transaction status ---
commit 138d557e0b3d3f2a65351b5debb754e7d0b8485e
Author: Alba Mendez <[email protected]>

spmi: apple: check transaction status

Check for parity errors and missing command ACKs.

> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> index 15721cb41d5c8..b1c127cf5f441 100644
> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c
[ ... ]
> @@ -66,7 +74,7 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  {
>  	struct apple_spmi *spmi = spmi_controller_get_drvdata(ctrl);
>  	u32 spmi_cmd = apple_spmi_pack_cmd(opc, sid, saddr, len);

[Severity: High]
This is a pre-existing issue, but this multi-step transaction begins here
without holding a lock to serialize with other SPMI commands.

If two kernel threads (e.g. a regulator driver and a thermal sensor driver)
initiate an SPMI transaction on the same controller simultaneously, can this
lead to interleaved register writes to the shared SPMI_CMD_REG and reads from
SPMI_RSP_REG?

This missing synchronization seems like it could corrupt SPMI transactions,
leading to incorrect PMIC accesses. This patch also exacerbates the issue by
adding unsynchronized non-atomic accesses to spmi->prev_fail.

[Severity: High]
This isn't a bug introduced by this patch, but does the bitwise shift inside
apple_spmi_pack_cmd() invoke undefined behavior due to signed integer overflow?

In apple_spmi_pack_cmd():
    return opc | sid << 8 | saddr << 16 | (len - 1) | (1 << 15);

Since saddr is implicitly promoted to a signed int, will shifting it by 16
when saddr >= 0x8000 overflow INT_MAX?

There appear to be similar signed integer overflows during data transfers.

In spmi_read_cmd():
    buf[len_read++] = ((0xff << (8 * i)) & rsp) >> (8 * i);

Does shifting the signed literal 0xff left by 24 (when i == 3) overflow
INT_MAX?

In spmi_write_cmd():
    spmi_cmd |= buf[i++] << (j++ * 8);

Since buf[i] is promoted to a signed int, will shifting left by 24 (when
j == 3 and the byte is >= 128) overflow INT_MAX?

Compilers can exploit this undefined behavior to aggressively optimize or drop
code, which may result in corrupted SPMI commands or missing bytes in
transactions.

> -	u32 rsp;
> +	u32 reply, rsp;
>  	size_t len_read = 0;
>  	u8 i;

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