Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Heap Out-Of-Bounds Write in `init_8952` when device tree overrides `#qcom,sensors`. - [High] Use-After-Free via Section Mismatch: `init_8952` is annotated with `__init`, but can be called after boot memory is freed. - [Medium] Potential thermal calibration inaccuracy by replacing SoC-specific logic with hardcoded generics. --
commit 7ec3bedeaa96c98b6f844188546432e42095ad77 Author: Muzaffer Kadir <[email protected]> thermal: qcom: tsens: Add support for MSM8952 tsens Add MSM8952 support for tsens. Downstream does a lot of advanced things with calculations. But seems like using tsens-v1 directly works too. 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..f4201748ea297 100644 > --- a/drivers/thermal/qcom/tsens-v1.c > +++ b/drivers/thermal/qcom/tsens-v1.c > @@ -145,6 +145,37 @@ 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] Since the tsens driver probe can be deferred due to nvmem calibration data dependencies, or the device could be manually unbound and rebound via sysfs, could this __init annotation cause a use-after-free? The ops_8952 struct stores the pointer to this function in .rodata. If the function is freed after boot memory is reclaimed, calling priv->ops->init() during a deferred probe will jump to a freed address. [Severity: Medium] The commit message notes that downstream does advanced calculations, but this implementation replaces them with static generic offsets. Could this lead to thermal calibration inaccuracies? If these static offsets do not accurately reflect the physical thermal curves of all MSM8952 chips, could the kernel read skewed temperatures and fail to act on thermal events? > +{ > + 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[9].p1_calib_offset = 0; > + priv->sensor[9].p2_calib_offset = 0; > + priv->sensor[10].p1_calib_offset = -3; > + priv->sensor[10].p2_calib_offset = -2; [Severity: High] Could this result in a heap out-of-bounds write? In tsens_probe(), num_sensors defaults to 11 but can be overridden by the "#qcom,sensors" device tree property. The priv->sensor array is then dynamically allocated based on this count using struct_size(). If a device tree specifies a num_sensors value less than 11, these hardcoded unconditional assignments will write beyond the bounds of the allocated heap array. > + > + return init_common(priv); > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260912-msm8952-initial-support-v1-0-0e742578b...@mainlining.org?part=17
