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

[PATCH v2] 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

Read the interrupt status register at the top of the handler. If
FIFO_RDY is not set, return early because it isn't our interrupt.
Because FIFO_RDY is the only interrupt source enabled by this driver in
max30102_chip_init(), an invocation of max30102_interrupt_handler()
without FIFO_RDY set carries no data to read and can return early before
ever accessing active_scan_mask. This status read also deasserts the
MAX30102's active-low interrupt pin.

Since the interrupt status register is now read at the top of the
handler, pass the status value into max30102_fifo_count() instead of
having it read the register a second time.

Fixes: 90579b69e94b ("iio: health: max30102: Add MAX30105 support")
Signed-off-by: Marco Chen <marcochen.dev@gmail.com>
---
Changes in v2:
- Check FIFO_RDY bit in the interrupt status register instead of 
  active_scan_mask, because that was racy in v1, as pointed out by 
  Jonathan and David.
- Read interrupt status once and pass it into max30102_fifo_count().
- Check the regmap_read() return value and log on failure.

Tested with a MAX30102 on a Raspberry Pi 4 over I2C.
v1: https://lore.kernel.org/linux-iio/20260731184124.112124-1-marcochen.dev@gmail.com/

 drivers/iio/health/max30102.c | 37 +++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/health/max30102.c b/drivers/iio/health/max30102.c
index c37316c86f14..b949ce36c248 100644
--- a/drivers/iio/health/max30102.c
+++ b/drivers/iio/health/max30102.c
@@ -235,17 +235,10 @@ static const struct iio_buffer_setup_ops max30102_buffer_setup_ops = {
 	.predisable = max30102_buffer_predisable,
 };
 
