Re: [PATCH 2/2] irqchip: add MSM8x60 MPM wakeup interrupt controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] The driver requests an IPC mailbox channel to notify the RPM of configuration changes but never sends any messages, rendering wake configurations ineffective.
- [High] Devres teardown sequence mismatch leaves the parent interrupt active after the IRQ domain is freed, leading to a Use-After-Free.
- [High] Calling `regmap_read()` and `regmap_write()` on a syscon regmap from within an irqchip's raw spinlock context causes a sleeping-while-atomic bug on PREEMPT_RT.
- [High] The irqchip fails to implement `.irq_set_wake`, breaking wakeup capabilities for the MPM and its parent GIC.
- [High] Clearing edge-triggered pending interrupts after executing the interrupt handler will drop subsequent edges.
- [Medium] Missing bounds validation on MPM pin indices from Device Tree allows arbitrary out-of-bounds register accesses.
- [Medium] Unsafe consumer API lacks reference counting or device links, leading to UAF if the MPM driver is unbound.
- [Medium] Failure to check the return value of `regmap_read()` results in uninitialized stack memory being used as register state.
--

commit 73a7572c3031147f818620520a565acba0e95ad1
Author: Herman van Hazendonk <github.com@herrie.org>

irqchip: add MSM8x60 MPM wakeup interrupt controller driver

This commit adds a driver for the MSM Power Manager (MPM) on the MSM8x60
family. It implements an irqchip wrapper to configure wake pins and exposes
an API for consumer drivers to manage raw wake signals. The driver interacts
with the RPM via a syscon regmap and IPC mailbox.

> diff --git a/drivers/irqchip/irq-msm8660-mpm.c b/drivers/irqchip/irq-msm8660-mpm.c
> new file mode 100644
> index 0000000000000..c579d1476e01b
> --- /dev/null
> +++ b/drivers/irqchip/irq-msm8660-mpm.c
> @@ -0,0 +1,627 @@

[ ... ]

> +static u32 msm8660_mpm_read(struct msm8660_mpm *mpm, unsigned int reg)
> +{
> +	u32 val;
> +	regmap_read(mpm->regmap, mpm->regmap_offset + reg, &val);
> +	return val;
> +}

[Severity: Medium]
Will this return uninitialized stack memory if the register read fails?

The variable val is uninitialized. If the regmap_read() function fails, it
will not update val, and this function will return stack garbage.

This could result in corrupted state when the read value is treated as
hardware pending status or used in read-modify-write operations.

[ ... ]

