[PATCH] thermal: sysfs: take the zone lock in the tzp attribute handlers

[PATCH] thermal: sysfs: take the zone lock in the tzp attribute handlers

[Yu Zhang]

The thermal zone parameter (tz->tzp) sysfs attributes - sustainable_power,
k_po, k_pu, k_i, k_d, integral_cutoff, slope and offset - dereference
tz->tzp without holding the zone lock:

	if (!tz->tzp)
		return -EIO;
	...
	tz->tzp->name = value;

For a zone registered without thermal zone parameters (tzp == NULL at
registration - the thermal testing zones, ACPI thermal and every driver
using thermal_zone_device_register_with_trips(..., NULL, ...)),
power_allocator_bind() allocates tz->tzp and power_allocator_unbind()
frees it and sets it back to NULL, both under the zone lock:

	tz->tzp = kzalloc(sizeof(*tz->tzp), GFP_KERNEL);
	...
	params->allocated_tzp = true;
	/* unbind */
	if (params->allocated_tzp) {
		kfree(tz->tzp);
		tz->tzp = NULL;
	}

bind/unbind run from policy_store() -> thermal_zone_device_set_policy()
under the zone lock when the governor is switched to or away from
power_allocator.  The tzp attribute handlers take no lock, so a read or
write of one of these files racing a governor switch dereferences a
freed tz->tzp or a NULL pointer.  The read side (0644) is reachable by
an unprivileged user.

KASAN, one test thermal zone, one loop switching the policy between
power_allocator and step_wise and one loop writing k_pu, panics in
seconds:

  BUG: KASAN: slab-use-after-free in k_pu_store+0xd7/0x130
  Write of size 4 at addr ffff8880064e5c94 by task init
   k_pu_store+0xd7/0x130
   dev_attr_store+0x46/0x70
   sysfs_kf_write+0xed/0x110
   kernfs_fop_write_iter+0x29f/0x380
   vfs_write+0x5df/0x950
   ksys_write+0xfd/0x1e0
  Allocated by task:
   power_allocator_bind+0x51a/0x600
   thermal_set_governor+0x86/0x170
   thermal_zone_device_set_policy+0x58/0xb0
  Freed by task:
   power_allocator_unbind+0xad/0xd0
   thermal_set_governor+0x57/0x170
   thermal_zone_device_set_policy+0x58/0xb0

Take the zone lock in the tzp attribute handlers, as mode_show() and
weight_store() already do ("Don't race with governors using the
'weight' value").  The governor already reads these fields under the
zone lock, so this also serialises the PID constant updates against a
running power_allocator.

Fixes: f5cbb182586e ("thermal: power_allocator: don't require tzp to be present for the thermal zone")
Cc: stable@vger.kernel.org
Signed-off-by: Yu Zhang <yuz08559@gmail.com>
---
 drivers/thermal/thermal_sysfs.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -258,6 +258,8 @@ sustainable_power_show(struct device *de
 {
 	struct thermal_zone_device *tz = to_thermal_zone(dev);
 
+	guard(thermal_zone)(tz);
+
 	if (tz->tzp)
 		return sysfs_emit(buf, "%u\n", tz->tzp->sustainable_power);
 	else
@@ -271,6 +273,8 @@ sustainable_power_store(struct device *d
 	struct thermal_zone_device *tz = to_thermal_zone(dev);
 	u32 sustainable_power;
 
+	guard(thermal_zone)(tz);
+
 	if (!tz->tzp)
 		return -EIO;
 
@@ -289,6 +293,8 @@ sustainable_power_store(struct device *d
 	{								\
 		struct thermal_zone_device *tz = to_thermal_zone(dev);	\
 									\
+		guard(thermal_zone)(tz);				\
+									\
 		if (!tz->tzp)						\
 			return -EIO;					\
 									\
@@ -302,6 +308,8 @@ sustainable_power_store(struct device *d
 		struct thermal_zone_device *tz = to_thermal_zone(dev);	\
 		s32 value;						\
 									\
+		guard(thermal_zone)(tz);				\
+									\
 		if (!tz->tzp)						\
 			return -EIO;					\
 									\