Re: [PATCH v5 4/7] thermal: qcom: tsens: Add support for MSM8952 tsens

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] thermal: qcom: tsens: OOB write in init_8952() due to missing DT bounds check
- [High] thermal: qcom: tsens: UAF in probe due to __init on init_8952
- [Medium] thermal: qcom: tsens: hardware interrupt left enabled on probe error
--

commit 348b40706a606c87400c1bc3052d8b80b1d7ed76
Author: Muzaffer Kadir <muzafferkadir@mainlining.org>

thermal: qcom: tsens: Add support for MSM8952 tsens

This commit adds support for the TSENS v1 block found on the MSM8952 SoC. It
introduces initialization functions, operations, and platform data for the 11
sensors present on the device. Sensor offsets are adapted from downstream
sources.

Link: https://android.googlesource.com/kernel/msm.git/+/refs/tags/android-wear-7.0.0_r0.3/drivers/thermal/msm-tsens.c#1621

> diff --git a/drivers/thermal/qcom/tsens-v1.c b/drivers/thermal/qcom/tsens-v1.c
> index faa5d00788ca6..7bda8e6087c40 100644
> --- a/drivers/thermal/qcom/tsens-v1.c
> +++ b/drivers/thermal/qcom/tsens-v1.c
> @@ -145,6 +145,34 @@ static const struct reg_field tsens_v1_regfields[MAX_REGFIELDS] = {
>  	[TRDY] = REG_FIELD(TM_TRDY_OFF, 0, 0),
>  };
>  
> +static int __init init_8952(struct tsens_priv *priv)

[Severity: High]
Does marking this new initialization callback with __init lead to a
use-after-free during a late probe?

The __init attribute means this function's memory is discarded after system
or module initialization.

The tsens_probe() function is not marked __init and executes
priv->ops->init(priv) during its setup phase.

If the platform device defers probing until after .init.text is freed, or
if a user unbinds and rebinds the driver via sysfs, will tsens_probe() jump
to the freed function pointer and cause a use-after-free?

> +{
> +	priv->sensor[0].p1_calib_offset = 2;
> +	priv->sensor[0].p2_calib_offset = -1;

[ ... ]

> +	priv->sensor[9].p2_calib_offset = 0;
> +	priv->sensor[10].p1_calib_offset = -3;
> +	priv->sensor[10].p2_calib_offset = -2;

[Severity: High]
Can this code overflow priv->sensor[]?

In tsens_probe(), the flexible array priv->sensor is dynamically allocated
based on the #qcom,sensors device tree property:

drivers/thermal/qcom/tsens.c:tsens_probe() {
	...
	if (np)
		of_property_read_u32(np, "#qcom,sensors", &num_sensors);
	...
	priv = devm_kzalloc(dev,
			     struct_size(priv, sensor, num_sensors),
			     GFP_KERNEL);
	...
}

If a device tree provides a #qcom,sensors value less than 11, could this
initialization write beyond the allocated bounds of the array and cause a
heap buffer overflow?

> +
> +	return init_common(priv);
> +}

[Severity: Medium]
If a subsequent step in the probe fails, is the hardware interrupt left
enabled?

init_common() sets the INT_EN hardware interrupt flag.

If a later step in tsens_probe() fails, the driver returns directly without
cleaning up the interrupt:

drivers/thermal/qcom/tsens.c:tsens_probe() {
	...
		ret = priv->ops->calibrate(priv);
		if (ret < 0)
			return dev_err_probe(dev, ret, "%s: calibration failed\n",
					     __func__);
	...
	ret = tsens_register(priv);
	if (!ret)
		tsens_debug_init(pdev);

	return ret;
}

If the interrupt fires after the probe exits with an error, the Linux IRQ
subsystem will receive it without a registered handler.

Could this lead to a temporary interrupt storm until the kernel disables the
IRQ line?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011-msm8952-initial-support-v5-0-1857074992ec@mainlining.org?part=4