> +static irqreturn_t msm8660_mpm_irq(int irq, void *data)
> +{
> +	struct msm8660_mpm *mpm = data;
> +	unsigned long pending[MSM8660_MPM_REG_WIDTH];
> +	unsigned long enable[MSM8660_MPM_REG_WIDTH];
> +	int i, j;
> +
> +	for (i = 0; i < MSM8660_MPM_REG_WIDTH; i++) {
> +		pending[i] = msm8660_mpm_read(mpm,
> +			MSM8660_MPM_STATUS_OFFSET + i * 4);
> +		enable[i] = msm8660_mpm_read(mpm,
> +			MSM8660_MPM_REG_ENABLE + i * 4);
> +	}
> +
> +	for (i = 0; i < MSM8660_MPM_REG_WIDTH; i++) {
> +		unsigned long bits = pending[i] & enable[i];
> +
> +		for_each_set_bit(j, &bits, 32) {
> +			int pin = i * 32 + j;
> +			int hwirq = msm8660_mpm_pin_to_hwirq(mpm, pin);
> +
> +			if (hwirq >= 0) {
> +				dev_dbg(mpm->dev, "wake pin %d -> hwirq %d\n",
> +					pin, hwirq);
> +				generic_handle_domain_irq(mpm->domain, hwirq);
> +			}
> +		}
> +
> +		if (bits)
> +			msm8660_mpm_write(mpm,
> +				MSM8660_MPM_REG_CLEAR + i * 4, bits);

[Severity: High]
Does clearing the pending bits after executing the handler drop subsequent
edges?

If a new edge interrupt arrives on any of these pins while the handler is
executing in generic_handle_domain_irq(), the hardware will latch it again.

Because the write to the CLEAR register is delayed until after the handler
finishes, it will wipe out the newly latched interrupt, permanently losing
the new edge so it is not serviced.

> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void msm8660_mpm_enable_hwirq(struct irq_data *d, bool enable)
> +{
> +	struct msm8660_mpm *mpm = irq_data_get_irq_chip_data(d);
> +	unsigned long flags;
> +	int i, pin = -1;
> +	u32 val;
> +
> +	for (i = 0; i < mpm->pin_map_count; i++) {
> +		if (mpm->pin_map[i].hwirq == d->hwirq) {
> +			pin = mpm->pin_map[i].pin;
> +			break;
> +		}
> +	}
> +
> +	if (pin < 0)
> +		return;
> +
> +	raw_spin_lock_irqsave(&mpm->lock, flags);
> +
> +	val = msm8660_mpm_read(mpm,
> +		MSM8660_MPM_REG_ENABLE + (pin / 32) * 4);

[Severity: High]
Can this sleepable syscon regmap access while holding a raw spinlock cause a
locking violation?

The core IRQ subsystem invokes irqchip callbacks like irq_mask() and
irq_unmask() with the descriptor's raw_spinlock_t held and preemption
disabled.

Inside these functions, the driver explicitly acquires another raw_spinlock_t
and calls msm8660_mpm_read(), which invokes regmap_read() on the syscon
regmap.

Syscon regmaps use a standard spinlock_t for synchronization, which on
PREEMPT_RT is a sleepable rt_mutex. Attempting to acquire a sleepable lock
while holding a raw_spinlock_t is a locking violation.

> +	if (enable)
> +		val |= BIT(pin % 32);
> +	else
> +		val &= ~BIT(pin % 32);
> +	msm8660_mpm_write(mpm,
> +		MSM8660_MPM_REG_ENABLE + (pin / 32) * 4, val);

[Severity: High]
Should this register update be followed by a mailbox notification to the RPM?

The driver requests an IPC mailbox channel in msm8660_mpm_probe() to signal
configuration changes to the MPM. However, the driver never calls
mbox_send_message() after updating the hardware state here.

Without the doorbell ping, the RPM hardware will not re-evaluate the wake
configuration, which prevents the device from successfully waking the system
from suspend.

> +
> +	raw_spin_unlock_irqrestore(&mpm->lock, flags);
> +}

[ ... ]

> +static int msm8660_mpm_set_type(struct irq_data *d, unsigned int type)
> +{
> +	struct msm8660_mpm *mpm = irq_data_get_irq_chip_data(d);
> +	unsigned long flags;
> +	int i, pin = -1;
> +	u32 detect, polarity;
> +
> +	for (i = 0; i < mpm->pin_map_count; i++) {
> +		if (mpm->pin_map[i].hwirq == d->hwirq) {
> +			pin = mpm->pin_map[i].pin;
> +			break;
> +		}
> +	}
> +
> +	if (pin < 0)
> +		return -ENOENT;
> +
> +	raw_spin_lock_irqsave(&mpm->lock, flags);
> +
> +	detect = msm8660_mpm_read(mpm,
> +		MSM8660_MPM_REG_DETECT_CTL + (pin / 32) * 4);

[Severity: Medium]
Could this result in an out-of-bounds register access if the pin index is
invalid?

The pin value is parsed from the Device Tree in msm8660_mpm_probe() without
any bounds checking. If a malformed or malicious DT specifies a large pin
index, calculating the register offset using (pin / 32) * 4 will result in an
out-of-bounds read and write inside the RPM syscon block.

[ ... ]

> +static struct irq_chip msm8660_mpm_chip = {
> +	.name			= "msm8660-mpm",
> +	.irq_mask		= msm8660_mpm_mask_irq,
> +	.irq_unmask		= msm8660_mpm_unmask_irq,
> +	.irq_set_type		= msm8660_mpm_set_type,
> +	.irq_eoi		= irq_chip_eoi_parent,
> +	.irq_set_affinity	= irq_chip_set_affinity_parent,
> +	.flags			= IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE,

[Severity: High]
Does this missing irq_set_wake callback break system wakeup functionality?

The commit message states the driver "overrides ->irq_set_wake() to enable
the MPM mirror of the same interrupt".

However, the .irq_set_wake callback is omitted here and the flag
IRQCHIP_SKIP_SET_WAKE is applied instead.

When a consumer driver calls enable_irq_wake(), the generic IRQ core will
return 0 and stop propagating the wake request. Neither the MPM hardware nor
the parent GIC will receive the wake configuration.

> +};

[ ... ]

> +struct msm8660_mpm *msm8660_mpm_get(struct device_node *np,
> +				    const char *propname)
> +{
> +	struct device_node *mpm_np;
> +
> +	if (!msm8660_mpm_global)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	if (np && propname) {
> +		mpm_np = of_parse_phandle(np, propname, 0);
> +		if (!mpm_np)
> +			return ERR_PTR(-ENOENT);
> +		of_node_put(mpm_np);
> +	}
> +
> +	return msm8660_mpm_global;

[Severity: Medium]
Does returning the devm-managed pointer here create a use-after-free risk?

This consumer API returns a direct pointer to the devm_kzalloc allocated
structure without establishing a device_link, incrementing module refcounts,
or suppressing bind attributes.

If the MPM driver is unbound, devres will immediately free the structure.
Consumer drivers like mmci will retain a dangling pointer, leading to a
use-after-free when they later call msm8660_mpm_enable_pin().

> +}
> +EXPORT_SYMBOL_GPL(msm8660_mpm_get);

[ ... ]

> +static int msm8660_mpm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct msm8660_mpm *mpm;
> +	struct irq_domain *parent_domain;
> +	struct device_node *parent_np;
> +	int ret, i;
> +	u32 offset;

[ ... ]

> +	for (i = 0; i < mpm->pin_map_count; i++) {
> +		u32 pin, hwirq;
> +
> +		of_property_read_u32_index(np, "qcom,mpm-pin-map",
> +					   i * 2, &pin);
> +		of_property_read_u32_index(np, "qcom,mpm-pin-map",
> +					   i * 2 + 1, &hwirq);
> +
> +		mpm->pin_map[i].pin = pin;

[Severity: Medium]
Should the pin value be validated against MSM8660_MPM_PIN_COUNT before use?

The pin value is extracted from the Device Tree and stored directly. Missing
bounds validation here allows arbitrary out-of-bounds register accesses later
in functions like msm8660_mpm_set_type() and msm8660_mpm_enable_hwirq().

[ ... ]

> +	ret = devm_request_irq(dev, mpm->parent_irq, msm8660_mpm_irq,
> +			       IRQF_TRIGGER_HIGH | IRQF_NO_SUSPEND,
> +			       "msm8660-mpm", mpm);

[Severity: High]
Does registering the IRQ handler with devm create a teardown mismatch?

This registers the parent interrupt handler with devm_request_irq(), meaning
it remains active until the devres cleanup phase executes. The devres cleanup
happens after the remove() callback returns.

> +	if (ret) {
> +		dev_err(dev, "failed to request IRQ %d: %d\n",
> +			mpm->parent_irq, ret);
> +		goto err_remove_domain;
> +	}
> +
> +	/* Mailbox channel for poking MPM to re-read its config (writes to
> +	 * GCC + 0x008 bit 1 via the qcom-apcs-ipc mailbox).
> +	 */
> +	mpm->mbox_client.dev = dev;
> +	mpm->mbox_client.knows_txdone = true;
> +	mpm->mbox_chan = mbox_request_channel(&mpm->mbox_client, 0);

[Severity: High]
Is the mailbox channel requested here actually used to send messages?

The channel is requested for the purpose of signaling the MPM, but the driver
never calls mbox_send_message() after updating the vMPM registers to trigger
the IPC ping.

[ ... ]

> +static void msm8660_mpm_remove(struct platform_device *pdev)
> +{
> +	struct msm8660_mpm *mpm = platform_get_drvdata(pdev);
> +
> +	msm8660_mpm_global = NULL;
> +	if (mpm->mbox_chan)
> +		mbox_free_channel(mpm->mbox_chan);
> +	irq_domain_remove(mpm->domain);

[Severity: High]
Can freeing the irq domain manually here cause a use-after-free crash?

The irq_domain_remove() function immediately frees the domain memory, while
the devm-managed msm8660_mpm_irq() handler is still active.

If a hardware interrupt fires between the return of msm8660_mpm_remove() and
devres teardown, the active handler will execute and access the freed domain
pointer in generic_handle_domain_irq().

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1780148149.git.github.com@herrie.org?part=2