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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] thermal: qcom: tsens: heap out-of-bounds write in init_8952() due to unvalidated #qcom,sensors
- [High] thermal: qcom: tsens: use-after-free in tsens_probe() when executing __init init_8952()
--

commit 485835aa537dd437aeac92e15ded8ca82d079ba0
Author: Muzaffer Kadir <muzafferkadir@mainlining.org>

thermal: qcom: tsens: Add support for MSM8952 tsens

Add support for tsens v1 block that exists on MSM8952 with 11 tsens
sensors. Sensor Offsets and slope values are taken from downstream.

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
> --- a/drivers/thermal/qcom/tsens-v1.c
> +++ b/drivers/thermal/qcom/tsens-v1.c
[ ... ]
> +static int __init init_8952(struct tsens_priv *priv)

[Severity: High]
Is it safe to mark init_8952() with __init?

The pointer to this function is stored in the ops_8952 structure below,
which is non-init data. If tsens_probe() is deferred due to calibration
nvmem cell returning -EPROBE_DEFER, or if the device is manually unbound
and rebound via sysfs after boot, it will eventually execute
priv->ops->init(priv).

Could this result in a use-after-free regression by dereferencing
discarded init memory during a later probe?

> +{
> +	for (int i = 0; i < priv->num_sensors; ++i)
> +		priv->sensor[i].slope = 3200;
> +
> +	priv->sensor[0].p1_calib_offset = 2;
> +	priv->sensor[0].p2_calib_offset = -1;
[ ... ]
> +	priv->sensor[10].p1_calib_offset = -3;
> +	priv->sensor[10].p2_calib_offset = -2;

[Severity: High]
Does this sequence unconditionally access up to 11 sensors?

In tsens_probe(), the priv->sensor array is dynamically allocated based
on the num_sensors value, which can be restricted by the device tree:

drivers/thermal/qcom/tsens.c:tsens_probe() {
    ...
    priv = devm_kzalloc(dev,
                 struct_size(priv, sensor, num_sensors),
                 GFP_KERNEL);
    ...
}

If a device tree provides a #qcom,sensors property with a value less
than 11, could these assignments overwrite heap memory and cause a
corruption regression during driver load?

> +
> +	return init_common(priv);
> +}
[ ... ]
> +static const struct tsens_ops ops_8952 = {
> +	.init		= init_8952,
> +	.calibrate	= tsens_calibrate_common,
> +	.get_temp	= get_temp_tsens_valid,
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006-msm8952-initial-support-v4-0-ecdcc1ac620c@mainlining.org?part=4