Re: [RFC PATCH 13/14] hwmon: add support for x86 macs in macsmc-hwmon

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] hwmon: macsmc: NULL pointer dereference in DT sensor label handling
- [High] hwmon: macsmc: macsmc_hwmon_create_fan_acpi() treats positive read return as error
- [High] hwmon: macsmc: unchecked fan setup in macsmc_hwmon_populate_sensors_acpi()
- [High] hwmon: macsmc: endianness bug in macsmc_hwmon_write_old_fan_manual()
- [Medium] hwmon: macsmc: macsmc_hwmon_write_fan() swallows target write errors with > 0
--

commit 4bcde7b8b0bda2dbb66c2bfcaa1b0806af093c48
Author: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>

hwmon: add support for x86 macs in macsmc-hwmon

This commit adds support for Intel Macs to macsmc-hwmon, exposing readable
sensor names for temperature, power, current, and voltage keys, as well as
fan speed reporting and manual control on ACPI platforms.

> diff --git a/drivers/hwmon/macsmc-hwmon.c b/drivers/hwmon/macsmc-hwmon.c
> index 2e74dcd6651b3..29e43ff4b4feb 100644
> --- a/drivers/hwmon/macsmc-hwmon.c
> +++ b/drivers/hwmon/macsmc-hwmon.c
[ ... ]
> @@ -47,7 +52,7 @@ MODULE_PARM_DESC(fan_control,
>  struct macsmc_hwmon_sensor {
>  	struct apple_smc_key_info info;
>  	smc_key macsmc_key;
> -	char label[MAX_LABEL_LENGTH];
> +	char *label;
>  	u32 attrs;
>  };

[Severity: High]
Will changing sensor->label from an inline char array to a pointer cause a
null pointer dereference on Device Tree platforms during
macsmc_hwmon_probe() -> macsmc_hwmon_populate_sensors()?

In macsmc_hwmon_populate_sensors(), the sensor arrays are allocated with
devm_kcalloc(), leaving sensor->label initialized to NULL. When
macsmc_hwmon_create_sensor() runs for a Device Tree node with a label
property, it still writes directly to sensor->label using sizeof(char *):

drivers/hwmon/macsmc-hwmon.c:macsmc_hwmon_create_sensor() {
	...
	ret = of_property_read_string(sensor_node, "label", &label);
	if (ret)
		dev_dbg(dev, "No label found for sensor %s\n", key);
	else
		strscpy_pad(sensor->label, label, sizeof(sensor->label));
}

And if a Device Tree sensor node does not have a label property,
sensor->label remains NULL when macsmc_hwmon_populate_sensors() immediately
dereferences it across the current, power, temperature, and voltage loops:

drivers/hwmon/macsmc-hwmon.c:macsmc_hwmon_populate_sensors() {
	...
	if (!macsmc_hwmon_create_sensor(hwmon->dev, hwmon->smc, key_node, sensor)) {
		sensor->attrs = HWMON_C_INPUT;

		if (*sensor->label)
			sensor->attrs |= HWMON_C_LABEL;
	...
}

[ ... ]
> @@ -599,6 +605,23 @@ static int macsmc_hwmon_read_fan(struct macsmc_hwmon *hwmon, u32 attr, int chan,
>  	}
>  }
>  
> +static int macsmc_hwmon_write_old_fan_manual(struct macsmc_hwmon *hwmon, u8 val, int channel)
> +{
> +	int ret;
> +	u16 buf;
> +
> +	ret = apple_smc_read_u16(hwmon->smc, __SMC_KEY('F', 'S', '!', ' '), &buf);
> +	if (ret)
> +		return ret;
> +
> +	if (val)
> +		buf |= BIT(channel);
> +	else
> +		buf &= ~BIT(channel);
> +
> +	return apple_smc_write_u16(hwmon->smc, __SMC_KEY('F', 'S', '!', ' '), cpu_to_be16(buf));
> +}

[Severity: High]
Should buf be converted with be16_to_cpu() after apple_smc_read_u16() in
macsmc_hwmon_write_old_fan_manual()?

Because apple_smc_read_u16() reads raw big-endian bytes into buf without
byte-swapping, modifying BIT(channel) on the unswapped value and then
passing cpu_to_be16(buf) to apple_smc_write_u16() swaps the high and low
bytes of the manual fan bitmask on little-endian systems. For example, once
fan 0 is placed in manual mode (0x0001 big-endian, read into buf as 0x0100),
clearing BIT(0) on buf is a no-op and writes 0x0001 big-endian back to the
SMC, leaving fan 0 stuck in manual mode.

>  static int macsmc_hwmon_write_fan(struct device *dev, u32 attr, int channel,
>  				  long val)
>  {
[ ... ]
> @@ -625,7 +649,10 @@ static int macsmc_hwmon_write_fan(struct device *dev, u32 attr, int channel,
>  	if (val >= min && val <= max) {
>  		if (!hwmon->fan.fans[channel].manual) {
>  			/* Write 1 to mode key for manual control */
> -			ret = macsmc_hwmon_write_key(hwmon->smc,
> +			if (hwmon->fan.has_old_manual)
> +				ret = macsmc_hwmon_write_old_fan_manual(hwmon, 1, channel);
> +			else
> +				ret = macsmc_hwmon_write_key(hwmon->smc,
>  						     &hwmon->fan.fans[channel].mode, 1);
>  			if (ret < 0)
>  				return ret;
>  
>  			hwmon->fan.fans[channel].manual = true;
>  		}
>  		return macsmc_hwmon_write_key(hwmon->smc,
> -					      &hwmon->fan.fans[channel].set, val);
> +					      &hwmon->fan.fans[channel].set, val) > 0;

[Severity: Medium]
Why is the return value of macsmc_hwmon_write_key() compared with > 0 here
in macsmc_hwmon_write_fan()?

The relational comparison turns any negative error code returned by
macsmc_hwmon_write_key() into 0, causing hwmon_attr_store() to treat failed
writes to the fan target key as successful.

[ ... ]
> @@ -871,6 +1475,57 @@ static int macsmc_hwmon_create_fan(struct device *dev, struct apple_smc *smc,
>  	return 0;
>  }
>  
> +static int macsmc_hwmon_create_fan_acpi(struct apple_smc *smc,
> +					struct macsmc_hwmon_fan *fan, int num)
> +{
> +	const char num_char = num + '0';
> +	int ret;
> +
> +	ret = macsmc_hwmon_create_sensor_acpi(smc, &fan->now,
> +			__SMC_KEY('F', num_char, 'A', 'c'), NULL, 0);
> +	if (ret)
> +		return ret;
> +
> +	if (apple_smc_key_exists(smc, __SMC_KEY('F', num_char, 'I', 'D'))) {
> +		char buf[17];
> +
> +		ret = apple_smc_read(smc, __SMC_KEY('F', num_char, 'I', 'D'), buf, 16);
> +		if (ret)
> +			return ret;

[Severity: High]
Does checking if (ret) after apple_smc_read() cause
macsmc_hwmon_create_fan_acpi() to abort on success whenever the F%dID key is
present?

On ACPI platforms, apple_smc_read() delegates to apple_smc_acpi_read(),
which returns the positive number of bytes read (16) on success:

drivers/platform/x86/macsmc-acpi.c:apple_smc_acpi_read() {
	...
	if (ret < 0)
		return ret;

	return size;
}

When F%dID exists and is read, ret is 16, so macsmc_hwmon_create_fan_acpi()
returns 16 early before initializing fan->label, the min/max/target/mode
sensors, and fan->attrs.

[ ... ]
> @@ -990,6 +1645,101 @@ static int macsmc_hwmon_populate_sensors(struct macsmc_hwmon *hwmon,
[ ... ]
> +static int macsmc_hwmon_populate_sensors_acpi(struct macsmc_hwmon *hwmon)
> +{
> +	int ret, i;
> +
> +	ret = apple_smc_read_u8(hwmon->smc, SMC_KEY(FNum), &hwmon->fan.count);
> +	if (hwmon->fan.count && !ret) {
> +		hwmon->fan.fans = devm_kcalloc(hwmon->dev, hwmon->fan.count,
> +				sizeof(struct macsmc_hwmon_fan), GFP_KERNEL);
> +		if (!hwmon->fan.fans)
> +			return -ENOMEM;
> +
> +		hwmon->fan.has_old_manual = apple_smc_key_exists(hwmon->smc,
> +				__SMC_KEY('F', 'S', '!', ' '));
> +
> +		for (i = 0; i < hwmon->fan.count; i++) {
> +			macsmc_hwmon_create_fan_acpi(hwmon->smc, &hwmon->fan.fans[i], i);
> +			if (ret)
> +				return ret;
> +		}
> +	}

[Severity: High]
Was ret meant to capture the return value of macsmc_hwmon_create_fan_acpi()
in macsmc_hwmon_populate_sensors_acpi()?

Because ret is not assigned when calling macsmc_hwmon_create_fan_acpi(),
if (ret) always checks the stale 0 from apple_smc_read_u8(), ignoring fan
initialization failures and leaving hwmon->fan.fans[i].attrs as 0.

Also, if apple_smc_read_u8() fails after modifying hwmon->fan.count,
hwmon->fan.count can remain non-zero while hwmon->fan.fans is still NULL,
which would later dereference NULL in macsmc_hwmon_create_infos() ->
macsmc_hwmon_populate_fan_configs().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029085504.3547-1-atharvatiwarilinuxdev@gmail.com?part=13