-static inline int max30102_fifo_count(struct max30102_data *data)
+static inline int max30102_fifo_count(unsigned int status)
 {
-	unsigned int val;
-	int ret;
-
-	ret = regmap_read(data->regmap, MAX30102_REG_INT_STATUS, &val);
-	if (ret)
-		return ret;
-
 	/* FIFO has one sample slot left */
-	if (val & MAX30102_REG_INT_STATUS_FIFO_RDY)
+	if (status & MAX30102_REG_INT_STATUS_FIFO_RDY)
 		return 1;
 
 	return 0;
@@ -290,19 +283,39 @@ 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, status;
 	int ret, cnt = 0;
 
+	ret = regmap_read(data->regmap, MAX30102_REG_INT_STATUS, &status);
+	if (ret) {
+		dev_err_ratelimited(&data->client->dev,
+				    "Failed to read IRQ status: %d\n", ret);
+		return IRQ_HANDLED;
+	}
+	if (!(status & MAX30102_REG_INT_STATUS_FIFO_RDY))
+		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) {
+	while (cnt || (cnt = max30102_fifo_count(status)) > 0) {
 		ret = max30102_read_measurement(data, measurements);
 		if (ret)
 			break;
 
 		iio_push_to_buffers(data->indio_dev, data->processed_buffer);
 		cnt--;
+
+		ret = regmap_read(data->regmap, MAX30102_REG_INT_STATUS,
+				  &status);
+		if (ret) {
+			dev_err_ratelimited(&data->client->dev,
+					    "Failed to read IRQ status: %d\n",
+					    ret);
+			break;
+		}
 	}
 
 	mutex_unlock(&data->lock);
-- 
2.55.0

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

[Jonathan Cameron]

On Tue,  4 Aug 2026 19:43:55 -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
> 
> Read the interrupt status register at the top of the handler. If
> FIFO_RDY is not set, return early because it isn't our interrupt.
> Because FIFO_RDY is the only interrupt source enabled by this driver in
> max30102_chip_init(), an invocation of max30102_interrupt_handler()
> without FIFO_RDY set carries no data to read and can return early before
> ever accessing active_scan_mask. This status read also deasserts the
> MAX30102's active-low interrupt pin.
> 
> Since the interrupt status register is now read at the top of the
> handler, pass the status value into max30102_fifo_count() instead of
> having it read the register a second time.
> 
> Fixes: 90579b69e94b ("iio: health: max30102: Add MAX30105 support")
> Signed-off-by: Marco Chen <marcochen.dev@gmail.com>
> ---
> Changes in v2:
> - Check FIFO_RDY bit in the interrupt status register instead of 
>   active_scan_mask, because that was racy in v1, as pointed out by 
>   Jonathan and David.
> - Read interrupt status once and pass it into max30102_fifo_count().
> - Check the regmap_read() return value and log on failure.
> 
> Tested with a MAX30102 on a Raspberry Pi 4 over I2C.
> v1: https://lore.kernel.org/linux-iio/20260731184124.112124-1-marcochen.dev@gmail.com/
> 
>  drivers/iio/health/max30102.c | 37 +++++++++++++++++++++++------------
>  1 file changed, 25 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/health/max30102.c b/drivers/iio/health/max30102.c
> index c37316c86f14..b949ce36c248 100644
> --- a/drivers/iio/health/max30102.c
> +++ b/drivers/iio/health/max30102.c
> @@ -235,17 +235,10 @@ static const struct iio_buffer_setup_ops max30102_buffer_setup_ops = {
>  	.predisable = max30102_buffer_predisable,
>  };
>  
> -static inline int max30102_fifo_count(struct max30102_data *data)
> +static inline int max30102_fifo_count(unsigned int status)

Not as such related to this patch, but would be nice if this
was renamed to something that reflected it doesn't really provide
any form of counting and only returns 0 or 1.
Would be nicer as a bool.


>  {
> -	unsigned int val;
> -	int ret;
> -
> -	ret = regmap_read(data->regmap, MAX30102_REG_INT_STATUS, &val);
> -	if (ret)
> -		return ret;
> -
>  	/* FIFO has one sample slot left */
> -	if (val & MAX30102_REG_INT_STATUS_FIFO_RDY)
> +	if (status & MAX30102_REG_INT_STATUS_FIFO_RDY)
>  		return 1;
>  
>  	return 0;
> @@ -290,19 +283,39 @@ 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, status;
>  	int ret, cnt = 0;
>  
> +	ret = regmap_read(data->regmap, MAX30102_REG_INT_STATUS, &status);
> +	if (ret) {
> +		dev_err_ratelimited(&data->client->dev,
> +				    "Failed to read IRQ status: %d\n", ret);
> +		return IRQ_HANDLED;
> +	}
> +	if (!(status & MAX30102_REG_INT_STATUS_FIFO_RDY))
> +		return IRQ_HANDLED;

This chunk above is replicating the oddly named max30102_fifo_count() more
or less. I'd leave that functon as it stood before and do

	cnt = max30102_fifo_count();
	if (cnt == 0)
		return IRQ_HANDLED;

	measurements = bitmap_weight(indio_dev->active_scan_mask,
				     iio_get_masklength(indio_dev));

	mutex_lock()
	while (cnt || (cnt = max30102_fifo_count(data)) > 0) {

as the second part of that won't be evaluated on first entry as
we know cnt is set.

> +
> +	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) {
> +	while (cnt || (cnt = max30102_fifo_count(status)) > 0) {
>  		ret = max30102_read_measurement(data, measurements);
>  		if (ret)
>  			break;
>  
>  		iio_push_to_buffers(data->indio_dev, data->processed_buffer);
>  		cnt--;
> +
> +		ret = regmap_read(data->regmap, MAX30102_REG_INT_STATUS,
> +				  &status);
> +		if (ret) {
> +			dev_err_ratelimited(&data->client->dev,
> +					    "Failed to read IRQ status: %d\n",
> +					    ret);
> +			break;
> +		}
With the above, I don't think this part is needed.

Jonathan

>  	}
>  
>  	mutex_unlock(&data->lock);