[PATCH] hwmon: chipcap2: serialize access to low/high_alarm indicators

[PATCH] hwmon: chipcap2: serialize access to low/high_alarm indicators

[Javier Carrasco]

Access to low_alarm and high_alarm from the threaded interrupt handlers
and sysfs is not protected by any locking mechanism at the moment, which
can lead to missed events.

Add a per-interrupt mutex as the two alarm indicators are completely
independent.

Fixes: 3af350929e75 ("hwmon: Add support for Amphenol ChipCap 2")
Cc: stable@vger.kernel.org
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
This issue was found by Sashiko[1] when an unrelated patch affected
chipcap2.c. The issue is real as the access to the variables is not
protected by any locking mechanism although 2 different sources
(threaded interrupts and sysfs) could modify them.

The fix has been validated on real hardware with an Amphenol
ChipCap 2 CC2D23S sensor.

Link: [1] https://lore.kernel.org/linux-hwmon/20260625162114.417EA1F000E9@smtp.kernel.org/
---
 drivers/hwmon/chipcap2.c | 38 ++++++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
index 086571d556b7..1bb4ec96514e 100644
--- a/drivers/hwmon/chipcap2.c
+++ b/drivers/hwmon/chipcap2.c
@@ -73,7 +73,11 @@
 
 struct cc2_rh_alarm_info {
 	bool low_alarm;
+	/* Serialize accesses to low_alarm from threaded IRQ and sysfs */
+	struct mutex low_alarm_lock;
 	bool high_alarm;
+	/* Serialize accesses to high_alarm from threaded IRQ and sysfs */
+	struct mutex high_alarm_lock;
 	bool low_alarm_visible;
 	bool high_alarm_visible;
 };
@@ -500,6 +504,7 @@ static irqreturn_t cc2_low_interrupt(int irq, void *data)
 	if (cc2->process_irqs) {
 		hwmon_notify_event(cc2->hwmon, hwmon_humidity,
 				   hwmon_humidity_min_alarm, CC2_CHAN_HUMIDITY);
+		guard(mutex)(&cc2->rh_alarm.low_alarm_lock);
 		cc2->rh_alarm.low_alarm = true;
 	}
 
@@ -513,6 +518,7 @@ static irqreturn_t cc2_high_interrupt(int irq, void *data)
 	if (cc2->process_irqs) {
 		hwmon_notify_event(cc2->hwmon, hwmon_humidity,
 				   hwmon_humidity_max_alarm, CC2_CHAN_HUMIDITY);
+		guard(mutex)(&cc2->rh_alarm.high_alarm_lock);
 		cc2->rh_alarm.high_alarm = true;
 	}
 
@@ -529,11 +535,13 @@ static int cc2_humidity_min_alarm_status(struct cc2_data *data, long *val)
 	if (ret < 0)
 		return ret;
 
