Re: [PATCH v7 2/2] thermal/drivers/imx: Add calibration offset support

[sashiko-bot@kernel.org]

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

New issues:
- [High] An unbounded DT property causes integer underflow in the alarm calculation, leading to out-of-bounds register bit manipulation.

Pre-existing issues:
- [High] Use-after-free of `data->tz` in the thermal alarm IRQ thread during device removal.
--

--- Patch [2]: [PATCH v7 2/2] thermal/drivers/imx: Add calibration offset support ---
commit 09bf5f606220dc9c50fce508974c8440db8631c3
Author: Haoning CHENG <Haoning.CHENG@cn.bosch.com>

thermal/drivers/imx: Add calibration offset support

This patch introduces support for a per-design temperature calibration
offset to align reported CPU temperatures with board-level measurements.
It reads the fsl,temp-calibration-offset-millicelsius property from the
device tree and applies it to the calibration formulas.

> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index 5aaacbc534786..5e3cff69aced7 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c

[ ... ]

> @@ -223,6 +224,7 @@ static void imx_set_panic_temp(struct imx_thermal_data *data,
>  	struct regmap *map = data->tempmon;
>  	int critical_value;
>  
> +	panic_temp -= data->calibration_offset;
>  	critical_value = (data->c2 - panic_temp) / data->c1;
>  
>  	regmap_write(map, soc_data->panic_alarm_ctrl + REG_CLR,

[Severity: High]
Does subtracting an unvalidated calibration_offset allow critical_value to
exceed the register bitfield width?

If the device tree provides an extremely large positive or negative value,
critical_value could overflow or underflow. When shifted and written to
REG_SET:

    regmap_write(map, soc_data->panic_alarm_ctrl + REG_SET,
                 critical_value << soc_data->panic_alarm_shift);

Could this out-of-bounds value pollute adjacent register fields, since REG_SET
applies any bits that are 1? Should data->calibration_offset be clamped before
use?

> @@ -239,11 +241,14 @@ static void imx_set_alarm_temp(struct imx_thermal_data *data,
>  	int alarm_temp)
>  	int alarm_value;
>  
>  	data->alarm_temp = alarm_temp;
> +	alarm_temp -= data->calibration_offset;
>  
> -	if (data->socdata->version == TEMPMON_IMX7D)
> -		alarm_value = alarm_temp / 1000 + data->c1 - 25;
> -	else
> +	if (data->socdata->version == TEMPMON_IMX7D) {
> +		alarm_value = DIV_ROUND_UP(alarm_temp, 1000) + data->c1 - 25;
> +		alarm_value = clamp(alarm_value, 0, 0x1ff);
> +	} else {
>  		alarm_value = (data->c2 - alarm_temp) / data->c1;
> +	}
>  
>  	regmap_write(map, soc_data->high_alarm_ctrl + REG_CLR,
>  		     soc_data->high_alarm_mask);

[Severity: High]
Can alarm_value overflow the register bitfield for non-IMX7D variants?

The IMX7D variant uses clamp(), but the else branch does not:

    } else {
        alarm_value = (data->c2 - alarm_temp) / data->c1;
    }

Since alarm_temp is adjusted by an unbounded calibration_offset, this could
result in an out-of-bounds alarm_value that pollutes adjacent fields when
written to soc_data->high_alarm_ctrl.

[ ... ]

> @@ -629,6 +635,10 @@ static int imx_thermal_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, data);
>  
> +	of_property_read_s32(dev->of_node,
> +			     "fsl,temp-calibration-offset-millicelsius",
> +			     &data->calibration_offset);
> +
>  	if (of_property_present(dev->of_node, "nvmem-cells")) {
>  		ret = imx_init_from_nvmem_cells(pdev);
>  		if (ret)

[Severity: High]
This is a pre-existing issue, but does this driver have a use-after-free
during device removal?

The driver requests its interrupt via devm_request_threaded_irq() in
imx_thermal_probe(). Because it uses devres, the IRQ is freed after
imx_thermal_remove() returns.

However, imx_thermal_remove() manually unregisters the thermal zone:

imx_thermal_remove()
    ...
    thermal_zone_device_unregister(data->tz);

This frees data->tz. If the hardware thermal alarm triggers after this
unregister but before the devres cleanup, the IRQ handler
imx_thermal_alarm_irq_thread() will execute:

imx_thermal_alarm_irq_thread()
    ...
    thermal_zone_device_update(data->tz, THERMAL_EVENT_UNSPECIFIED);

Could this dereference the freed data->tz pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-b4-symana21-11221-imx-thermal-support-upstream-6-18-v7-0-09b6b7669be1@cn.bosch.com?part=2