Re: [PATCH v3 6/6] spmi: apple: Add interrupt functionality

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

New issues:
- [Critical] 64-bit MMIO access (`readq`) on 32-bit registers causes SError on Apple Silicon.
- [High] Use-After-Free on probe error path due to un-cleaned chained IRQ handler.
- [High] Level-triggered interrupt not masked in handler causes interrupt storm.
- [High] Command timeouts race with `reinit_completion`, causing state corruption.
- [Medium] Unconditional deregistration of potentially negative IRQ in `apple_spmi_remove()`.

Pre-existing issues:
- [Critical] Buffer overflow in `spmi_raw_cmd` due to integer underflow.
--

--- Patch [6]: [PATCH v3 6/6] spmi: apple: Add interrupt functionality ---
commit 36738a2961c152dbbdb4e4ffe86859f785d88e60
Author: Alba Mendez <[email protected]>

spmi: apple: Add interrupt functionality
    
Add support for interrupts sent by slave devices and use IRQ for
RX FIFO if possible, as that IRQ fires as soon as the reply is
available, which is usually takes a few us instead of the 10ms sleep
interval for polling

> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> index 74d4b5d98b113..340d28126b3d4 100644
> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c

[ ... ]

> @@ -91,6 +122,8 @@ static int spmi_raw_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  
>  	guard(mutex)(&spmi->fifo_lock);
>  
> +	reinit_completion(&spmi->fifo_rx);

[Severity: High]
Can this reinit_completion() race with a late hardware interrupt?

If a previous command timed out and left the RX FIFO un-flushed, could a late
reply trigger complete() concurrently with this reinit_completion(), allowing
the new command to bypass the wait and read stale data?

[Severity: Critical]
This isn't a bug introduced by this patch, but is there a potential buffer
overflow in spmi_raw_cmd() when calculating the reply length?

While reading the data reply:

	while (len_read < ilen) {
		...
		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;
	}

If a caller requests a small read where ilen is less than 4, the condition
len_read < ilen ensures len_read - ilen is negative. Will this underflow to a
massive positive value when cast to size_t by min_t(), causing it to select
sizeof(spmi_cmd) and overwrite the buffer bounds?

> +
>  	writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);

[ ... ]

> +static void apple_spmi_irq_handler(struct irq_desc *desc)
> +{
> +	struct apple_spmi *spmi = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	bool handled = false;
> +	unsigned long val, offset, bit;
> +
> +	chained_irq_enter(chip, desc);
> +	val = readl(spmi->regs + SPMI_IRQ_ACK_BASE + (SPMI_IRQ_FIFO_RX / 32) * 4);
> +	if (val & BIT(SPMI_IRQ_FIFO_RX % 32)) {
> +		apple_spmi_irq_ack_raw(spmi, SPMI_IRQ_FIFO_RX);
> +		complete(&spmi->fifo_rx);
> +		handled = true;
> +	}

[Severity: High]
Does this leave the level-triggered interrupt unmasked? 

If the RX FIFO isn't drained immediately before returning, could the persistent
hardware level condition re-assert the interrupt right after exiting the 
handler, creating an interrupt storm that starves the thread trying to drain
the FIFO?

> +
> +	for (offset = 0; offset < SPMI_NUM_PERIPHERAL_IRQS / 8; offset += sizeof(val)) {
> +		val = readq(spmi->regs + SPMI_IRQ_ACK_BASE + offset);

[Severity: Critical]
Will using readq() on a 32-bit initialized MMIO region cause a synchronous
external abort (SError) on Apple Silicon? 

Since these registers are expected to be 32-bit regions, does a 64-bit access
trigger a hardware trap?

[ ... ]

> @@ -209,6 +441,15 @@ static int apple_spmi_probe(struct platform_device *pdev)
>  	ctrl->write_cmd = spmi_write_cmd;
>  	ctrl->cmd = spmi_cmd;
>  
> +	spmi->irq = platform_get_irq_optional(pdev, 0);
> +	if (spmi->irq < 0 && spmi->irq != -ENXIO)
> +		return spmi->irq;
> +	if (spmi->irq >= 0) {
> +		ret = apple_spmi_init_irq(pdev, spmi, spmi->irq);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ret = devm_spmi_controller_add(&pdev->dev, ctrl);
>  	if (ret)
>  		return dev_err_probe(&pdev->dev, ret,

[Severity: High]
If devm_spmi_controller_add() fails, will this leak the chained handler
registered inside apple_spmi_init_irq()? 

Since irq_set_chained_handler_and_data() is used without a devres cleanup 
action, could a subsequent interrupt dereference the freed spmi structure 
on this error path?

[ ... ]

> +static void apple_spmi_remove(struct platform_device *pdev)
> +{
> +	struct apple_spmi *spmi = platform_get_drvdata(pdev);
> +
> +	irq_set_chained_handler_and_data(spmi->irq, NULL, NULL);
> +}

[Severity: Medium]
Can spmi->irq be negative here? 

If platform_get_irq_optional() returned -ENXIO during probe, wouldn't we be
passing a negative value (which becomes a very large unsigned integer) to
irq_set_chained_handler_and_data() on removal?

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