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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 31 Jul 2026 14:41:23 -0400
Marco Chen <marcochen.dev@gmail.com> wrote:

> The interrupt is requested in max30102_probe() and stays enabled
> 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 <marcochen.dev@gmail.com>
> ---
> 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 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.
> - 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.
> 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) {

That is racy as this could be going on in parallel with the buffer being disabled
so we check here but it's gone before it is accessed.

It does protect against the spurious interrupt when the device isn't really in
use.

Hmm. I'm not sure what the right fix for this is or how much effort we put
into protecting against this level of things going wrong.

We could do a buffer mode claim, but then we'd need to do that in every
driver that touches active_scan_mask.  Also for that matter that pushes to
buffers as they can change at an time when buffered mode isn't in use.

> +		/*
> +		 * 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);

That we should only do if we have any reason to believe it was set in the first
place. Did we check it?   Looks like we didn't.

Which leads me to suggest alternative fix - check INT_STATUS and if nothing
set it isn't our interrupt.  That doesn't get into the potential buffer
enabled / disabled races but should close your condition I think
- looks like it is read in the fifo part below. You may need to refactor a
little to not read it twice.

Jonathan

> +		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) {