Re: [PATCH v5 7/7] thermal/drivers: airoha: Add support for AN7583 Thermal Sensor

[sashiko-bot@kernel.org]

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

New issues:
- [High] Potential 32-bit signed integer overflow in temperature calculation.
- [Medium] The MUX_SENSOR register field width is too narrow to hold all defined sensor indices, leading to silent truncation.

Pre-existing issues:
- [Critical] Missing `remove` callback causes devres Use-After-Free of thermal zone.
- [High] Unprotected read-modify-write sequence on a shared syscon register creates a race condition.
- [High] Unchecked regmap read operations lead to use of uninitialized stack variables.
- [High] IRQ requested before thermal zone pointer is initialized leads to potential NULL pointer dereference.
- [High] Missing NULL check on `device_get_match_data()` leads to kernel panic on sysfs bind.
--

commit 57a01e256a4324da608f512eaa2939419b755b71
Author: Christian Marangi <ansuelsmth@gmail.com>

thermal/drivers: airoha: Add support for AN7583 Thermal Sensor

Add support for Airoha AN7583 Thermal driver. This apply similar logic
on how to read the temperature but totally drop support for the
PTP_THERMAL subsystem.

> diff --git a/drivers/thermal/airoha_thermal.c b/drivers/thermal/airoha_thermal.c
> index 249abbbd46bcd..c84b5c36e8803 100644
> --- a/drivers/thermal/airoha_thermal.c
> +++ b/drivers/thermal/airoha_thermal.c
[ ... ]
>  static void airoha_set_thermal_mux(struct airoha_thermal_priv *priv,
> -				   int tdac_idx)
> +				   int tdac_idx, int sensor_idx)
>  {
>  	u32 pllrg;
>  
[ ... ]
>  	/* Configure Thermal ADC mux to tdac_idx */
> -	regmap_field_write(priv->chip_scu_fields[AIROHA_THERMAL_MUX_TADC],
> -			   tdac_idx);
> +	if (priv->current_adc != tdac_idx) {
> +		regmap_field_write(priv->chip_scu_fields[AIROHA_THERMAL_MUX_TADC],
> +				   tdac_idx);
> +		priv->current_adc = tdac_idx;
> +	}
>  
>  	/* Restore PLLRG value on exit */
>  	regmap_write(priv->chip_scu, EN7581_PLLRG_PROTECT, pllrg);

[Severity: High]
This is a pre-existing issue, but since this patch significantly increases
the frequency of this function for runtime polling, is there a race
condition with the read-modify-write sequence on the shared syscon register?

The regmap lock only protects individual reads and writes, making the
unlock-modify-lock sequence on EN7581_PLLRG_PROTECT non-atomic. If another
driver sharing the SCU performs operations concurrently, could they race and
leave the protection register permanently unlocked or restore an incorrect
state?

[Severity: High]
This is a pre-existing issue, but does the unchecked regmap read
operation lead to using an uninitialized stack variable?

In airoha_set_thermal_mux(), if the regmap_read() for EN7581_PLLRG_PROTECT
fails, the pllrg variable remains uninitialized and is later written back to
the hardware protection register.

Similarly, in airoha_get_thermal_ADC(), an unchecked regmap_field_read()
leaves the stack variable uninitialized before being returned and used in
temperature calculations.

[ ... ]
> +static int an7583_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
> +{
> +	struct airoha_thermal_priv *priv = thermal_zone_device_priv(tz);
> +	int sensor_idx;
> +	int delta_diode, delta_gain;
> +	int coeff, slope, offset;
> +
> +	int diode_zero, diode_d0, diode_d1;
> +
[ ... ]
> +	delta_diode = diode_d1 - diode_d0;
> +	delta_gain = (delta_diode * coeff) / 100 + (diode_zero - diode_d1);
> +	if (!delta_gain)
> +		return -EINVAL;
> +
> +	*temp = (slope * delta_diode * 10) / delta_gain - offset * 10;
> +	*temp *= 100;
> +
> +	return 0;
> +}

[Severity: High]
Can the multiplication of slope and delta_diode overflow a 32-bit signed
integer?

The slope variable can be up to 8390, and delta_diode is derived from the
difference of two 32-bit ADC readings. A delta_diode value exceeding 25,595
would cause the (slope * delta_diode * 10) calculation to overflow the signed
32-bit maximum limit (2,147,483,647), leading to undefined behavior and
wildly incorrect temperature readings.

[ ... ]
> +static const struct thermal_zone_device_ops an7583_tz_ops = {
> +	.get_temp = an7583_thermal_get_temp,
> +};
> +
> +static const struct reg_field an7583_chip_scu_fields[AIROHA_THERMAL_FIELD_MAX] = {
> +	[AIROHA_THERMAL_DOUT_TADC] = REG_FIELD(AN7583_DOUT_TADC, 0, 31),
> +	[AIROHA_THERMAL_MUX_TADC] = REG_FIELD(AN7583_MUX_TADC, 1, 3),
> +	[AIROHA_THERMAL_MUX_SENSOR] = REG_FIELD(AN7583_MUX_SENSOR, 2, 3),
> +};

[Severity: Medium]
Is the AIROHA_THERMAL_MUX_SENSOR register field width too narrow to hold all
defined sensor indices?

The field is defined as a 2-bit field (bits 2 to 3), which can hold a
maximum value of 3. However, the driver enumerates sensor indices up to 6
(AN7583_CPU_TEMP_SENSOR = 6). If the driver is extended to use these other
sensors, calling regmap_field_write() with values larger than 3 will silently
truncate the values, causing the hardware to sample the wrong sensor.

[ ... ]
>  static int airoha_thermal_probe(struct platform_device *pdev)
>  {
>  	const struct airoha_thermal_soc_data *soc_data;
>  	struct airoha_thermal_priv *priv;
>  	struct device *dev = &pdev->dev;
>  	int ret;
>  
>  	soc_data = device_get_match_data(dev);
>  
>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
>  
>  	priv->pllrg_protect = soc_data->pllrg_protect;

[Severity: High]
This is a pre-existing issue, but could device_get_match_data() return NULL
and cause a kernel panic here?

If the driver is manually bound via sysfs to a device that lacks an
associated device tree node, device_get_match_data() returns NULL, leading
to a NULL pointer dereference when accessing soc_data->pllrg_protect.

> +	priv->current_adc = -1;
>  
>  	if (!soc_data->probe)
>  		return -EINVAL;
>  
>  	ret = soc_data->probe(pdev, priv);
>  	if (ret)
>  		return ret;
>  
>  	/* register of thermal sensor and get info from DT */
>  	priv->tz = devm_thermal_of_zone_register(dev, 0, priv,
>  						 soc_data->thdev_ops);

[Severity: High]
This is a pre-existing issue, but is the IRQ requested before the thermal
zone pointer is initialized?

In en7581_thermal_probe(), which is called by soc_data->probe(), the hardware
interrupt is registered via devm_request_threaded_irq() before priv->tz is
assigned here. If an interrupt fires immediately, en7581_thermal_irq() will
execute and pass the uninitialized NULL priv->tz to
thermal_zone_device_update(), causing a kernel panic.

[Severity: Critical]
This is a pre-existing issue, but does the lack of a remove callback cause
a use-after-free of the thermal zone?

The driver enables hardware interrupts but does not provide a remove function
to disable them. On module unload or manual unbind, devres cleans up
resources in reverse order: the thermal zone priv->tz is unregistered and
freed before the threaded IRQ handler is freed. If the interrupt fires
during this unbind window, en7581_thermal_irq() executes and accesses the
freed thermal zone.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520155525.22239-1-ansuelsmth@gmail.com?part=7