Re: [PATCH 3/7] irqchip/irq-qcom-mpm: Prepare common access path for timer and pin regs

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The initialization loop in the probe function writes zeroes to registers based on the device tree property `qcom,mpm-pin-count` without verifying if the calculated `reg_stride` fits within the mapped I/O memory region.
- [High] Lack of bounds checking on `d->hwirq` combined with signed integer conversion allows arbitrary out-of-bounds memory accesses when reading/writing IRQ registers.
--

--- Patch [3]: [PATCH 3/7] irqchip/irq-qcom-mpm: Prepare common access path for timer and pin regs ---
commit 481c7b7ccc27a48778456542a65ce5c589dcaa00
Author: Sneh Mankad <sneh.mankad@oss.qualcomm.com>

irqchip/irq-qcom-mpm: Prepare common access path for timer and pin regs

This commit introduces a common addressing helper, qcom_mpm_offset(),
to calculate memory offsets for both timer and pin registers. It also 
replaces the macro definitions for vMPM registers with an enum and 
updates the read/write accessor functions to use it.

> diff --git a/drivers/irqchip/irq-qcom-mpm.c b/drivers/irqchip/irq-qcom-mpm.c
> index 01fd1843172aa..763eddee99dc4 100644
> --- a/drivers/irqchip/irq-qcom-mpm.c
> +++ b/drivers/irqchip/irq-qcom-mpm.c
[ ... ]
> @@ -92,18 +97,36 @@ struct qcom_mpm_priv {
>  	atomic_t cpus_in_pm;
>  };
>  
> -static u32 qcom_mpm_read(struct qcom_mpm_priv *priv, unsigned int reg,
> -			 unsigned int index)
> +static unsigned int qcom_mpm_offset(struct qcom_mpm_priv *priv, enum qcom_mpm_reg reg,
> +				    unsigned int index)
> +{
> +	unsigned int reg_offset;
> +
> +	/*
> +	 * Per the vMPM register map, TIMER[0..1] starts at register index 0 and all pin-specific
> +	 * registers start after the two TIMER regs. Pin-specific register IDs start at
> +	 * MPM_REG_ENABLE, so subtract it to convert to a zero-based pin-register group index.
> +	 */
> +	if (reg == MPM_REG_TIMER)
> +		reg_offset = index;
> +	else
> +		reg_offset = MPM_TIMER_REGS +
> +			 (reg - MPM_REG_ENABLE) * priv->reg_stride + index;
> +
> +	return reg_offset * sizeof(u32);
> +}

[Severity: High]
This isn't a bug introduced by this patch, but I'm wondering if
priv->reg_stride is validated against the size of the mapped I/O memory region?

Looking at qcom_mpm_probe():

	priv->reg_stride = DIV_ROUND_UP(pin_cnt, 32);

If a very large qcom,mpm-pin-count is provided in the device tree, could the
initialization loop later in qcom_mpm_probe():

	for (i = 0; i < priv->reg_stride; i++) {
		qcom_mpm_write(priv, MPM_REG_ENABLE, i, 0);
		...

write zeroes out of bounds into memory following priv->base?

[Severity: High]
This is a pre-existing issue, but does this function safely handle the index
parameter against malformed device tree inputs?

In qcom_mpm_alloc(), pin is assigned as d->hwirq from the device tree
interrupt specifier without bounds validation against domain->hwirq_max.

Later in qcom_mpm_enable_irq():

	int pin = d->hwirq;
	unsigned int index = pin / 32;

If a sufficiently large d->hwirq (e.g., 0x80000000) is provided, would the
signed integer conversion cause it to wrap into a negative number, resulting
in a massive unsigned index being passed into qcom_mpm_offset() and allowing
arbitrary out-of-bounds memory accesses?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-b4-shikra_lpm_addition-v1-0-3d858df2cbbf@oss.qualcomm.com?part=3