Re: [PATCH v5 2/3] hwmon: (applesmc) Fix lockless cache validation data race

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Race condition in `applesmc_calibrate()` exposes intermediate invalid state for coordinates
- [High] Unprotected read-modify-write cycle in `applesmc_store_fan_manual` can lose configuration updates
- [High] Missing error handling in `applesmc_calibrate()` permanently corrupts resting baseline
- [High] Kernel stack information leak in `applesmc_light_show`
--

--- Patch [2]: [PATCH v5 2/3] hwmon: (applesmc) Fix lockless cache validation data race ---
commit ad485314bce775beb0b1efb51243bec0e40a88b0
Author: Shih-Yuan Lee <fourdollars@debian.org>

hwmon: (applesmc) Fix lockless cache validation data race

This commit addresses a data race in applesmc_get_entry_by_index() where the
valid flag was checked locklessly. It introduces smp_load_acquire() and
smp_store_release() to ensure cache contents are safely committed before
being marked valid.

> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 9b2d9ecb20c09..317135fc4b73b 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c

[ ... ]

> @@ -392,7 +394,8 @@ static const struct applesmc_entry *applesmc_get_entry_by_index(int index)
>  	cache->len = info[0];
>  	memcpy(cache->type, &info[1], 4);
>  	cache->flags = info[5];
> -	cache->valid = true;
> +	/* Pairs with smp_load_acquire() to commit cache contents before setting valid */
> +	smp_store_release(&cache->valid, true);
>  
>  out:
>  	mutex_unlock(&smcreg.mutex);

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a race condition
in applesmc_calibrate() that exposes an intermediate invalid state:

drivers/hwmon/applesmc.c:applesmc_calibrate() {
	...
	applesmc_read_s16(MOTION_SENSOR_X_KEY, &rest_x);
	applesmc_read_s16(MOTION_SENSOR_Y_KEY, &rest_y);
	rest_x = -rest_x;
	...
}

Since rest_x is updated without any locking, could concurrent readers observe
the raw positive value before it gets negated? If applesmc_idev_poll()
executes in this window, it computes the input event using the non-negated
value:

drivers/hwmon/applesmc.c:applesmc_idev_poll() {
	...
	input_report_abs(idev, ABS_X, x - rest_x);
	...
}

[Severity: High]
This is a pre-existing issue, but there appears to be missing error handling
in applesmc_calibrate() that could corrupt the resting baseline:

drivers/hwmon/applesmc.c:applesmc_calibrate() {
	...
	applesmc_read_s16(MOTION_SENSOR_X_KEY, &rest_x);
	applesmc_read_s16(MOTION_SENSOR_Y_KEY, &rest_y);
	rest_x = -rest_x;
	...
}

If the hardware read fails (e.g., transient I/O error), applesmc_read_s16()
leaves rest_x unmodified and returns an error code. Because the return value
is ignored, does this unconditionally execute rest_x = -rest_x and
arbitrarily flip the sign of the baseline on every failed read?

[Severity: High]
This is a pre-existing issue, but I noticed an unprotected read-modify-write
cycle in applesmc_store_fan_manual():

drivers/hwmon/applesmc.c:applesmc_store_fan_manual() {
	...
	ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
	if (ret)
		goto out;

	val = (buffer[0] << 8 | buffer[1]);

	if (input)
		val = val | (0x01 << to_index(attr));
	else
		val = val & ~(0x01 << to_index(attr));

	buffer[0] = (val >> 8) & 0xFF;
	buffer[1] = val & 0xFF;

	ret = applesmc_write_key(FANS_MANUAL, buffer, 2);
	...
}

Since smcreg.mutex is acquired and released internally by applesmc_read_key()
and applesmc_write_key(), the lock is dropped during the modification. Could
concurrent calls for different fans read the same initial bitmask and
subsequently overwrite each other's configuration updates?

[Severity: High]
This isn't a bug introduced by this patch, but does applesmc_light_show()
leak uninitialized kernel stack memory?

drivers/hwmon/applesmc.c:applesmc_light_show() {
	...
	ret = applesmc_read_key(LIGHT_SENSOR_LEFT_KEY, buffer, data_length);
	if (ret)
		goto out;
	/* newer macbooks report a single 10-bit bigendian value */
	if (data_length == 10) {
		left = be16_to_cpu(*(__be16 *)(buffer + 6)) >> 2;
		goto out;
	}
	left = buffer[2];
	...
	ret = applesmc_read_key(LIGHT_SENSOR_RIGHT_KEY, buffer, data_length);
	if (ret)
		goto out;
	right = buffer[2];

out:
	if (ret)
		return ret;

	return sysfs_emit(sysfsbuf, "(%d,%d)\n", left, right);
}

If the hardware reports a data_length of 1 or 2 bytes, only those bytes are
written into the uninitialized stack array buffer. Since the code then
unconditionally reads buffer[2], is it emitting uninitialized stack memory
to userspace via sysfs_emit()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711075754.11358-1-fourdollars@debian.org?part=2