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

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

[Marco Chen]

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

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

[David Lechner]

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

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

[Jonathan Cameron]

On Sat, 1 Aug 2026 10:18:42 -0500
David Lechner <dlechner@baylibre.com> wrote:

> 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.

It teaches discipline.  More serious from what I recall the no autoen thing
used to be done via a racey bit of trying to turn it off as fast as you can.

Most of the time we should be able to write drivers that don't need to
use that flag and don't touch enable_irq() at all.

Obviously that's not always true.

> 
> > 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 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.

This is always a fun debate.  If you do return IRQ_NONE you will enter
the handler again, but not for long.  The spurious interrupt detection
code kicks in and disables the interrupt with a nice loud splat.
That may or may not be appropriate.

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

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

[Jonathan Cameron]

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

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

[David Lechner]

On 8/1/26 9:10 PM, Jonathan Cameron wrote:
> On Sat, 1 Aug 2026 10:18:42 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 
>> 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.
> 
> It teaches discipline.  More serious from what I recall the no autoen thing
> used to be done via a racey bit of trying to turn it off as fast as you can.
> 
> Most of the time we should be able to write drivers that don't need to
> use that flag and don't touch enable_irq() at all.
> 
> Obviously that's not always true.
> 

Hmm... so I should go back and change [1] since it isn't strictly
required there?

[1]: https://lore.kernel.org/linux-iio/20260731-iio-adc-ti-ads112c14-continuous-mode-v2-1-eb13da38e8fc@baylibre.com/

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

[Jonathan Cameron]

On Sun, 2 Aug 2026 10:21:41 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 8/1/26 9:10 PM, Jonathan Cameron wrote:
> > On Sat, 1 Aug 2026 10:18:42 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> >   
> >> 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.  
> > 
> > It teaches discipline.  More serious from what I recall the no autoen thing
> > used to be done via a racey bit of trying to turn it off as fast as you can.
> > 
> > Most of the time we should be able to write drivers that don't need to
> > use that flag and don't touch enable_irq() at all.
> > 
> > Obviously that's not always true.
> >   
> 
> Hmm... so I should go back and change [1] since it isn't strictly
> required there?
> 
> [1]: https://lore.kernel.org/linux-iio/20260731-iio-adc-ti-ads112c14-continuous-mode-v2-1-eb13da38e8fc@baylibre.com/
> 
> 

Ah. I was assuming that was spurious irq territory or a device
where we couldn't disable it device end.

I remember a discussion a long time back with the irq maintainers
(probably tglx) about the fact that they really advise drivers not
to turn interrupts on or off because it can be a very expensive
operation on some systems and such control belongs at the device
end when possible. What a host does with a disabled irq is just
too variable. The only guarantee is it won't immediately report it.

Maybe that has changed over time - I have no idea!

Anyhow, yes - I would drop that enable / disable dance if it
was about spurious prevention.  If we want to prevent spurious then
better thing to do is to check the software interrupt status register.
Looks like for that part there is a drdy status bit.   Bit annoying that you'd
have to drop to a thread to read it but we probably should do that
before the complete.  Maybe we could speculatively complete and
check it in the main thread.  

I note you do use that for polling.  I'd treat the irq as a way
to ensure you only need to read it once rather than not read it at all.

Jonathan

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

[Marco Chen]

On Sun, Aug 02, 2026 at 03:26:09AM +0100, Jonathan Cameron wrote:
> > +	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.

I understand what you mean. I will remove this active_scan_mask check.

> 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.

This makes a lot of sense. For the v2 patch, I will plan to read
INT_STATUS once at the top of the handler and pass that value into
max30102_fifo_count() so it is not read twice. Consequently I will
remove the INT_STATUS read from max30102_fifo_count(), too.

For checking INT_STATUS to see if it isn't our interrupt, instead of
checking if nothing is set, I was thinking to check the
MAX30102_REG_INT_STATUS_FIFO_RDY bit specifically because that is the 
only interrupt enabled in max30102_chip_init().

This will close the reproducer I was hitting earlier as you said. Since
we only ever reach bitmap_weight() when FIFO_RDY is set, the NULL dereference
is avoided. I think there is a theoretical window if the buffer is disabled
between the FIFO_RDY check and bitmap_weight() call, but as you said,
closing this would need the buffered mode claim. I plan to leave that
out of this fix.


On Sat, Aug 01, 2026 at 16:18:00 +0100, David Lechner wrote:

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

Fair point, I was pretty careless.

> 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.

Got it. I will keep IRQ_HANDLED for the v2.

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

Understood. Since the INT_STATUS read at the top of the interrupt handler now
determines if it is our interrupt, I will check the return value of
regmap_read() and log the error on failure.