Re: [PATCH v1] iio: health: max30102: fix NULL dereference in interrupt handler

On 7/31/26 1:41 PM, Marco Chen wrote:
> The interrupt is requested in max30102_probe() and stays enabled

I've always wondered why most IIO drivers seem to do this. It seems to
me that requesting the irq with IRQF_NO_AUTOEN and only enabling it
when everything is set up for reading data would avoid spurious interrupts
like this.

> for the lifetime of the device, but indio_dev->active_scan_mask is only
> valid while a buffer is enabled. When an interrupt arrives while no
> buffer is enabled, the handler dereferences the NULL active_scan_mask:
> 
>   Unable to handle kernel NULL pointer dereference at virtual address 
> 0000000000000000
>   pc : __bitmap_weight+0x64/0x98
>   lr : max30102_interrupt_handler+0x48/0x160 [max30102]
>   Call trace:
>    __bitmap_weight+0x64/0x98 (P)
>    max30102_interrupt_handler+0x48/0x160 [max30102]
>    irq_thread_fn+0x28/0xa8
>    irq_thread+0x184/0x30c
>    kthread+0x118/0x124
>    ret_from_fork+0x10/0x20
> 
> Return early when no buffer is enabled. The interrupt status register is
> read before returning, since reading it deasserts the chip's active-low
> interrupt pin. Otherwise the pin would stay asserted and no further
> edges would be delivered.
> 
> Fixes: 90579b69e94b ("iio: health: max30102: Add MAX30105 support")
> Signed-off-by: Marco Chen <[email protected]>
> ---
> I ran into this while interfacing with the MAX30102 over I2C on a
> Raspberry Pi 4 running the IIO subsystem tree testing branch and
> learning the IIO sysfs interface for the first time. When physically
> rearranging INT pin wiring with no buffer enabled, the kernel oopsed.

This is one of the reasons why you are supposed to turn off power before
moving wires. ;-)

> This happened because max30102_interrupt_handler() attempted to
> dereference active_scan_mask, which is NULL because no buffer is
> enabled. With this patch, the same situation no longer oopses
> and the buffered capture was tested to work normally afterward.
> 
> Some things I am unsure about though:
> - Is IRQ_HANDLED or IRQ_NONE preferred here? I chose IRQ_HANDLED 
>   because of the status register read to deassert the INT pin, but 
>   I am not 100% confident on this choice.

IRQ_NONE would cause the handler to just run again as soon as it
exits, so it is almost never the right value to return. IRQ_HANDLED
is correct.

> - Should the regmap_read() return value be checked? I did not add a 
>   check because I do not see a useful recovery path from this I2C failure, 
>   but I can add a check in a v2 if it is better.

There isn't anything we could do other than log the error.


> Thank you.
> 
> drivers/iio/health/max30102.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/health/max30102.c b/drivers/iio/health/max30102.c
> index c37316c86f14..c30b029ba8aa 100644
> --- a/drivers/iio/health/max30102.c
> +++ b/drivers/iio/health/max30102.c
> @@ -290,10 +290,23 @@ static irqreturn_t max30102_interrupt_handler(int irq, void *private)
>  {
>  	struct iio_dev *indio_dev = private;
>  	struct max30102_data *data = iio_priv(indio_dev);
> -	unsigned int measurements = bitmap_weight(indio_dev->active_scan_mask,
> -						  iio_get_masklength(indio_dev));
> +	unsigned int measurements, val;
>  	int ret, cnt = 0;
>  
> +	if (!indio_dev->active_scan_mask) {
> +		/*
> +		 * No buffer is enabled so there is nothing to read. Read the
> +		 * status register anyway to deassert the max30102's interrupt
> +		 * pin; otherwise it would stay asserted and further edges
> +		 * would not be delivered.
> +		 */
> +		regmap_read(data->regmap, MAX30102_REG_INT_STATUS, &val);
> +		return IRQ_HANDLED;
> +	}
> +
> +	measurements = bitmap_weight(indio_dev->active_scan_mask,
> +				     iio_get_masklength(indio_dev));
> +
>  	mutex_lock(&data->lock);
>  
>  	while (cnt || (cnt = max30102_fifo_count(data)) > 0) {