Re: [PATCH 2/6] thermal: add hardware-binning trip-point filtering support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] thermal: thermal_zone_set_supported_hw_bin() unconditionally fails with -EINVAL
- [Medium] thermal: broken post-registration hardware binning API
- [Medium] thermal: TOCTOU double-fetch race and UAF in thermal_zone_get_hw_bin_info()
- [Low] thermal: unreachable subgroup match failure in thermal_of_trip_read_temperature()
--

commit 6467f849e176530b564d3eca94d59d40b3f0f114
Author: Amit Kucheria <amit.kucheria@oss.qualcomm.com>

thermal: add hardware-binning trip-point filtering support

Differences in SoC packaging can be used to provide multiple SoC SKUs at
different thermal and performance characteristics. The underlying SoC
remains the same except for how much heat the packaging can dissipate.
This patch adds thermal core/OF framework support to populate appropriate
trip points applicable to an SKU at runtime.

> diff --git a/Documentation/driver-api/thermal/hardware-binning.rst b/Documentation/driver-api/thermal/hardware-binning.rst
> index 0000000000000..fcb8cdd8b2d17
> --- /dev/null
> +++ b/Documentation/driver-api/thermal/hardware-binning.rst
[ ... ]
> +After registration
> +  Call ``thermal_zone_set_supported_hw_bin()`` (or the devm-managed
> +  ``devm_thermal_zone_set_supported_hw_bin()``) on an already-registered
> +  zone::

[Severity: Medium]
The documentation states thermal_zone_set_supported_hw_bin() can be called
after zone registration to provide hardware binning data.

However, OF trips are parsed synchronously during registration in
thermal_of_zone_register_with_bin(). Calling this API afterwards only updates
the struct but fails to re-evaluate the already-parsed trips. This could
cause the thermal core to fall back to default temperatures.

Additionally, when using devm_thermal_of_zone_register_with_bin(), the
binning info is not saved to tz->hw_bin_info, causing
thermal_zone_get_hw_bin_info() to incorrectly return NULL. Are there missing
updates to re-evaluate trips and store the binning info?

> +
> +      int thermal_zone_set_supported_hw_bin(struct thermal_zone_device *tz,
[ ... ]
> diff --git a/drivers/thermal/thermal_bin.c b/drivers/thermal/thermal_bin.c
> index 0000000000000..ef09619813696
> --- /dev/null
> +++ b/drivers/thermal/thermal_bin.c
[ ... ]
> +int thermal_zone_set_supported_hw_bin(struct thermal_zone_device *tz,
> +				      const u32 *vers, unsigned int count)
> +{
> +	u32 *copy;
> +
> +	if (WARN_ON(!tz || !vers || !count || count > THERMAL_HW_BIN_MAX_LEVELS))
> +		return -EINVAL;
> +
> +	/*
> +	 * The copy is allocated against the zone's parent device so that it
> +	 * is freed when that device goes away.  Reject zones registered
> +	 * without a parent rather than scribbling on dev=NULL inside devm.
> +	 */
> +	if (!tz->device.parent)
> +		return -EINVAL;

[Severity: High]
Does this check structurally guarantee failure?

The function checks if (!tz->device.parent) to allocate devm memory against
the parent device, returning -EINVAL if NULL. However, the thermal core
allocates struct thermal_zone_device dynamically in
thermal_zone_device_register_with_trips() and never sets tz->device.parent
(thermal zones are registered as virtual sysfs devices).

Because this seems to always be NULL, this API appears permanently unusable
for any platform driver. Should the memory be allocated against a different
device or managed differently?

> +
> +	copy = devm_kmemdup(tz->device.parent, vers, count * sizeof(u32),
> +			    GFP_KERNEL);
[ ... ]
> +const struct thermal_hw_bin_info *
> +thermal_zone_get_hw_bin_info(const struct thermal_zone_device *tz)
> +{
> +	/* Pairs with smp_store_release() in thermal_zone_set_supported_hw_bin() */
> +	if (!tz || !smp_load_acquire(&tz->hw_bin_info.supported_hw_bin))
> +		return NULL;
> +
> +	return &tz->hw_bin_info;

[Severity: Medium]
Does returning the struct pointer here introduce a double-fetch race?

thermal_zone_get_hw_bin_info() uses an acquire load to check
tz->hw_bin_info.supported_hw_bin but returns a pointer to the mutable
structure instead of a safe array pointer.

Callers must fetch the array pointer a second time. Concurrently,
devm_thermal_zone_clear_supported_hw_bin() locklessly clears the pointer
during unbind:

	smp_store_release(&tz->hw_bin_info.supported_hw_bin, NULL);
	tz->hw_bin_info.supported_hw_bin_count = 0;

After this returns, devm frees the memory. The caller's second fetch can
dereference NULL or freed memory. Should this API return the snapshot of
the array pointer instead to avoid a use-after-free?

> +}
> +EXPORT_SYMBOL_GPL(thermal_zone_get_hw_bin_info);
> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> index fe47d74b7219e..8cc5b5aa141cd 100644
> --- a/drivers/thermal/thermal_of.c
> +++ b/drivers/thermal/thermal_of.c
[ ... ]
> +static int thermal_of_trip_read_temperature(struct device_node *np,
> +					    const struct thermal_hw_bin_info *hw,
> +					    int *temp)
> +{
[ ... ]
> +	for (i = 0; i < ngroups; i++) {
> +		if (thermal_of_match_hw_bin_group(np, hw, levels, i)) {
> +			u32 utmp;
> +
> +			if (of_property_read_u32_index(np, "temperature-bin",
> +						       i, &utmp))
> +				return -EINVAL;
> +			*temp = (s32)utmp;
> +			return 0;
> +		}
> +	}
> +
> +	pr_warn("%pOF: no thermal-hw-bin sub-group matched for temperature-bin\n", np);
> +	return -ENODEV;

[Severity: Low]
Is this failure path unreachable?

This function loops to match a hardware bin subgroup, logging a warning and
returning -ENODEV if no match is found. However, this function is only
called from thermal_of_populate_trip() when thermal_of_trip_is_supported()
returns true.

Since thermal_of_trip_is_supported() executes the exact same match validation
loop beforehand, it seems guaranteed that the loop will find a match. Can
this dead code be removed, or is there a case where it could still be
reached?

> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002-thermal_bin-v1-0-5a29be7e8ec5@oss.qualcomm.com?part=2