-	if (data->rh_alarm.low_alarm) {
-		*val = (measurement < min_hyst) ? 1 : 0;
-		data->rh_alarm.low_alarm = *val;
-	} else {
-		*val = 0;
+	scoped_guard(mutex, &data->rh_alarm.low_alarm_lock) {
+		if (data->rh_alarm.low_alarm) {
+			*val = (measurement < min_hyst) ? 1 : 0;
+			data->rh_alarm.low_alarm = *val;
+		} else {
+			*val = 0;
+		}
 	}
 
 	return 0;
@@ -549,11 +557,13 @@ static int cc2_humidity_max_alarm_status(struct cc2_data *data, long *val)
 	if (ret < 0)
 		return ret;
 
-	if (data->rh_alarm.high_alarm) {
-		*val = (measurement > max_hyst) ? 1 : 0;
-		data->rh_alarm.high_alarm = *val;
-	} else {
-		*val = 0;
+	scoped_guard(mutex, &data->rh_alarm.high_alarm_lock) {
+		if (data->rh_alarm.high_alarm) {
+			*val = (measurement > max_hyst) ? 1 : 0;
+			data->rh_alarm.high_alarm = *val;
+		} else {
+			*val = 0;
+		}
 	}
 
 	return 0;
@@ -720,6 +730,14 @@ static int cc2_probe(struct i2c_client *client)
 	if (!data)
 		return -ENOMEM;
 
+	ret = devm_mutex_init(dev, &data->rh_alarm.low_alarm_lock);
+	if (ret)
+		return ret;
+
+	ret = devm_mutex_init(dev, &data->rh_alarm.high_alarm_lock);
+	if (ret)
+		return ret;
+
 	i2c_set_clientdata(client, data);
 
 	data->client = client;

---
base-commit: 818bebeb63dd6bf5f4e07e145f6cdbace520a34c
change-id: 20260820-chipcap2_locks-c01013a24a0c

Best regards,
-- 
Javier Carrasco <javier.carrasco.cruz@gmail.com>

Re: [PATCH] hwmon: chipcap2: serialize access to low/high_alarm indicators

[sashiko-bot]

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

New issues:
- [Low] The driver implements custom locks for interrupt synchronization instead of using the hwmon subsystem's mandated locking API.

Pre-existing issues:
- [High] The threaded interrupt handlers pass the wrong channel index to hwmon_notify_event(), causing the sysfs notification to fail silently.
- [High] Threaded IRQs are requested before the hwmon device is fully registered, leading to a Use-After-Free (UAF) during driver teardown.
- [High] The driver permanently times out on all measurements if the regulator is already enabled at boot or if a previous disable failed.
--

--- Patch [1]: [PATCH] hwmon: chipcap2: serialize access to low/high_alarm indicators ---
commit f294324394a3b551fc4139386f0f74282d1fdb24
Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>

hwmon: chipcap2: serialize access to low/high_alarm indicators

This patch adds per-interrupt mutexes to protect low_alarm and high_alarm
accesses from threaded interrupt handlers and sysfs, fixing a potential race
condition that could lead to missed events.

Link: https://lore.kernel.org/linux-hwmon/20260625162114.417EA1F000E9@smtp.kernel.org/

Note: This review is provided in a free-form summary format without direct
inline patch quoting due to recitation restrictions.

[Severity: Low]
Is it correct to introduce custom per-alarm mutexes for interrupt
synchronization here? Looking at the hwmon subsystem guidelines, drivers
should use the mandated locking API via hwmon_lock(dev) and hwmon_unlock(dev)
instead of defining and using custom locks like low_alarm_lock and
high_alarm_lock in struct cc2_rh_alarm_info and cc2_low_interrupt().

[Severity: High]
This isn't a bug introduced by this patch, but does the threaded interrupt
handler pass the correct channel index to hwmon_notify_event()?

In cc2_low_interrupt(), hwmon_notify_event() receives CC2_CHAN_HUMIDITY
(which is 1). The hwmon core adds this to the humidity base index (1),
looking for humidity2_min_alarm.

However, the driver only registers one humidity channel (index 0), which is
created as humidity1_min_alarm. Will this cause the sysfs notification to
fail silently when an alarm fires?

[Severity: High]
This is a pre-existing issue, but is there a teardown ordering problem with
devres in cc2_probe()?

The threaded IRQs are requested via cc2_request_ready_irq() before
devm_hwmon_device_register_with_info() is called. During devres unbind,
resources are released in LIFO order, meaning the hwmon device is
unregistered and freed while the IRQs are still active.

Concurrent sysfs reads during unbind could re-enable process_irqs. If an
interrupt fires during this window, could it dereference the freed
cc2->hwmon pointer, resulting in a use-after-free?

[Severity: High]
This isn't a bug introduced by this patch, but does the driver permanently
time out on measurements if the regulator is already enabled?

In cc2_enable(), if regulator_is_enabled() returns true (e.g., for an
always-on DT regulator), the function returns 0 immediately without setting
data->process_irqs = true.

As a result, cc2_ready_interrupt() will ignore all interrupts, and the
measurement wait will unconditionally time out. Should data->process_irqs
be set even if the regulator is already enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821-chipcap2_locks-v1-1-1d8ccabfc1b2@gmail.com?part=1

Re: [PATCH] hwmon: chipcap2: serialize access to low/high_alarm indicators

[Javier Carrasco]

Some extra homework, but the issues are real and I am open to tackle
them all.

On Fri Aug 21, 2026 at 11:37 AM CEST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
>
> New issues:
> - [Low] The driver implements custom locks for interrupt synchronization instead of using the hwmon subsystem's mandated locking API.
>
> Pre-existing issues:
> - [High] The threaded interrupt handlers pass the wrong channel index to hwmon_notify_event(), causing the sysfs notification to fail silently.
> - [High] Threaded IRQs are requested before the hwmon device is fully registered, leading to a Use-After-Free (UAF) during driver teardown.
> - [High] The driver permanently times out on all measurements if the regulator is already enabled at boot or if a previous disable failed.
> --
>
> --- Patch [1]: [PATCH] hwmon: chipcap2: serialize access to low/high_alarm indicators ---
> commit f294324394a3b551fc4139386f0f74282d1fdb24
> Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>
> hwmon: chipcap2: serialize access to low/high_alarm indicators
>
> This patch adds per-interrupt mutexes to protect low_alarm and high_alarm
> accesses from threaded interrupt handlers and sysfs, fixing a potential race
> condition that could lead to missed events.
>
> Link: https://lore.kernel.org/linux-hwmon/20260625162114.417EA1F000E9@smtp.kernel.org/
>
> Note: This review is provided in a free-form summary format without direct
> inline patch quoting due to recitation restrictions.
>
> [Severity: Low]
> Is it correct to introduce custom per-alarm mutexes for interrupt
> synchronization here? Looking at the hwmon subsystem guidelines, drivers
> should use the mandated locking API via hwmon_lock(dev) and hwmon_unlock(dev)
> instead of defining and using custom locks like low_alarm_lock and
> high_alarm_lock in struct cc2_rh_alarm_info and cc2_low_interrupt().
>

I used two mutexes to decouple the alarms as they are independent, so
actions taken for any of them is not delayed by the other. On the other
hand, that delay will be very short, so I am open to use hwmon_lock(dev)
if it is preferred in this case too.

> [Severity: High]
> This isn't a bug introduced by this patch, but does the threaded interrupt
> handler pass the correct channel index to hwmon_notify_event()?
>
> In cc2_low_interrupt(), hwmon_notify_event() receives CC2_CHAN_HUMIDITY
> (which is 1). The hwmon core adds this to the humidity base index (1),
> looking for humidity2_min_alarm.
>
> However, the driver only registers one humidity channel (index 0), which is
> created as humidity1_min_alarm. Will this cause the sysfs notification to
> fail silently when an alarm fires?
>

This is a real issue. I will remove the enum with the channel definition
because it is only used here and pass 0 instead (there is only one
humidity channel).

> [Severity: High]
> This is a pre-existing issue, but is there a teardown ordering problem with
> devres in cc2_probe()?
>
> The threaded IRQs are requested via cc2_request_ready_irq() before
> devm_hwmon_device_register_with_info() is called. During devres unbind,
> resources are released in LIFO order, meaning the hwmon device is
> unregistered and freed while the IRQs are still active.
>
> Concurrent sysfs reads during unbind could re-enable process_irqs. If an
> interrupt fires during this window, could it dereference the freed
> cc2->hwmon pointer, resulting in a use-after-free?
>

I will analyze this in more detail and get back with the results.

> [Severity: High]
> This isn't a bug introduced by this patch, but does the driver permanently
> time out on measurements if the regulator is already enabled?
>
> In cc2_enable(), if regulator_is_enabled() returns true (e.g., for an
> always-on DT regulator), the function returns 0 immediately without setting
> data->process_irqs = true.
>
> As a result, cc2_ready_interrupt() will ignore all interrupts, and the
> measurement wait will unconditionally time out. Should data->process_irqs
> be set even if the regulator is already enabled?


I will analyze this in more detail and get back with the results.

Best regards,
Javier

Re: [PATCH] hwmon: chipcap2: serialize access to low/high_alarm indicators

[Guenter Roeck]

On 8/21/26 03:16, Javier Carrasco wrote:
> Some extra homework, but the issues are real and I am open to tackle
> them all.
> 
> On Fri Aug 21, 2026 at 11:37 AM CEST, sashiko-bot wrote:
>> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
>>
>> New issues:
>> - [Low] The driver implements custom locks for interrupt synchronization instead of using the hwmon subsystem's mandated locking API.
>>
>> Pre-existing issues:
>> - [High] The threaded interrupt handlers pass the wrong channel index to hwmon_notify_event(), causing the sysfs notification to fail silently.
>> - [High] Threaded IRQs are requested before the hwmon device is fully registered, leading to a Use-After-Free (UAF) during driver teardown.
>> - [High] The driver permanently times out on all measurements if the regulator is already enabled at boot or if a previous disable failed.
>> --
>>
>> --- Patch [1]: [PATCH] hwmon: chipcap2: serialize access to low/high_alarm indicators ---
>> commit f294324394a3b551fc4139386f0f74282d1fdb24
>> Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>>
>> hwmon: chipcap2: serialize access to low/high_alarm indicators
>>
>> This patch adds per-interrupt mutexes to protect low_alarm and high_alarm
>> accesses from threaded interrupt handlers and sysfs, fixing a potential race
>> condition that could lead to missed events.
>>
>> Link: https://lore.kernel.org/linux-hwmon/20260625162114.417EA1F000E9@smtp.kernel.org/
>>
>> Note: This review is provided in a free-form summary format without direct
>> inline patch quoting due to recitation restrictions.
>>
>> [Severity: Low]
>> Is it correct to introduce custom per-alarm mutexes for interrupt
>> synchronization here? Looking at the hwmon subsystem guidelines, drivers
>> should use the mandated locking API via hwmon_lock(dev) and hwmon_unlock(dev)
>> instead of defining and using custom locks like low_alarm_lock and
>> high_alarm_lock in struct cc2_rh_alarm_info and cc2_low_interrupt().
>>
> 
> I used two mutexes to decouple the alarms as they are independent, so
> actions taken for any of them is not delayed by the other. On the other
> hand, that delay will be very short, so I am open to use hwmon_lock(dev)
> if it is preferred in this case too.
> 

Please do. Introducing separate locks for low frequency events such as this one
is overkill.

>> [Severity: High]
>> This isn't a bug introduced by this patch, but does the threaded interrupt
>> handler pass the correct channel index to hwmon_notify_event()?
>>
>> In cc2_low_interrupt(), hwmon_notify_event() receives CC2_CHAN_HUMIDITY
>> (which is 1). The hwmon core adds this to the humidity base index (1),
>> looking for humidity2_min_alarm.
>>
>> However, the driver only registers one humidity channel (index 0), which is
>> created as humidity1_min_alarm. Will this cause the sysfs notification to
>> fail silently when an alarm fires?
>>
> 
> This is a real issue. I will remove the enum with the channel definition
> because it is only used here and pass 0 instead (there is only one
> humidity channel).
> 
>> [Severity: High]
>> This is a pre-existing issue, but is there a teardown ordering problem with
>> devres in cc2_probe()?
>>
>> The threaded IRQs are requested via cc2_request_ready_irq() before
>> devm_hwmon_device_register_with_info() is called. During devres unbind,
>> resources are released in LIFO order, meaning the hwmon device is
>> unregistered and freed while the IRQs are still active.
>>
>> Concurrent sysfs reads during unbind could re-enable process_irqs. If an
>> interrupt fires during this window, could it dereference the freed
>> cc2->hwmon pointer, resulting in a use-after-free?
>>
> 
> I will analyze this in more detail and get back with the results.
> 
>> [Severity: High]
>> This isn't a bug introduced by this patch, but does the driver permanently
>> time out on measurements if the regulator is already enabled?
>>
>> In cc2_enable(), if regulator_is_enabled() returns true (e.g., for an
>> always-on DT regulator), the function returns 0 immediately without setting
>> data->process_irqs = true.
>>
>> As a result, cc2_ready_interrupt() will ignore all interrupts, and the
>> measurement wait will unconditionally time out. Should data->process_irqs
>> be set even if the regulator is already enabled?
> 
> 
> I will analyze this in more detail and get back with the results.
> 
Thanks!

Guenter

Re: [PATCH] hwmon: chipcap2: serialize access to low/high_alarm indicators

[Guenter Roeck]

On 8/21/26 02:22, Javier Carrasco wrote:
> Access to low_alarm and high_alarm from the threaded interrupt handlers
> and sysfs is not protected by any locking mechanism at the moment, which
> can lead to missed events.
> 
> Add a per-interrupt mutex as the two alarm indicators are completely
> independent.
> 
> Fixes: 3af350929e75 ("hwmon: Add support for Amphenol ChipCap 2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
> This issue was found by Sashiko[1] when an unrelated patch affected
> chipcap2.c. The issue is real as the access to the variables is not
> protected by any locking mechanism although 2 different sources
> (threaded interrupts and sysfs) could modify them.
> 
> The fix has been validated on real hardware with an Amphenol
> ChipCap 2 CC2D23S sensor.
> 

Needs explanation: Why can the hwmon subsystem lock not be used ?

Guenter

> Link: [1] https://lore.kernel.org/linux-hwmon/20260625162114.417EA1F000E9@smtp.kernel.org/
> ---
>   drivers/hwmon/chipcap2.c | 38 ++++++++++++++++++++++++++++----------
>   1 file changed, 28 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
> index 086571d556b7..1bb4ec96514e 100644
> --- a/drivers/hwmon/chipcap2.c
> +++ b/drivers/hwmon/chipcap2.c
> @@ -73,7 +73,11 @@
>   
>   struct cc2_rh_alarm_info {
>   	bool low_alarm;
> +	/* Serialize accesses to low_alarm from threaded IRQ and sysfs */
> +	struct mutex low_alarm_lock;
>   	bool high_alarm;
> +	/* Serialize accesses to high_alarm from threaded IRQ and sysfs */
> +	struct mutex high_alarm_lock;
>   	bool low_alarm_visible;
>   	bool high_alarm_visible;
>   };
> @@ -500,6 +504,7 @@ static irqreturn_t cc2_low_interrupt(int irq, void *data)
>   	if (cc2->process_irqs) {
>   		hwmon_notify_event(cc2->hwmon, hwmon_humidity,
>   				   hwmon_humidity_min_alarm, CC2_CHAN_HUMIDITY);
> +		guard(mutex)(&cc2->rh_alarm.low_alarm_lock);
>   		cc2->rh_alarm.low_alarm = true;
>   	}
>   
> @@ -513,6 +518,7 @@ static irqreturn_t cc2_high_interrupt(int irq, void *data)
>   	if (cc2->process_irqs) {
>   		hwmon_notify_event(cc2->hwmon, hwmon_humidity,
>   				   hwmon_humidity_max_alarm, CC2_CHAN_HUMIDITY);
> +		guard(mutex)(&cc2->rh_alarm.high_alarm_lock);
>   		cc2->rh_alarm.high_alarm = true;
>   	}
>   
> @@ -529,11 +535,13 @@ static int cc2_humidity_min_alarm_status(struct cc2_data *data, long *val)
>   	if (ret < 0)
>   		return ret;
>   
> -	if (data->rh_alarm.low_alarm) {
> -		*val = (measurement < min_hyst) ? 1 : 0;
> -		data->rh_alarm.low_alarm = *val;
> -	} else {
> -		*val = 0;
> +	scoped_guard(mutex, &data->rh_alarm.low_alarm_lock) {
> +		if (data->rh_alarm.low_alarm) {
> +			*val = (measurement < min_hyst) ? 1 : 0;
> +			data->rh_alarm.low_alarm = *val;
> +		} else {
> +			*val = 0;
> +		}
>   	}
>   
>   	return 0;
> @@ -549,11 +557,13 @@ static int cc2_humidity_max_alarm_status(struct cc2_data *data, long *val)
>   	if (ret < 0)
>   		return ret;
>   
> -	if (data->rh_alarm.high_alarm) {
> -		*val = (measurement > max_hyst) ? 1 : 0;
> -		data->rh_alarm.high_alarm = *val;
> -	} else {
> -		*val = 0;
> +	scoped_guard(mutex, &data->rh_alarm.high_alarm_lock) {
> +		if (data->rh_alarm.high_alarm) {
> +			*val = (measurement > max_hyst) ? 1 : 0;
> +			data->rh_alarm.high_alarm = *val;
> +		} else {
> +			*val = 0;
> +		}
>   	}
>   
>   	return 0;
> @@ -720,6 +730,14 @@ static int cc2_probe(struct i2c_client *client)
>   	if (!data)
>   		return -ENOMEM;
>   
> +	ret = devm_mutex_init(dev, &data->rh_alarm.low_alarm_lock);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_mutex_init(dev, &data->rh_alarm.high_alarm_lock);
> +	if (ret)
> +		return ret;
> +
>   	i2c_set_clientdata(client, data);
>   
>   	data->client = client;
> 
> ---
> base-commit: 818bebeb63dd6bf5f4e07e145f6cdbace520a34c
> change-id: 20260820-chipcap2_locks-c01013a24a0c
> 
> Best regards,